Artifact Pulling #2043
Open
Artifact Pulling #2043
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ieaves
requested review from
bmahabirbu,
cgruver,
engelmi,
jhjaggars,
maxamillion,
rhatdan and
swarajpande5
as code owners
Contributor
Reviewer's GuideIntroduce a strategy-based OCI transport layer with pure-Python OCI artifact pulling and registry inspection, so OCI images vs artifacts (including RLCR models) are handled uniformly across Podman/Docker and HTTP-download fallbacks, plus broaden and harden artifact-related tests and CLI behavior. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Summary of ChangesHello @ieaves, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request lays the groundwork for more flexible model artifact management by introducing direct OCI artifact pulling capabilities. It provides a pure Python implementation for interacting with OCI registries to fetch model artifacts, serving as an initial step towards addressing broader issues related to generic model artifact handling. The changes integrate this new functionality into the existing Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `ramalama/transports/rlcr.py:63` </location>
<code_context>
+ raise exc
+
+ def _attempt_artifact_pull(self, args) -> bool:
+ registry, reference, _ = self._target_decompose(self.model)
+
+ success = False
</code_context>
<issue_to_address>
**nitpick:** Unused third value from _target_decompose could be omitted for clarity.
Consider renaming the unused variable to '_' to make it clear that it is intentionally discarded. If the third value is never used elsewhere, refactoring _target_decompose to return only the required values may improve clarity.
</issue_to_address>
### Comment 2
<location> `ramalama/transports/oci_artifact.py:117-126` </location>
<code_context>
+
+ hasher = hashlib.sha256()
+
+ with TemporaryFile() as out_file:
+ while True:
+ chunk = response.read(BLOB_CHUNK_SIZE)
+ if not chunk:
+ break
+ out_file.write(chunk)
+ if hash_algo == "sha256":
+ hasher.update(chunk)
+
+ if hash_algo == "sha256" and (actual_hash := hasher.hexdigest()) != expected_hash:
+ raise ValueError(f"Digest mismatch for {digest}: expected {expected_hash}, got {actual_hash}")
+
+ os.replace(out_file.name, dest_path)
+
+ def _prepare_headers(self, headers: dict[str, str] | None = None) -> dict[str, str]:
</code_context>
<issue_to_address>
**issue:** Using TemporaryFile with os.replace may not work as expected on all platforms.
TemporaryFile may not provide a usable file path for os.replace on all platforms. Use NamedTemporaryFile with delete=False for better compatibility, or write directly to dest_path.
</issue_to_address>
### Comment 3
<location> `ramalama/transports/oci_artifact.py:133` </location>
<code_context>
+
+ def _prepare_headers(self, headers: dict[str, str] | None = None) -> dict[str, str]:
+ final_headers = dict() if headers is None else headers.copy()
+ final_headers.setdefault("Authorization", f"Bearer {self._bearer_token}")
+
+ return final_headers
</code_context>
<issue_to_address>
**issue (bug_risk):** Authorization header is set even if _bearer_token is None.
Set the Authorization header only when _bearer_token is not None to avoid sending 'Bearer None', which can cause authentication issues.
</issue_to_address>
### Comment 4
<location> `ramalama/transports/oci_artifact.py:193` </location>
<code_context>
+
+
+def _build_snapshot_files(client: OCIRegistryClient, manifest: dict[str, Any]) -> Iterable[SnapshotFile]:
+ descriptors = manifest.get("layers") or manifest.get("blobs") or []
+ for descriptor in descriptors:
+ digest = descriptor.get("digest")
</code_context>
<issue_to_address>
**issue (bug_risk):** Manifest parsing may miss descriptors if both 'layers' and 'blobs' are present.
Currently, only one of 'layers' or 'blobs' is processed, which may omit descriptors if both exist. Merge both lists to ensure all descriptors are included.
</issue_to_address>
### Comment 5
<location> `ramalama/transports/oci_artifact.py:81` </location>
<code_context>
+ return os.path.relpath(blob_file_path, start=snapshot_dir)
+
+
+class OCIRegistryClient:
+ def __init__(
+ self,
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the custom HTTP and token handling code with the 'requests' library to simplify the implementation.
```markdown
Most of this file is a hand-rolled HTTP client, token parser and chunked downloader. You can preserve exactly the same behavior with far less code (and less nesting) by switching to `requests` (or `httpx`) and its streaming support. Here’s a sketch of how you could collapse most of `_open`, `_request_bearer_token`, `get_manifest()` and `download_blob()`:
```python
import requests
class OCIRegistryClient:
def __init__(self, registry, repository, reference):
self.session = requests.Session()
self.base = f"https://{registry}/v2/{repository}"
self.ref = reference
def _authenticate(self, challenge):
# parse WWW-Authenticate header into dict
params = dict(item.split("=",1) for item in challenge.split(",") if "=" in item)
params = {k: v.strip('"') for k,v in params.items()}
url = params["realm"]
resp = self.session.get(url, params={"service": params["service"], "scope": params.get("scope")})
self.session.headers["Authorization"] = "Bearer " + resp.json().get("token")
def _get(self, url, **kw):
resp = self.session.get(url, **kw)
if resp.status_code == 401 and "Bearer" in resp.headers.get("WWW-Authenticate", ""):
self._authenticate(resp.headers["WWW-Authenticate"])
resp = self.session.get(url, **kw)
resp.raise_for_status()
return resp
def get_manifest(self):
url = f"{self.base}/manifests/{self.ref}"
resp = self._get(url, headers={"Accept": ",".join(MANIFEST_ACCEPT_HEADERS)})
digest = resp.headers.get("Docker-Content-Digest") or f"sha256:{hashlib.sha256(resp.content).hexdigest()}"
return resp.json(), digest
def download_blob(self, digest, dest_path):
url = f"{self.base}/blobs/{digest}"
resp = self._get(url, stream=True)
os.makedirs(os.path.dirname(dest_path), exist_ok=True)
hasher = hashlib.sha256()
with open(dest_path, "wb") as f:
for chunk in resp.iter_content(chunk_size=BLOB_CHUNK_SIZE):
f.write(chunk)
hasher.update(chunk)
algo, _, expected = digest.partition(":")
if algo == "sha256" and hasher.hexdigest() != expected:
raise ValueError(f"Digest mismatch {digest}")
```
Actionable steps:
1. Add `requests` to your dependencies.
2. Replace `urllib.request.urlopen` calls with `self.session.get(...)` (see `_get` above).
3. Use `resp.iter_content(...)` instead of `TemporaryFile` and manual reads.
4. Drop custom header‐merging and WWW-Authenticate parsing in favour of the `_authenticate` helper.
This preserves all existing functionality (manifest fetch + digest fallback, token retry, chunked download + checksum) in ~60 lines instead of 200+.
```
</issue_to_address>
### Comment 6
<location> `ramalama/transports/oci_artifact.py:99-101` </location>
<code_context>
digest = response.headers.get("Docker-Content-Digest")
if not digest:
digest = f"sha256:{hashlib.sha256(manifest_bytes).hexdigest()}"
</code_context>
<issue_to_address>
**suggestion (code-quality):** Use `or` for providing a fallback value ([`use-or-for-fallback`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/use-or-for-fallback))
```suggestion
digest = response.headers.get("Docker-Content-Digest") or f"sha256:{hashlib.sha256(manifest_bytes).hexdigest()}"
```
<br/><details><summary>Explanation</summary>Thanks to the flexibility of Python's `or` operator, you can use a single
assignment statement, even if a variable can retrieve its value from various
sources. This is shorter and easier to read than using multiple assignments with
`if not` conditions.
</details>
</issue_to_address>
### Comment 7
<location> `ramalama/transports/oci_artifact.py:132` </location>
<code_context>
final_headers = dict() if headers is None else headers.copy()
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace `dict()` with `{}` ([`dict-literal`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/dict-literal))
```suggestion
final_headers = {} if headers is None else headers.copy()
```
<br/><details><summary>Explanation</summary>The most concise and Pythonic way to create a dictionary is to use the `{}`
notation.
This fits in with the way we create dictionaries with items, saving a bit of
mental energy that might be taken up with thinking about two different ways of
creating dicts.
```python
x = {"first": "thing"}
```
Doing things this way has the added advantage of being a nice little performance
improvement.
Here are the timings before and after the change:
```
$ python3 -m timeit "x = dict()"
5000000 loops, best of 5: 69.8 nsec per loop
```
```
$ python3 -m timeit "x = {}"
20000000 loops, best of 5: 29.4 nsec per loop
```
Similar reasoning and performance results hold for replacing `list()` with `[]`.
</details>
</issue_to_address>
### Comment 8
<location> `ramalama/transports/rlcr.py:48-50` </location>
<code_context>
self._artifact_downloaded = self._has_artifact_snapshot()
if not self._artifact_downloaded:
self._artifact_downloaded = super().exists()
</code_context>
<issue_to_address>
**suggestion (code-quality):** Use `or` for providing a fallback value ([`use-or-for-fallback`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/use-or-for-fallback))
```suggestion
self._artifact_downloaded = self._has_artifact_snapshot() or super().exists()
```
<br/><details><summary>Explanation</summary>Thanks to the flexibility of Python's `or` operator, you can use a single
assignment statement, even if a variable can retrieve its value from various
sources. This is shorter and easier to read than using multiple assignments with
`if not` conditions.
</details>
</issue_to_address>
### Comment 9
<location> `test/unit/test_rlcr.py:149-150` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 10
<location> `test/unit/test_rlcr.py:162-163` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 11
<location> `ramalama/transports/oci_artifact.py:35-36` </location>
<code_context>
def get_snapshot_file_type(name: str, media_type: str) -> SnapshotFileType:
if name.endswith(".gguf") or media_type.endswith(".gguf"):
return SnapshotFileType.GGUFModel
if name.endswith(".safetensors") or media_type.endswith("safetensors"):
return SnapshotFileType.SafetensorModel
if name.endswith(".mmproj"):
return SnapshotFileType.Mmproj
if name.endswith(".json"):
return SnapshotFileType.Other
return SnapshotFileType.Other
</code_context>
<issue_to_address>
**suggestion (code-quality):** We've found these issues:
- Lift code into else after jump in control flow ([`reintroduce-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/reintroduce-else/))
- Hoist repeated code outside conditional statement ([`hoist-statement-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-statement-from-if/))
```suggestion
```
</issue_to_address>
### Comment 12
<location> `ramalama/transports/oci_artifact.py:145-146` </location>
<code_context>
def _open(self, url: str, headers: dict[str, str] | None = None):
req = urllib.request.Request(url, headers=self._prepare_headers(headers))
try:
return urllib.request.urlopen(req)
except urllib.error.HTTPError as exc:
if exc.code == 401:
www_authenticate = exc.headers.get("WWW-Authenticate", "")
if "Bearer" in www_authenticate:
token = self._request_bearer_token(www_authenticate)
if token:
self._bearer_token = token
req = urllib.request.Request(url, headers=self._prepare_headers(headers))
return urllib.request.urlopen(req)
raise
</code_context>
<issue_to_address>
**suggestion (code-quality):** Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
```suggestion
if token := self._request_bearer_token(www_authenticate):
```
</issue_to_address>
### Comment 13
<location> `ramalama/transports/oci_artifact.py:171` </location>
<code_context>
def _request_bearer_token(self, challenge: str) -> str | None:
scheme, _, params = challenge.partition(" ")
if scheme.lower() != "bearer":
return None
auth_params: dict[str, str] = {}
for item in params.split(","):
if "=" not in item:
continue
key, value = item.strip().split("=", 1)
auth_params[key.lower()] = value.strip('"')
realm = auth_params.get("realm")
if not realm:
return None
query = {}
if "service" in auth_params:
query["service"] = auth_params["service"]
if "scope" in auth_params:
query["scope"] = auth_params["scope"]
else:
query["scope"] = f"repository:{self.repository}:pull"
token_url = realm
if query:
token_url = f"{realm}?{urllib.parse.urlencode(query)}"
req_headers = {"User-Agent": "ramalama/oci-artifact"}
request = urllib.request.Request(token_url, headers=req_headers)
try:
response = urllib.request.urlopen(request)
data = json.loads(response.read().decode("utf-8"))
token = data.get("token") or data.get("access_token")
return token
except urllib.error.URLError as exc:
perror(f"Failed to obtain registry token: {exc}")
return None
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Replace if statement with if expression ([`assign-if-exp`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/assign-if-exp/))
- Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/inline-immediately-returned-variable/))
- Simplify dictionary access using default get ([`default-get`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/default-get/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| def _prepare_headers(self, headers: dict[str, str] | None = None) -> dict[str, str]: | ||
| final_headers = dict() if headers is None else headers.copy() | ||
| final_headers.setdefault("Authorization", f"Bearer {self._bearer_token}") |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Authorization header is set even if _bearer_token is None.
Set the Authorization header only when _bearer_token is not None to avoid sending 'Bearer None', which can cause authentication issues.
|
|
||
|
|
||
| def _build_snapshot_files(client: OCIRegistryClient, manifest: dict[str, Any]) -> Iterable[SnapshotFile]: | ||
| descriptors = manifest.get("layers") or manifest.get("blobs") or [] |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Manifest parsing may miss descriptors if both 'layers' and 'blobs' are present.
Currently, only one of 'layers' or 'blobs' is processed, which may omit descriptors if both exist. Merge both lists to ensure all descriptors are included.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a pure Python implementation for pulling OCI artifacts, integrating it into the RLCR transport as a fallback mechanism. The changes include a new oci_artifact module for fetching OCI manifests and downloading blobs, enhancements to the RamalamaContainerRegistry to enable fallback to OCI artifact pulling, and added unit tests to verify the new functionality. I have provided comments addressing potential improvements in error handling and code clarity.
ramalama/transports/rlcr.py
Outdated
|
|
||
| success = False | ||
| previous_store = self._model_store | ||
| self._model_store = None |
Contributor
There was a problem hiding this comment.
critical: Setting self._model_store to None before assigning the actual store could lead to issues if the model_store property is accessed during the artifact download process. This could cause a NoneType error. Consider assigning the store directly without setting it to None first.
success = False
store = self.model_store # Assign directly
previous_store = self._model_store
Comment on lines
+126
to
+133
| if hash_algo == "sha256" and (actual_hash := hasher.hexdigest()) != expected_hash: | ||
| raise ValueError(f"Digest mismatch for {digest}: expected {expected_hash}, got {actual_hash}") |
Contributor
There was a problem hiding this comment.
high: Consider using logger.error instead of raising a ValueError to allow the program to continue execution, possibly with a default value or alternative approach. This can prevent abrupt termination and provide a more graceful degradation of functionality. If the digest mismatch is critical and the application cannot proceed without a valid digest, then re-raising the exception is appropriate.
Comment on lines
+188
to
+207
| perror(f"Failed to obtain registry token: {exc}") | ||
| return None |
Contributor
There was a problem hiding this comment.
medium: It would be helpful to include the URL in the error message to provide more context for debugging.
Suggested change
| perror(f"Failed to obtain registry token: {exc}") | |
| return None | |
| perror(f"Failed to obtain registry token from {token_url}: {exc}") | |
| return None |
ramalama/transports/rlcr.py
Outdated
Comment on lines
92
to
93
| except Exception: | ||
| return False |
Contributor
There was a problem hiding this comment.
medium: Catching Exception is too broad. It might hide important errors. Consider catching more specific exceptions like OSError or ValueError that are more likely to occur when accessing the model store.
try:
_, cached_files, complete = self.model_store.get_cached_files(self.model_tag)
return complete and bool(cached_files)
except (OSError, ValueError): # Catch specific exceptions
return False
Member
|
@ieaves still working on this? I think this along with the podman PR which is waiting for podman 5.7 release should move us forward with artifact support. |
Collaborator
Author
|
@rhatdan I'm taking a look at your podman changes now. I need to adapt this to use
|
ieaves
force-pushed
the
feat/artifact-pulling
branch
from
06ba53b to
7c52164
Compare
Collaborator
Author
|
@rhatdan I rebased this to your branch and took a shot at integrating a hierarchy of decision strategies depending on the users configuration. The basic structure is supposed to be
Then, if the user is running docker we pipe the model file into the container since we can't use artifact/image mounting the same way we can with podman. I haven't done extensive testing but I've got things working with both docker and podman testing against one of my models at |
|
A friendly reminder that this PR had no activity for 30 days. |
Member
|
@ieaves still working on this? Can this be rebased after my Artifact PR was merged? |
ieaves
force-pushed
the
feat/artifact-pulling
branch
from
a80a118 to
f8b340d
Compare
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com> Signed-off-by: Ian Eaves <ian.k.eaves@gmail.com>
Signed-off-by: Ian Eaves <ian.k.eaves@gmail.com>
Signed-off-by: Ian Eaves <ian.k.eaves@gmail.com>
Signed-off-by: Ian Eaves <ian.k.eaves@gmail.com>
Signed-off-by: Ian Eaves <ian.k.eaves@gmail.com>
Signed-off-by: Ian Eaves <ian.k.eaves@gmail.com>
Signed-off-by: Ian Eaves <ian.k.eaves@gmail.com>
Signed-off-by: Ian Eaves <ian.k.eaves@gmail.com>
Signed-off-by: Ian Eaves <ian.k.eaves@gmail.com>
ieaves
force-pushed
the
feat/artifact-pulling
branch
from
277e569 to
21dd483
Compare
Signed-off-by: Ian Eaves <ian.k.eaves@gmail.com>
Signed-off-by: Ian Eaves <ian.k.eaves@gmail.com>
Signed-off-by: Ian Eaves <ian.k.eaves@gmail.com>
Contributor
There was a problem hiding this comment.
Hey - I've found 10 issues, and left some high level feedback:
- In
OCIRegistryClient._prepare_headers, you always set anAuthorization: Bearer Noneheader when no token is present; this should only be added once_bearer_tokenis non-None to avoid sending a bogus auth header on the initial request. - The new top-level
is_oci(self) -> TypeGuard["OCI"]intransports/base.pyreferencesOCIwithout importing it and is a bit misleadingly named for a free function; consider either importingOCIand giving the function a less method-like name (e.g.is_oci_transport(obj)) or inlining themodel_type == 'oci'check where used. - The updated
list_artifactsnow callsrun_cmdwithout ignoring stderr and no longer catchesCalledProcessError, so on older Podman versions or engines withoutartifact lssupport this may now raise instead of returning an empty list; if backward compatibility is desired, you may want to restore the defensive error handling here.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `OCIRegistryClient._prepare_headers`, you always set an `Authorization: Bearer None` header when no token is present; this should only be added once `_bearer_token` is non-None to avoid sending a bogus auth header on the initial request.
- The new top-level `is_oci(self) -> TypeGuard["OCI"]` in `transports/base.py` references `OCI` without importing it and is a bit misleadingly named for a free function; consider either importing `OCI` and giving the function a less method-like name (e.g. `is_oci_transport(obj)`) or inlining the `model_type == 'oci'` check where used.
- The updated `list_artifacts` now calls `run_cmd` without ignoring stderr and no longer catches `CalledProcessError`, so on older Podman versions or engines without `artifact ls` support this may now raise instead of returning an empty list; if backward compatibility is desired, you may want to restore the defensive error handling here.
## Individual Comments
### Comment 1
<location> `ramalama/transports/oci/strategies.py:75-78` </location>
<code_context>
+ self.engine = engine
+ self.model_store = model_store
+
+ def pull(self, ref: str, cmd_args: list[str] | None = None) -> None:
+ if cmd_args is None:
+ cmd_args = []
+ run_cmd([self.engine, "pull", ref, *cmd_args])
+
+ def exists(self, src: str) -> bool:
</code_context>
<issue_to_address>
**issue (bug_risk):** CLI option ordering in pull commands is likely incorrect for Docker/Podman
`run_cmd` currently builds `[self.engine, 'pull', ref, *cmd_args]`, but Docker/Podman expect flags before the image reference (e.g. `podman pull --quiet --tls-verify=false IMAGE`). Since `cmd_args` holds these flags, this ordering may cause pulls to fail or behave incorrectly. Please change to `[self.engine, 'pull', *cmd_args, ref]` here (and in `PodmanArtifactStrategy.pull`) so options precede the image reference.
</issue_to_address>
### Comment 2
<location> `ramalama/transports/base.py:77-78` </location>
<code_context>
return f"No ref file found for '{self.model}'. Please pull model."
+def is_oci(self) -> TypeGuard["OCI"]:
+ return self.model_type == "oci"
+
+
</code_context>
<issue_to_address>
**suggestion:** Module-level is_oci TypeGuard is defined with a misleading signature and implicit type assumptions
This helper is defined at module scope but takes `self` and assumes a `model_type` attribute, which makes it look like a method rather than a standalone function. It also uses `TypeGuard["OCI"]` without importing `OCI`. To make the typing and intent clearer, either type it as `def is_oci(transport: Transport) -> TypeGuard[OCI]:` with `OCI` imported, or inline `self.model_type == "oci"` at the call sites instead of using a separate TypeGuard function.
Suggested implementation:
```python
def is_oci(transport: "Transport") -> TypeGuard["OCI"]:
"""
Type guard to determine whether a given transport is an OCI transport.
This assumes the transport exposes a `model_type` attribute and that
OCI-based transports set `model_type` to the string `"oci"`.
"""
return getattr(transport, "model_type", None) == "oci"
```
1. Ensure `TypeGuard` is imported from `typing` at the top of `ramalama/transports/base.py`, e.g.:
`from typing import TYPE_CHECKING, TypeGuard`
2. Confirm that the concrete transport base class is named `Transport` in this module. If it has a different name, update the parameter type annotation and forward reference in the `is_oci` signature accordingly.
3. Update any call sites that currently use `is_oci(self)` to call `is_oci(self)` in a context where `self` is a `Transport` instance (no change in usage needed, but worth verifying they are indeed `Transport` objects).
</issue_to_address>
### Comment 3
<location> `ramalama/oci_tools.py:20-24` </location>
<code_context>
def list_artifacts(args: EngineArgType):
- if args.engine is None:
- raise ValueError("Cannot list artifacts without a provided engine like podman or docker.")
</code_context>
<issue_to_address>
**question (bug_risk):** list_artifacts now raises on podman errors instead of returning an empty list
This now propagates `CalledProcessError` from `podman` instead of returning `[]` on failure, which may break callers relying on best-effort behavior and an empty list on errors. If that change in contract isn’t desired, please reintroduce error handling (e.g., `ignore_stderr=True` and catching the exception) so `list_artifacts` still returns an empty list when listing fails.
</issue_to_address>
### Comment 4
<location> `ramalama/transports/oci/strategies.py:87-85` </location>
<code_context>
+ except Exception:
+ return False
+
+ def remove(self, ref: str, cmd_args: list[str] = []) -> bool:
+ try:
+ run_cmd([self.engine, "manifest", "rm", ref], ignore_stderr=True)
+ return True
+ except Exception:
+ pass
+ try:
+ run_cmd([self.engine, "image", "rm", *cmd_args, ref], ignore_stderr=True)
+ return True
+ except Exception:
+ return False
+
+ def filenames(self, ref: str) -> list[str]:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Mutable default argument for cmd_args in remove methods can lead to subtle bugs
Both `BaseImageStrategy.remove` and the artifact strategy `remove` methods use `cmd_args: list[str] = []`. Even though it isn’t mutated now, a shared list default is error-prone if future changes append or modify it. Please default `cmd_args` to `None` and create a new list inside the method instead.
Suggested implementation:
```python
def remove(self, ref: str, cmd_args: list[str] | None = None) -> bool:
if cmd_args is None:
cmd_args = []
try:
run_cmd([self.engine, "manifest", "rm", ref], ignore_stderr=True)
return True
except Exception:
pass
try:
run_cmd([self.engine, "image", "rm", *cmd_args, ref], ignore_stderr=True)
return True
except Exception:
return False
```
There are likely other `remove` methods in this file or related strategy classes (e.g., `BaseImageStrategy.remove`) that also use `cmd_args: list[str] = []`. For each of those:
1. Update the signature from `cmd_args: list[str] = []` to `cmd_args: list[str] | None = None`.
2. At the start of the method body, add:
```python
if cmd_args is None:
cmd_args = []
```
3. Keep the rest of the logic unchanged.
This will ensure there are no mutable default arguments in any of the `remove` methods.
</issue_to_address>
### Comment 5
<location> `ramalama/transports/oci/oci_artifact.py:31-40` </location>
<code_context>
+BLOB_CHUNK_SIZE = 1024 * 1024
+
+
+def get_snapshot_file_type(name: str, media_type: str) -> SnapshotFileType:
+ if name.endswith(".gguf") or media_type.endswith(".gguf"):
+ return SnapshotFileType.GGUFModel
+ if name.endswith(".safetensors") or media_type.endswith("safetensors"):
+ return SnapshotFileType.SafetensorModel
+ if name.endswith(".mmproj"):
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Media-type checks using endswith on the full string may misclassify some artifacts
The `get_snapshot_file_type` helper assumes `media_type` will contain a filename-like suffix (checking `endswith('.gguf')` / `endswith('safetensors')`). Standard OCI media types (e.g. `application/octet-stream`, custom vendor types) usually won’t, so this can misclassify blobs. Consider either matching `media_type` against explicit known constants or relying solely on the `name` suffix for detection.
```suggestion
def get_snapshot_file_type(name: str, media_type: str) -> SnapshotFileType:
# Rely on the filename suffix only; OCI media types typically do not carry
# file-extension-like suffixes and using endswith on the full media_type
# string can misclassify artifacts.
if name.endswith(".gguf"):
return SnapshotFileType.GGUFModel
if name.endswith(".safetensors"):
return SnapshotFileType.SafetensorModel
if name.endswith(".mmproj"):
return SnapshotFileType.Mmproj
if name.endswith(".json"):
return SnapshotFileType.Other
return SnapshotFileType.Other
```
</issue_to_address>
### Comment 6
<location> `test/system/056-artifact.bats:17` </location>
<code_context>
testmodel=$RAMALAMA_TMPDIR/testmodel
artifact=artifact-test:latest
- run_ramalama rm --ignore ${artifact}
+ run_ramalama ? rm ${artifact}
echo "hello" > ${testmodel}
</code_context>
<issue_to_address>
**issue (bug_risk):** Suspicious `?` in `run_ramalama` invocation will likely break this test
`run_ramalama ? rm ${artifact}` introduces an invalid `?` argument that doesn’t match existing helper usage and will cause the command (and thus the test) to misbehave. Please restore the previous form (e.g., `run_ramalama rm --ignore ${artifact}`) or adjust it to the intended semantics without the `?`.
</issue_to_address>
### Comment 7
<location> `test/system/056-artifact.bats:147` </location>
<code_context>
artifact="artifact-test:latest"
- run_podman artifact rm --ignore ${artifact}
+ run_podman ? podman artifact rm ${artifact}
# Create a regular image
echo "test model" > $RAMALAMA_TMPDIR/testmodel
</code_context>
<issue_to_address>
**issue (bug_risk):** `run_podman ? podman artifact rm` appears malformed and will likely fail
This line doesn’t follow the existing `run_podman` helper pattern: the `?` and repeated `podman` are invalid and will likely cause the test to fail or not run the intended command. It probably should be something like `run_podman artifact rm --ignore ${artifact}` to match the previous behavior.
</issue_to_address>
### Comment 8
<location> `ramalama/transports/oci/strategies.py:16` </location>
<code_context>
+K = TypeVar("K", bound=StrategyKind)
+
+
+class BaseOCIStrategy(Generic[K], ABC):
+ """Interface for artifact handling strategies."""
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the strategy hierarchy, centralizing OCI reference parsing, and extracting shared model-store helpers to reduce duplication and cognitive load without changing behaviour.
You can trim a fair bit of complexity without losing any functionality by:
### 1. Drop the generic base and simplify the hierarchy
`BaseOCIStrategy[K]` + `StrategyKind` + `BaseArtifactStrategy`/`BaseImageStrategy` provides little runtime value and adds cognitive load. You can keep the same behaviour with a non‑generic base and per‑kind subclasses.
For example:
```python
from typing import Literal
from abc import ABC, abstractmethod
StrategyKind = Literal["artifact", "image"]
class BaseOCIStrategy(ABC):
kind: StrategyKind
engine: str
model_store: ModelStore | None
def __init__(self, *, engine: str, model_store: ModelStore | None = None):
self.engine = engine
self.model_store = model_store
@abstractmethod
def pull(self, ref: str, *args, **kwargs) -> None: ...
@abstractmethod
def exists(self, *args, **kwargs) -> bool: ...
@abstractmethod
def mount_arg(self, *args, **kwargs) -> str | None: ...
@abstractmethod
def remove(self, ref: str, *args, **kwargs) -> bool: ...
@abstractmethod
def filenames(self, ref: str) -> list[str]: ...
@abstractmethod
def inspect(self, ref: str) -> str: ...
def entrypoint_path(self, ref: str, mount_dir: str | None = None) -> str:
mount_dir = mount_dir or MNT_DIR
filenames = self.filenames(ref)
if not filenames:
raise ValueError(f"No model files found for {ref}")
return os.path.join(mount_dir, filenames[0])
class BaseArtifactStrategy(BaseOCIStrategy):
kind: Literal["artifact"] = "artifact"
class BaseImageStrategy(BaseOCIStrategy):
kind: Literal["image"] = "image"
def pull(self, ref: str, cmd_args: list[str] | None = None) -> None:
run_cmd([self.engine, "pull", ref, *(cmd_args or [])])
# ... rest unchanged
```
Call sites still get `kind` as a discriminator, but the generics and duplicated `__init__` logic go away.
### 2. Centralize OCI reference parsing
You now have `normalize_reference`, `split_oci_reference`, and `model_tag_from_ref` in this file, and variants in `resolver.py`. Moving that logic into a single helper module keeps behaviour consistent and removes duplication.
E.g. `oci_ref.py`:
```python
# oci_ref.py
def normalize(reference: str) -> str:
if "://" in reference:
return reference.split("://", 1)[1]
return reference
def split(reference: str) -> tuple[str, str]:
normalized = normalize(reference)
if "/" not in normalized:
raise KeyError(
"You must specify a registry for the model in the form "
f"'oci://registry.acme.org/ns/repo:tag', got instead: {reference}"
)
registry, ref = normalized.split("/", 1)
return registry, ref
def model_tag(reference: str) -> str:
_, ref = split(reference)
if "@" in ref:
return ref.split("@", 1)[1]
if ":" in ref.rsplit("/", 1)[-1]:
return ref.rsplit(":", 1)[1]
return "latest"
```
Then in strategies:
```python
from ramalama.transports.oci.oci_ref import split as split_oci_reference, model_tag as model_tag_from_ref
```
This keeps the logic identical but in one place.
### 3. Make “engine vs model_store” responsibilities clearer
Right now `HttpArtifactStrategy` and `PodmanArtifactStrategy` both mix:
- how to talk to the engine/registry, and
- how to resolve into the model_store / snapshot paths.
You can keep the behaviour but pull the model‑store‑specific parts into a helper that the strategies call, so that the strategies focus on “engine interaction”.
For example:
```python
# model_store_helpers.py
def get_model_tag_from_src(src: str) -> str:
return model_tag_from_ref(src)
def model_exists(model_store: ModelStore | None, src: str) -> bool:
if not model_store:
return False
try:
model_tag = get_model_tag_from_src(src)
_, cached_files, complete = model_store.get_cached_files(model_tag)
return complete and bool(cached_files)
except Exception:
return False
```
Then in `HttpArtifactStrategy.exists`:
```python
from ramalama.transports.oci.model_store_helpers import model_exists
def exists(self, src: str) -> bool:
return model_exists(self.model_store, src)
```
This removes duplicated try/except and “interpret cached_files” logic while preserving the external API and semantics.
</issue_to_address>
### Comment 9
<location> `ramalama/transports/oci/strategy.py:45` </location>
<code_context>
+ artifact: BaseArtifactStrategy
+
+
+@lru_cache
+def get_strategy(
+ engine: str, engine_name: SUPPORTED_ENGINES, model_store: ModelStore, kind: Literal['image', 'artifact']
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the OCIStrategyFactory layer by inlining strategy caching/selection into the factory, removing unused types, and isolating version checks to reduce indirection and clarify responsibilities.
You can simplify this layer quite a bit without changing behavior.
### 1. Collapse `get_strategy` + `@lru_cache` into the factory
Right now resolution is split across `OCIStrategyFactory` → `get_strategy` (cached) → `get_engine_*_strategy`. You can keep the factory as the public API and move the strategy instantiation into it, dropping the global cache (or replacing it with a tiny instance cache):
```python
class OCIStrategyFactory:
"""Resolve reference kind and return the appropriate strategy implementation."""
def __init__(
self,
engine: SUPPORTED_ENGINES | Path | str | None,
model_store: ModelStore,
):
if (engine := engine or CONFIG.engine) is None:
raise Exception("OCIStrategyFactory require a valid engine")
self.engine = str(engine)
self.engine_name: SUPPORTED_ENGINES = cast(
SUPPORTED_ENGINES,
os.path.basename(self.engine),
)
self.model_store = model_store
self._type_resolver = oci_resolver.OCITypeResolver(
self.engine,
model_store=self.model_store,
)
# small instance cache instead of global lru_cache
self._image_strategy: BaseImageStrategy | None = None
self._artifact_strategy: BaseArtifactStrategy | None = None
def _get_image_strategy(self) -> BaseImageStrategy:
if self._image_strategy is None:
cls = get_engine_image_strategy(self.engine, self.engine_name)
self._image_strategy = cls(engine=self.engine, model_store=self.model_store)
return self._image_strategy
def _get_artifact_strategy(self) -> BaseArtifactStrategy:
if self._artifact_strategy is None:
cls = get_engine_artifact_strategy(self.engine, self.engine_name)
self._artifact_strategy = cls(engine=self.engine, model_store=self.model_store)
return self._artifact_strategy
def strategies(
self,
kind: Literal["image", "artifact"],
) -> BaseArtifactStrategy | BaseImageStrategy:
if kind == "image":
return self._get_image_strategy()
return self._get_artifact_strategy()
```
Then you can drop `@lru_cache` and `get_strategy` entirely; call sites using `OCIStrategyFactory` stay the same.
### 2. Remove `StrategiesType` (unused abstraction)
`StrategiesType` doesn’t appear to buy anything; you never return/group both strategies together. You can safely remove it:
```python
# delete this, and rely on the concrete return types on methods instead
# class StrategiesType(TypedDict):
# image: BaseImageStrategy
# artifact: BaseArtifactStrategy
```
Type information is already captured by the method signatures (`BaseArtifactStrategy | BaseImageStrategy`).
### 3. Encapsulate the version check
To decouple this module from version parsing details, extract the Podman capability check into a helper. That keeps `get_engine_artifact_strategy` focused on selection logic:
```python
def engine_supports_artifacts(engine: str, engine_name: SUPPORTED_ENGINES) -> bool:
if engine_name != "podman":
return False
version = SemVer.parse(engine_version(engine))
return version >= PODMAN_MIN_ARTIFACT_VERSION
def get_engine_artifact_strategy(
engine: str, engine_name: SUPPORTED_ENGINES
) -> type[BaseArtifactStrategy]:
if engine_supports_artifacts(engine, engine_name):
return PodmanArtifactStrategy
return HttpArtifactStrategy
```
Callers keep the same behavior, but the version/capability concern is clearly isolated.
</issue_to_address>
### Comment 10
<location> `ramalama/transports/oci/resolver.py:1` </location>
<code_context>
import os
import shlex
-from typing import Optional, Tuple
</code_context>
<issue_to_address>
**issue (complexity):** Consider centralizing OCI reference parsing and consolidating resolver/engine helper logic so the resolution flow is simpler and more explicit in one place.
You can reduce the new complexity by reusing existing parsing helpers and tightening the decision tree into fewer, more local helpers.
### 1. Avoid re‑implementing reference parsing
`normalize_reference` and `model_tag_from_reference` here overlap with existing helpers and `_split_reference`:
```python
+ from ramalama.transports.oci.oci_artifact import OCIRegistryClient, _split_reference
```
Instead of introducing new parsing logic, centralize it in a small shared utility and reuse it from both this resolver and `strategies.py` / `oci_artifact.py`.
For example, introduce a shared `oci_ref.py` (or similar) and use it here:
```python
# oci_ref.py (new shared module)
def normalize(reference: str) -> str:
return reference.removeprefix("oci://")
def split(reference: str) -> tuple[str, str]:
# wraps the existing _split_reference
normalized = normalize(reference)
if "/" not in normalized:
return normalized, ""
registry, remainder = normalized.split("/", 1)
return registry, remainder
def model_tag(reference: str) -> str:
normalized = normalize(reference)
ref = normalized.split("/", 1)[1] if "/" in normalized else normalized
if "@" in ref:
return ref.split("@", 1)[1]
if ":" in ref.rsplit("/", 1)[-1]:
return ref.rsplit(":", 1)[1]
return "latest"
```
Then in this resolver:
```python
- def normalize_reference(reference: str) -> str:
- return reference.removeprefix("oci://")
+ from ramalama.transports.oci import oci_ref
- def is_registry_reference(reference: str) -> bool:
- reference = normalize_reference(reference)
+ def is_registry_reference(reference: str) -> bool:
+ reference = oci_ref.normalize(reference)
host = reference.split("/", 1)[0]
return "." in host or ":" in host or host == "localhost"
- def fetch_manifest(reference: str) -> Optional[dict]:
- reference = normalize_reference(reference)
+ def fetch_manifest(reference: str) -> Optional[dict]:
+ reference = oci_ref.normalize(reference)
if "/" not in reference:
return None
registry, remainder = reference.split("/", 1)
- repository, ref = _split_reference(remainder)
+ repository, ref = _split_reference(remainder)
...
- def model_tag_from_reference(reference: str) -> str:
- normalized = normalize_reference(reference)
- ...
+ def model_tag_from_reference(reference: str) -> str:
+ return oci_ref.model_tag(reference)
```
This keeps all behavior but makes it obvious that normalization/tag extraction are defined in one place.
### 2. Tighten the decision tree into `resolve`
Right now, the resolution logic is spread across `model_store_has_snapshot`, `resolve_engine_kind`, `manifest_kind`, and `resolve`. You can keep helpers but make the decision tree visible in a single function.
For example, inline the control flow in `resolve` and make helpers clearly local (no extra parsing / branching):
```python
class OCITypeResolver:
...
def resolve(self, reference: str) -> ReferenceKind:
# 1) Model store
if self.model_store and model_store_has_snapshot(self.model_store, reference):
return "artifact"
# 2) Engine
kind = self._engine_kind(reference)
if kind != "unknown":
return kind
# 3) Registry manifest
if is_registry_reference(reference):
return self._manifest_kind(reference)
return "unknown"
def _engine_kind(self, reference: str) -> ReferenceKind:
return resolve_engine_kind(self.engine, reference, runner=self.runner)
def _manifest_kind(self, reference: str) -> ReferenceKind:
return manifest_kind(reference)
```
This keeps your existing helpers but makes the priority order and decision points obvious at a glance.
### 3. Consolidate engine existence checks
`engine_artifact_exists` and `engine_image_exists` both wrap `run_cmd` with `ignore_stderr=True`. You can reduce duplication and make behavior easier to adjust by using a single internal helper:
```python
def _engine_inspect(
engine: str,
subcommand: str,
reference: str,
runner: Callable | None = None,
) -> bool:
runner = runner or run_cmd
try:
runner([engine, subcommand, "inspect", reference], ignore_stderr=True)
return True
except Exception:
return False
def engine_artifact_exists(engine: str, reference: str, runner: Callable | None = None) -> bool:
return _engine_inspect(engine, "artifact", reference, runner=runner)
def engine_image_exists(engine: str, reference: str, runner: Callable | None = None) -> bool:
return _engine_inspect(engine, "image", reference, runner=runner)
```
If you later decide to move engine existence checks fully into the strategy layer, this consolidation makes that change localized and reduces the conceptual overlap between modules.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+75
to
+78
| def pull(self, ref: str, cmd_args: list[str] | None = None) -> None: | ||
| if cmd_args is None: | ||
| cmd_args = [] | ||
| run_cmd([self.engine, "pull", ref, *cmd_args]) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): CLI option ordering in pull commands is likely incorrect for Docker/Podman
run_cmd currently builds [self.engine, 'pull', ref, *cmd_args], but Docker/Podman expect flags before the image reference (e.g. podman pull --quiet --tls-verify=false IMAGE). Since cmd_args holds these flags, this ordering may cause pulls to fail or behave incorrectly. Please change to [self.engine, 'pull', *cmd_args, ref] here (and in PodmanArtifactStrategy.pull) so options precede the image reference.
Comment on lines
+77
to
+78
| def is_oci(self) -> TypeGuard["OCI"]: | ||
| return self.model_type == "oci" |
Contributor
There was a problem hiding this comment.
suggestion: Module-level is_oci TypeGuard is defined with a misleading signature and implicit type assumptions
This helper is defined at module scope but takes self and assumes a model_type attribute, which makes it look like a method rather than a standalone function. It also uses TypeGuard["OCI"] without importing OCI. To make the typing and intent clearer, either type it as def is_oci(transport: Transport) -> TypeGuard[OCI]: with OCI imported, or inline self.model_type == "oci" at the call sites instead of using a separate TypeGuard function.
Suggested implementation:
def is_oci(transport: "Transport") -> TypeGuard["OCI"]:
"""
Type guard to determine whether a given transport is an OCI transport.
This assumes the transport exposes a `model_type` attribute and that
OCI-based transports set `model_type` to the string `"oci"`.
"""
return getattr(transport, "model_type", None) == "oci"- Ensure
TypeGuardis imported fromtypingat the top oframalama/transports/base.py, e.g.:
from typing import TYPE_CHECKING, TypeGuard - Confirm that the concrete transport base class is named
Transportin this module. If it has a different name, update the parameter type annotation and forward reference in theis_ocisignature accordingly. - Update any call sites that currently use
is_oci(self)to callis_oci(self)in a context whereselfis aTransportinstance (no change in usage needed, but worth verifying they are indeedTransportobjects).
Comment on lines
20
to
24
| def list_artifacts(args: EngineArgType): | ||
| if args.engine is None: | ||
| raise ValueError("Cannot list artifacts without a provided engine like podman or docker.") | ||
|
|
||
| if args.engine == "docker": | ||
| if args.engine == "docker" or args.engine is None: | ||
| return [] | ||
|
|
||
| conman_args = [ |
Contributor
There was a problem hiding this comment.
question (bug_risk): list_artifacts now raises on podman errors instead of returning an empty list
This now propagates CalledProcessError from podman instead of returning [] on failure, which may break callers relying on best-effort behavior and an empty list on errors. If that change in contract isn’t desired, please reintroduce error handling (e.g., ignore_stderr=True and catching the exception) so list_artifacts still returns an empty list when listing fails.
| run_cmd([self.engine, "image", "inspect", src], ignore_stderr=True) | ||
| return True | ||
| except Exception: | ||
| return False |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Mutable default argument for cmd_args in remove methods can lead to subtle bugs
Both BaseImageStrategy.remove and the artifact strategy remove methods use cmd_args: list[str] = []. Even though it isn’t mutated now, a shared list default is error-prone if future changes append or modify it. Please default cmd_args to None and create a new list inside the method instead.
Suggested implementation:
def remove(self, ref: str, cmd_args: list[str] | None = None) -> bool:
if cmd_args is None:
cmd_args = []
try:
run_cmd([self.engine, "manifest", "rm", ref], ignore_stderr=True)
return True
except Exception:
pass
try:
run_cmd([self.engine, "image", "rm", *cmd_args, ref], ignore_stderr=True)
return True
except Exception:
return FalseThere are likely other remove methods in this file or related strategy classes (e.g., BaseImageStrategy.remove) that also use cmd_args: list[str] = []. For each of those:
- Update the signature from
cmd_args: list[str] = []tocmd_args: list[str] | None = None. - At the start of the method body, add:
if cmd_args is None: cmd_args = []
- Keep the rest of the logic unchanged.
This will ensure there are no mutable default arguments in any of the remove methods.
| testmodel=$RAMALAMA_TMPDIR/testmodel | ||
| artifact=artifact-test:latest | ||
| run_ramalama rm --ignore ${artifact} | ||
| run_ramalama ? rm ${artifact} |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Suspicious ? in run_ramalama invocation will likely break this test
run_ramalama ? rm ${artifact} introduces an invalid ? argument that doesn’t match existing helper usage and will cause the command (and thus the test) to misbehave. Please restore the previous form (e.g., run_ramalama rm --ignore ${artifact}) or adjust it to the intended semantics without the ?.
|
|
||
| artifact="artifact-test:latest" | ||
| run_podman artifact rm --ignore ${artifact} | ||
| run_podman ? podman artifact rm ${artifact} |
Contributor
There was a problem hiding this comment.
issue (bug_risk): run_podman ? podman artifact rm appears malformed and will likely fail
This line doesn’t follow the existing run_podman helper pattern: the ? and repeated podman are invalid and will likely cause the test to fail or not run the intended command. It probably should be something like run_podman artifact rm --ignore ${artifact} to match the previous behavior.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are a number of open issues related to pulling and running generic model artifacts with a number of open technical questions related to each. Among which include
podman artifactAdd support for podman artifacts into Ramalama #1152This PR does not attempt to address all of these question. Given the scope of the problem though, I thought having some code to look at might form a useful jumping off point to begin picking off problems one at a time.
As pertains standards, one path is to implement a system capable of identifying discrete packaging strategies (like modelpack) but so far as I'm aware docker doesn't provide equivalent functionality to
podman artifact. To that end, I've taken a rough shot at a pure python implementation for oci_artifacts.I've attached some of this code to the rlcr transport option only because I have a variety of oras packaged model artifacts (e.g. rlcr://gemma3-270m:gguf) I could test against already.
I just want to reiterate this PR is only intended to facilitate the broader conversation around artifact pulling. Would it be better to only support podman for artifact running? Are there strong opinions about package structure?
Comments and thoughts would be welcome @rhatdan @engelmi.
Summary by Sourcery
Add pure Python OCI artifact pulling support and integrate it as a fallback in the RLCR transport when standard pulls fail.
New Features:
Enhancements:
Tests:
Summary by Sourcery
Introduce a strategy-based OCI transport that supports both container images and CNAI-compliant artifacts, enabling HTTP-based artifact pulling and registry snapshotting alongside existing Podman/Docker workflows.
New Features:
Enhancements:
Tests: