-
Notifications
You must be signed in to change notification settings - Fork 306
Resolves bad rebase for artifact pulling #2473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -185,7 +185,7 @@ def list_models(self) -> list[str]: | |
| if exc.code in (401, 403): | ||
| message = ( | ||
| f"Could not authenticate with {self.provider}." | ||
| "The provided API key was either missing or invalid.\n" | ||
| " The provided API key was either missing or invalid.\n" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's an issue but not related to this feature so I would remove and address in a follow-up
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Come on man.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ditto. If this was a trivial PR then sure. However it is a huge PR. Every unrelated change just reduces the signal to noise ratio, requiring far more effort to identify and review the relevant changes.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also more likely to cause conflicts with other PRs
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR wasn't as large when it was opened in... November? Dan opened his PR with podman in particular after I opened this but we agreed to push his through and rebase this into it which forced the scope to expand. I just don't think you have any appreciation for how much time I've spent rebasing this thing to keep up with main over the past four months with zero eyes.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This isn't a credible concern applied to correcting an errant whitespace.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not sure why that frustration is directed my way when the feature was mostly a collaboration between you and @rhatdan. Resolving merge conflicts when rebasing a branch is never fun. It just can't be avoided sometimes, esp with large PRs or multiple PRs that depend on each other. Cleaning up a bad rebase really is not fun. It is not how I wanted to spend my Friday afternoon. I've pretty much dropped everything to get this PR merged again ASAP before something else conflicts with it.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Because this thread is almost entirely nits. This thread is about an entirely cosmetic whitespace change that, were it a problem, could have been flagged during review. This is not the appropriate context for long drawn out back and forths about these sorts of stylistic preferences.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 yeah, this change is nothing - just was the first of many so the thread spawned from here. Most of the unrelated changes that made this PR quite difficult to review have been addressed now in any case |
||
| f"Set RAMALAMA_API_KEY or ramalama.provider.<provider_name>.api_key." | ||
| ) | ||
| try: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ | |
|
|
||
| import os | ||
| import shlex | ||
| from typing import Optional, Tuple | ||
| from typing import Optional | ||
|
|
||
| from ramalama.common import RAG_DIR, get_accel_env_vars | ||
| from ramalama.file import PlainFile | ||
|
|
@@ -13,9 +13,9 @@ class Compose: | |
| def __init__( | ||
| self, | ||
| model_name: str, | ||
| model_paths: Tuple[str, str], | ||
| chat_template_paths: Optional[Tuple[str, str]], | ||
| mmproj_paths: Optional[Tuple[str, str]], | ||
| model_paths: tuple[str, str], | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could drop this file from the PR |
||
| chat_template_paths: Optional[tuple[str, str]], | ||
| mmproj_paths: Optional[tuple[str, str]], | ||
| args, | ||
| exec_args, | ||
| ): | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,11 +1,13 @@ | ||||||||||
| import json | ||||||||||
| import subprocess | ||||||||||
| from dataclasses import dataclass | ||||||||||
| from datetime import datetime | ||||||||||
| from itertools import chain | ||||||||||
| from typing import TypedDict | ||||||||||
|
|
||||||||||
| import ramalama.annotations as annotations | ||||||||||
| from ramalama.arg_types import EngineArgType | ||||||||||
| from ramalama.common import engine_version, run_cmd | ||||||||||
| from ramalama.common import SemVer, engine_version, run_cmd | ||||||||||
| from ramalama.logger import logger | ||||||||||
|
|
||||||||||
| ocilabeltype = "org.containers.type" | ||||||||||
|
|
@@ -69,21 +71,33 @@ def list_artifacts(args: EngineArgType): | |||||||||
| except subprocess.CalledProcessError as e: | ||||||||||
| logger.debug(e) | ||||||||||
| return [] | ||||||||||
| if output == "": | ||||||||||
| return [] | ||||||||||
|
|
||||||||||
| artifacts = json.loads(f"[{output[:-1]}]") | ||||||||||
| models: list[ListModelResponse] = [] | ||||||||||
| try: | ||||||||||
| artifacts = json.loads(f"[{output[:-1]}]") | ||||||||||
| except json.JSONDecodeError: | ||||||||||
| return [] | ||||||||||
|
|
||||||||||
| models = [] | ||||||||||
| for artifact in artifacts: | ||||||||||
| conman_args = [ | ||||||||||
| args.engine, | ||||||||||
| "artifact", | ||||||||||
| "inspect", | ||||||||||
| artifact["ID"], | ||||||||||
| ] | ||||||||||
| output = run_cmd(conman_args).stdout.decode("utf-8").strip() | ||||||||||
| try: | ||||||||||
| output = run_cmd(conman_args, ignore_stderr=True).stdout.decode("utf-8").strip() | ||||||||||
| except Exception: | ||||||||||
| continue | ||||||||||
|
Comment on lines
+92
to
+93
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Catching a broad
Suggested change
|
||||||||||
|
|
||||||||||
| if output == "": | ||||||||||
| continue | ||||||||||
| inspect = json.loads(output) | ||||||||||
| try: | ||||||||||
| inspect = json.loads(output) | ||||||||||
| except json.JSONDecodeError: | ||||||||||
| continue | ||||||||||
| if "Manifest" not in inspect: | ||||||||||
| continue | ||||||||||
| if "artifactType" not in inspect["Manifest"]: | ||||||||||
|
|
@@ -103,8 +117,12 @@ def list_artifacts(args: EngineArgType): | |||||||||
| def engine_supports_manifest_attributes(engine) -> bool: | ||||||||||
| if not engine or engine == "" or engine == "docker": | ||||||||||
| return False | ||||||||||
| if engine == "podman" and engine_version(engine) < "5": | ||||||||||
| return False | ||||||||||
| if engine == "podman": | ||||||||||
| try: | ||||||||||
| if engine_version(engine) < SemVer(5, 0, 0): | ||||||||||
| return False | ||||||||||
| except Exception: | ||||||||||
| return False | ||||||||||
| return True | ||||||||||
|
|
||||||||||
|
|
||||||||||
|
|
@@ -227,12 +245,67 @@ def list_images(args: EngineArgType) -> list[ListModelResponse]: | |||||||||
|
|
||||||||||
|
|
||||||||||
| def list_models(args: EngineArgType) -> list[ListModelResponse]: | ||||||||||
| conman = args.engine | ||||||||||
| if conman is None: | ||||||||||
| if args.engine is None: | ||||||||||
| return [] | ||||||||||
|
|
||||||||||
| models = list_images(args) | ||||||||||
| models.extend(list_manifests(args)) | ||||||||||
| models.extend(list_artifacts(args)) | ||||||||||
| model_gen = chain(list_images(args), list_manifests(args), list_artifacts(args)) | ||||||||||
|
|
||||||||||
| seen: set[str] = set() | ||||||||||
| models: list[ListModelResponse] = [] | ||||||||||
| for m in model_gen: | ||||||||||
| if (name := m["name"]) in seen: | ||||||||||
| continue | ||||||||||
| seen.add(name) | ||||||||||
| models.append(m) | ||||||||||
| return models | ||||||||||
|
|
||||||||||
|
|
||||||||||
| @dataclass(frozen=True) | ||||||||||
| class OciRef: | ||||||||||
| registry: str | ||||||||||
| repository: str | ||||||||||
| specifier: str # Either the digest or the tag | ||||||||||
| tag: str | None = None | ||||||||||
| digest: str | None = None | ||||||||||
|
|
||||||||||
| def __str__(self) -> str: | ||||||||||
| if self.digest: | ||||||||||
| return f"{self.registry}/{self.repository}@{self.digest}" | ||||||||||
| return f"{self.registry}/{self.repository}:{self.tag or self.specifier}" | ||||||||||
|
|
||||||||||
| @staticmethod | ||||||||||
| def from_ref_string(ref: str) -> "OciRef": | ||||||||||
| return split_oci_reference(ref) | ||||||||||
|
|
||||||||||
|
|
||||||||||
| def split_oci_reference(ref: str, default_registry: str = "docker.io") -> OciRef: | ||||||||||
| ref = ref.strip() | ||||||||||
|
|
||||||||||
| name, digest = ref.split("@", 1) if "@" in ref else (ref, None) | ||||||||||
|
|
||||||||||
| slash = name.rfind("/") | ||||||||||
| colon = name.rfind(":") | ||||||||||
| if colon > slash: | ||||||||||
| name, tag = name[:colon], name[colon + 1 :] | ||||||||||
| else: | ||||||||||
| tag = None | ||||||||||
|
|
||||||||||
| parts = name.split("/", 1) | ||||||||||
| if len(parts) == 1: | ||||||||||
| registry = default_registry | ||||||||||
| repository = parts[0] | ||||||||||
| else: | ||||||||||
| first, rest = parts[0], parts[1] | ||||||||||
| if first == "localhost" or "." in first or ":" in first: | ||||||||||
| registry = first | ||||||||||
| repository = rest | ||||||||||
| else: | ||||||||||
| registry = default_registry | ||||||||||
| repository = name # keep full path | ||||||||||
|
|
||||||||||
| specifier = digest or tag | ||||||||||
| if specifier is None: | ||||||||||
| tag = "latest" | ||||||||||
| specifier = tag | ||||||||||
|
|
||||||||||
| return OciRef(registry=registry, repository=repository, tag=tag, digest=digest, specifier=specifier) | ||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,7 @@ | |
| from ramalama.engine import BuildEngine, Engine, is_healthy, stop_container, wait_for_healthy | ||
| from ramalama.path_utils import get_container_mount_path | ||
| from ramalama.transports.base import Transport | ||
| from ramalama.transports.oci import OCI | ||
| from ramalama.transports.oci.oci import OCI | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||
|
|
||
| INPUT_DIR = "/docs" | ||
|
|
||
|
|
@@ -197,7 +197,6 @@ def serve(self, args: RagArgsType, cmd: list[str]): | |
| stop_container(args.model_args, args.model_args.name, remove=True) | ||
|
|
||
| def run(self, args: RagArgsType, cmd: list[str]): | ||
|
|
||
| args.model_args.name = self.imodel.get_container_name(args.model_args) | ||
| process = self.imodel.serve_nonblocking(args.model_args, self.model_cmd) | ||
| rag_process = self.serve_nonblocking(args, cmd) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g this doesn't look related to artifact pulling, but it's not obvious whether it is an intentional change or a rebase issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just better edge case handling, no grand conspiracy here.