Artifact Pulling #2043
Draft
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 GuideThis PR introduces Python-based OCI artifact pulling support and integrates it as a fallback in the RLCR transport when container pulls fail, along with unit tests validating both the fallback logic and the new OCI artifact download module. Sequence diagram for RLCR transport artifact pulling fallbacksequenceDiagram
participant User
participant RLCRTransport
participant OCI
participant OCIArtifactModule
User->>RLCRTransport: pull(args)
RLCRTransport->>OCI: pull(args)
alt OCI pull fails
RLCRTransport->>OCIArtifactModule: attempt_artifact_pull(args)
OCIArtifactModule->>OCIArtifactModule: download_oci_artifact(...)
OCIArtifactModule-->>RLCRTransport: success/failure
RLCRTransport-->>User: result
else OCI pull succeeds
OCI-->>RLCRTransport: success
RLCRTransport-->>User: result
end
Class diagram for new and updated RLCR and OCI artifact classesclassDiagram
class RamalamaContainerRegistry {
- _artifact_downloaded: bool
+ pull(args)
+ _attempt_artifact_pull(args) bool
+ _has_artifact_snapshot() bool
+ exists() bool
+ _get_entry_model_path(*args, **kwargs) str
+ setup_mounts(args)
}
RamalamaContainerRegistry --|> OCI
class OCIRegistryClient {
- registry: str
- repository: str
- reference: str
- base_url: str
- _bearer_token: str | None
+ get_manifest() (dict, str)
+ download_blob(digest, dest_path)
+ _prepare_headers(headers)
+ _open(url, headers)
+ _request_bearer_token(challenge)
}
class RegistryBlobSnapshotFile {
+ download(blob_file_path, snapshot_dir) str
- client: OCIRegistryClient
- digest: str
}
RegistryBlobSnapshotFile --|> SnapshotFile
class SnapshotFile {
<<external>>
}
class download_oci_artifact {
<<function>>
}
OCIRegistryClient <.. RegistryBlobSnapshotFile : uses
RegistryBlobSnapshotFile <.. download_oci_artifact : used by
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.
Comment on lines
+117
to
+126
| 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: |
Contributor
There was a problem hiding this comment.
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.
|
|
||
| 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.
Comment on lines
+35
to
+36
| if name.endswith(".json"): | ||
| return SnapshotFileType.Other |
Contributor
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Lift code into else after jump in control flow (
reintroduce-else) - Hoist repeated code outside conditional statement (
hoist-statement-from-if)
Suggested change
| if name.endswith(".json"): | |
| return SnapshotFileType.Other |
Comment on lines
+145
to
+146
| token = self._request_bearer_token(www_authenticate) | ||
| if token: |
Contributor
There was a problem hiding this comment.
suggestion (code-quality): Use named expression to simplify assignment and conditional (use-named-expression)
Suggested change
| token = self._request_bearer_token(www_authenticate) | |
| if token: | |
| if token := self._request_bearer_token(www_authenticate): |
| query = {} | ||
| if "service" in auth_params: | ||
| query["service"] = auth_params["service"] | ||
| if "scope" in auth_params: |
Contributor
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Replace if statement with if expression (
assign-if-exp) - Inline variable that is immediately returned (
inline-immediately-returned-variable) - Simplify dictionary access using default get (
default-get)
![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.
|
|
||
| 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
+127
| 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
+55
to
+60
| except subprocess.CalledProcessError as exc: | ||
| if getattr(args, "dryrun", False): | ||
| raise | ||
| if self._attempt_artifact_pull(args): | ||
| return | ||
| raise exc |
Contributor
There was a problem hiding this comment.
high: Re-raising the original exception after attempting the artifact pull might mask the underlying issue. Consider logging the artifact pull attempt's success or failure for debugging purposes.
Suggested change
| except subprocess.CalledProcessError as exc: | |
| if getattr(args, "dryrun", False): | |
| raise | |
| if self._attempt_artifact_pull(args): | |
| return | |
| raise exc | |
| except subprocess.CalledProcessError as exc: | |
| if getattr(args, "dryrun", False): | |
| raise | |
| if self._attempt_artifact_pull(args): | |
| logger.info("Successfully pulled artifact as fallback.") # Log success | |
| return | |
| logger.error("Artifact pull failed after container pull.", exc_info=True) # Log failure | |
| raise exc |
Comment on lines
+188
to
+189
| 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 |
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
|
Signed-off-by: Daniel J Walsh <[email protected]>
Signed-off-by: Ian Eaves <[email protected]>
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 |
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: