feat: add support for specifying model alias used in requests and responses - #2615
feat: add support for specifying model alias used in requests and responses#2615Christopher-Chianelli wants to merge 5 commits into
Conversation
Reviewer's GuideAdds a new optional --alias CLI flag for File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
is_healthy, alias unconditionally overriding themodel_nameargument means explicit callers passingmodel_namecannot win; consider only usingargs.aliaswhenmodel_nameisNoneto avoid surprising behavior for existing callers. - In
_connect_and_chat,chat_args.model = getattr(args, 'alias', f"{self.model_organization}/{self.model_name}")will setmodel=Nonewhenargs.aliasexists but isNone; using something likechat_args.model = args.alias or f"{self.model_organization}/{self.model_name}"would avoid accidentally nulling out the model name.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `is_healthy`, alias unconditionally overriding the `model_name` argument means explicit callers passing `model_name` cannot win; consider only using `args.alias` when `model_name` is `None` to avoid surprising behavior for existing callers.
- In `_connect_and_chat`, `chat_args.model = getattr(args, 'alias', f"{self.model_organization}/{self.model_name}")` will set `model=None` when `args.alias` exists but is `None`; using something like `chat_args.model = args.alias or f"{self.model_organization}/{self.model_name}"` would avoid accidentally nulling out the model name.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new --alias command-line option for the run and serve commands, allowing users to specify a custom model name for API interactions. The implementation includes updates to documentation, CLI parsing, health check logic, and inference engine plugins for llama.cpp and vllm. A critical issue was identified in the transport layer where the absence of an alias could lead to the model name being set to None, potentially breaking chat functionality; a suggestion was made to use the existing model_alias property as a fallback.
| # Model name in the chat request must match RamalamaModelContext.alias() | ||
| chat_args = copy.deepcopy(args) | ||
| chat_args.model = f"{self.model_organization}/{self.model_name}" | ||
| chat_args.model = getattr(args, 'alias', f"{self.model_organization}/{self.model_name}") |
There was a problem hiding this comment.
This implementation introduces a regression when the --alias argument is not provided. In argparse, the default value for an optional argument is None. The getattr(args, 'alias', default) function returns the attribute's value if it exists, even if that value is None. As a result, chat_args.model will be set to None instead of falling back to the default model name, which will break the chat functionality for standard runs.
Additionally, you can leverage the existing self.model_alias property to simplify the code and ensure consistency with other parts of the codebase.
| chat_args.model = getattr(args, 'alias', f"{self.model_organization}/{self.model_name}") | |
| chat_args.model = getattr(args, 'alias', None) or self.model_alias |
There was a problem hiding this comment.
I agree it is reasonable, although I am more interested in why existing unit and e2e tests pass without it (as well as ramalama run and ramalama chat).
This suggests either:
- The attribute
chat_args.modelis unused. - The code path that reads
chat_args.modelis untested.
As I mentioned in the PR description, this change is unnecessary for the feature to work, so another option would be to just remove this particular change. But I am more interesting in knowing why this particular attribute does not appear to matter.
There was a problem hiding this comment.
(there a third option that it fails on a e2e that was skipped on my machine for a feature my machine does not support, but beside the lint failing on perp and other man pages, I don't see any failing tests on the GH runners).
There was a problem hiding this comment.
unit tests and and e2e tests don't run when lint fails. If you enable actions on your github fork it should run the tests on your branches when you push them
GitHub Actions
Inactive
|
/ok-to-test |
| help="name of container in which the Model will be run", | ||
| completer=suppressCompleter, | ||
| ) | ||
| parser.add_argument( |
There was a problem hiding this comment.
This is specific to inference runtimes so should move to BaseInferenceRuntime, or possible need to add this in llama.cpp and vllm plugins as I don't think mlx supports it
|
|
||
| conn = None | ||
| if getattr(args, "alias", None): | ||
| model_name = args.alias |
There was a problem hiding this comment.
Only applies to llama.cpp and vllm plugins so should do this in their service_ready_check methods.
| # Model name in the chat request must match RamalamaModelContext.alias() | ||
| chat_args = copy.deepcopy(args) | ||
| chat_args.model = f"{self.model_organization}/{self.model_name}" | ||
| chat_args.model = getattr(args, 'alias', f"{self.model_organization}/{self.model_name}") |
There was a problem hiding this comment.
unit tests and and e2e tests don't run when lint fails. If you enable actions on your github fork it should run the tests on your branches when you push them
|
@olliewalsh In regards to linting locally, make lint
! git grep -n -- '#!/usr/bin/python3' -- ':!Makefile'
ruff check ramalama scripts test bin/ramalama
All checks passed!
shellcheck *.sh */*.sh */*/*.sh
/bin/sh: line 1: shellcheck: command not found
make: *** [Makefile:119: lint] Error 127Should I create an issue/PR to add |
|
Additionally, the |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds support for overriding the model alias in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
087298c to
51fe235
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
ramalama/plugins/runtimes/inference/llama_cpp.py (1)
227-230: Tighten alias matching in readiness check to avoid false-positive model matches.Line 228 currently uses substring matching, so aliases like
localcan matchlocal2. Also, Line 230 logsmodel_nameeven though the effective key ismodel_id.Proposed patch
- model_id = getattr(args, 'alias', None) or model_name - if not any(model_id in name for name in model_names): + model_id = getattr(args, "alias", None) or model_name + expected_ids = {model_id} + if isinstance(model_id, str): + expected_ids.update(part.strip() for part in model_id.split(",") if part.strip()) + + if not any(name in expected_ids for name in model_names): logger.debug( - f'{self.name} {container_name} /models does not include "{model_name}" in the model list: {model_names}' + f'{self.name} {container_name} /models does not include "{model_id}" in the model list: {model_names}' ) return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ramalama/plugins/runtimes/inference/llama_cpp.py` around lines 227 - 230, The readiness check currently uses substring matching (any(model_id in name for name in model_names)) which can produce false positives; change it to an exact membership test (model_id not in model_names) so aliases like "local" don't match "local2", and update the logger.debug call to reference model_id (the effective key) instead of model_name; locate the check around model_id/model_name/model_names in the readiness path of llama_cpp.py and adjust the condition and log message accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/ramalama-sandbox-goose.1.md`:
- Line 24: Under the "## OPTIONS" section the heading "#### **--alias**" jumps
from h2 to h4 causing MD001; change that specific heading "#### **--alias**" to
an h3 (### **--alias**) or add an intermediate h3 container so option headings
are normalized (ensure all option headings under "## OPTIONS" use the same
level, e.g., ###).
In `@docs/ramalama-sandbox-opencode.1.md`:
- Line 24: The heading level for the "--alias" section is incorrect (skips from
h2 to h4), triggering MD001; locate the "#### **--alias**" heading and change it
to an h3 (e.g., "### **--alias**") or alternatively adjust surrounding option
headings so all headings form a proper h2→h3→h4 hierarchy, ensuring consistent
nesting for the "--alias" section.
In `@ramalama/plugins/runtimes/inference/vllm.py`:
- Around line 99-100: The docstring for VllmPlugin._add_inference_args
incorrectly mentions "llama.cpp-specific inference args"; update this docstring
to say "vLLM-specific inference args" (or similar wording referencing vLLM) so
it accurately describes the method in VllmPlugin and avoid confusing references
to llama.cpp.
In `@ramalama/transports/base.py`:
- Line 502: The assignment to chat_args.model should normalize args.alias when
it may contain multiple comma-separated aliases; update the logic that sets
chat_args.model (currently using getattr(args, 'alias', None) or
self.model_alias) to split the alias string on commas, strip whitespace, and
pick the first non-empty token (falling back to self.model_alias if none), so
chat requests always carry a single model identifier.
---
Nitpick comments:
In `@ramalama/plugins/runtimes/inference/llama_cpp.py`:
- Around line 227-230: The readiness check currently uses substring matching
(any(model_id in name for name in model_names)) which can produce false
positives; change it to an exact membership test (model_id not in model_names)
so aliases like "local" don't match "local2", and update the logger.debug call
to reference model_id (the effective key) instead of model_name; locate the
check around model_id/model_name/model_names in the readiness path of
llama_cpp.py and adjust the condition and log message accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f6b12ce5-f1aa-456d-937d-9a71efd2e829
📒 Files selected for processing (11)
docs/ramalama-sandbox-goose.1.mddocs/ramalama-sandbox-opencode.1.mddocs/ramalama-serve.1.mdramalama/cli.pyramalama/plugins/runtimes/inference/llama_cpp.pyramalama/plugins/runtimes/inference/llama_cpp_commands.pyramalama/plugins/runtimes/inference/vllm.pyramalama/transports/base.pytest/e2e/test_run.pytest/unit/test_engine.pytest/unit/test_inference_engine_plugins.py
| # Model name in the chat request must match RamalamaModelContext.alias() | ||
| chat_args = copy.deepcopy(args) | ||
| chat_args.model = f"{self.model_organization}/{self.model_name}" | ||
| chat_args.model = getattr(args, 'alias', None) or self.model_alias |
There was a problem hiding this comment.
Normalize multi-alias input before setting chat model ID.
Line 502 forwards args.alias verbatim into chat_args.model. For llama.cpp, --alias may be comma-separated, but chat requests should carry one model identifier; otherwise model matching can fail.
💡 Proposed fix
- chat_args.model = getattr(args, 'alias', None) or self.model_alias
+ alias = getattr(args, "alias", None)
+ if isinstance(alias, str) and getattr(args, "runtime", "") == "llama.cpp" and "," in alias:
+ alias = alias.split(",", 1)[0].strip()
+ chat_args.model = alias or self.model_alias🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ramalama/transports/base.py` at line 502, The assignment to chat_args.model
should normalize args.alias when it may contain multiple comma-separated
aliases; update the logic that sets chat_args.model (currently using
getattr(args, 'alias', None) or self.model_alias) to split the alias string on
commas, strip whitespace, and pick the first non-empty token (falling back to
self.model_alias if none), so chat requests always carry a single model
identifier.
There was a problem hiding this comment.
Valid-ish; issue is vLLM parses --served-model-name differently, where model1,model2' will be treated as the model name (to do multiple model names in vLLM, you must do --served-model-name model1 model2instead. As such, doing a llama.cpp fix here will breakvLLM`. Then again, this field does not seem to matter in tests.
There was a problem hiding this comment.
@Christopher-Chianelli, good point — my proposed fix already gates on getattr(args, "runtime", "") == "llama.cpp" specifically to avoid touching the vLLM path, so it wouldn't cause the comma-as-literal-name breakage you described for vLLM.
That said, your concern about args.runtime reliability is valid — if that attribute isn't consistently set when _connect_and_chat is called, the guard itself could silently mis-fire. And since you confirm the field doesn't appear to affect test outcomes currently, this is a low-priority concern.
Two options worth considering:
-
Leave as-is for now, and track the multi-alias normalization as a follow-up. If someone actually uses a comma-separated llama.cpp alias and then tries to use the
runchat mode, they'd see a model-not-found error — which is a real but edge-case failure. -
Apply a minimal, runtime-agnostic fix — always take the first comma-separated token, since neither llama.cpp nor vLLM would register
"alias1,alias2"as a single valid model name in the models endpoint:alias = getattr(args, "alias", None) if alias and "," in alias: alias = alias.split(",", 1)[0].strip() chat_args.model = alias or self.model_alias
This is safe for vLLM too, since vLLM would never accept a comma-joined string as a valid registered model name anyway.
Would you like to open a follow-up issue for this, or apply option 2 here?
There was a problem hiding this comment.
@CodeRabbit runtimes are plugins, you must not place runtime specific conditionals in the core modules
There was a problem hiding this comment.
@olliewalsh, understood and noted — runtime-specific conditionals belong in the plugin layer, not in core modules. My earlier suggestion gating on "llama.cpp" was incorrect.
The only valid fix here would be the runtime-agnostic approach: always normalize the alias by taking the first comma-separated token, since no runtime registers a comma-joined string as a valid model name:
alias = getattr(args, "alias", None)
if alias and "," in alias:
alias = alias.split(",", 1)[0].strip()
chat_args.model = alias or self.model_aliasGiven Christopher-Chianelli's note that this field doesn't appear to affect current tests, I'd suggest applying this normalization here or tracking it as a follow-up issue. Would you like me to open a follow-up issue for this?
(ᵔ◡ᔸ)🐇 Noted — keeping the core clean of plugin concerns.
✏️ Learnings added
Learnt from: olliewalsh
Repo: containers/ramalama PR: 2615
File: ramalama/transports/base.py:502-502
Timestamp: 2026-04-15T14:38:52.257Z
Learning: In the containers/ramalama repository, runtimes are implemented as plugins. Runtime-specific conditionals (e.g., checking for "llama.cpp", "vllm", etc.) must never be placed in core modules such as `ramalama/transports/base.py`. Runtime-specific logic must live in the plugin layer (e.g., under `ramalama/plugins/runtimes/`).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: olliewalsh
Repo: containers/ramalama PR: 2627
File: test/e2e/test_run.py:300-303
Timestamp: 2026-04-15T14:34:25.915Z
Learning: In this repo’s Python 3.10+ compatibility layer, always import `NamedTemporaryFile` from `ramalama.compat` (not from `tempfile`) so that the `delete_on_close` behavior is correctly backported for Python 3.10/3.11. When you need the temporary file to remain accessible by filesystem path within the `with` block, pass `delete_on_close=False` to `NamedTemporaryFile`. Use this pattern consistently (as in `test/unit/test_inference_engine_plugins.py`, `test/unit/test_common.py`, `ramalama/engine.py`, and `ramalama/stack.py`) to avoid Python-version-specific failures.
There was a problem hiding this comment.
I think it's fine to pass the value through to llama.cpp/vllm verbatim
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
docs/ramalama-sandbox-goose.1.md (1)
24-24:⚠️ Potential issue | 🟡 MinorFix the heading level jump under
## OPTIONS.
##to####still skips one level and keeps MD001 active.Proposed fix
-#### **--alias** +### **--alias**🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ramalama-sandbox-goose.1.md` at line 24, Under the "## OPTIONS" section the heading for "--alias" uses "#### **--alias**" which causes a level jump (MD001); change that heading to "### **--alias**" so it is one level deeper than "## OPTIONS" and resolves the lint warning, and update any adjacent headings in the same block (e.g., other option headings) to maintain consistent heading hierarchy.docs/ramalama-sandbox-opencode.1.md (1)
24-24:⚠️ Potential issue | 🟡 MinorAdjust heading nesting for
--aliasto satisfy MD001.The section still jumps from
##to####.Proposed fix
-#### **--alias** +### **--alias**🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ramalama-sandbox-opencode.1.md` at line 24, The `--alias` section uses a #### heading that skips a level after the preceding ## heading, triggering MD001; change the heading for `--alias` from "#### **--alias**" to "### **--alias**" (or otherwise increment only one level relative to the previous header) so header nesting is sequential and MD001 is satisfied.ramalama/plugins/runtimes/inference/vllm.py (1)
100-100:⚠️ Potential issue | 🟡 MinorUpdate docstring to reference vLLM, not llama.cpp.
This method belongs to
VllmPlugin, so the current wording is misleading.Proposed fix
- """Add llama.cpp-specific inference args to an already-created parser.""" + """Add vLLM-specific inference args to an already-created parser."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ramalama/plugins/runtimes/inference/vllm.py` at line 100, The docstring incorrectly references llama.cpp; update the string in the VllmPlugin method (the method whose docstring is currently """Add llama.cpp-specific inference args to an already-created parser.""") to mention vLLM instead (e.g., "Add vLLM-specific inference args to an already-created parser.") so the documentation accurately reflects VllmPlugin's purpose.ramalama/transports/base.py (1)
502-502:⚠️ Potential issue | 🟠 MajorNormalize alias before assigning
chat_args.model.Passing raw
args.aliascan leak comma-separated values into a field that should be a single model identifier.Proposed fix
- chat_args.model = getattr(args, 'alias', None) or self.model_alias + alias = getattr(args, "alias", None) + if isinstance(alias, str): + alias = next((part.strip() for part in alias.split(",") if part.strip()), None) + chat_args.model = alias or self.model_aliasIn llama.cpp's OpenAI-compatible server, how is `--alias` interpreted when passed as a comma-separated list, and what exact value should clients send in the `model` field for `/v1/chat/completions`?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ramalama/transports/base.py` at line 502, Normalize args.alias before assigning to chat_args.model: when computing chat_args.model (in the code using chat_args.model = getattr(args, 'alias', None) or self.model_alias), split getattr(args, 'alias', '') on commas, trim whitespace, take the first non-empty segment as the model identifier (falling back to self.model_alias if none), and assign that sanitized single identifier to chat_args.model so comma-separated alias values cannot be leaked; clients should therefore send a single model identifier (the first alias token) in the model field for /v1/chat/completions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ramalama/plugins/runtimes/inference/vllm.py`:
- Around line 102-108: The --alias flag is only added when command == "serve"
but _cmd_run reads args.alias, so register the alias argument for the "run"
command as well; update the parser.add_argument call (the one adding "--alias",
dest="alias", help="model name alias", completer=suppressCompleter) so it is
included when command == "run" (or moved out of the serve-only branch into the
shared parser creation) ensuring _cmd_run sees args.alias at runtime.
---
Duplicate comments:
In `@docs/ramalama-sandbox-goose.1.md`:
- Line 24: Under the "## OPTIONS" section the heading for "--alias" uses "####
**--alias**" which causes a level jump (MD001); change that heading to "###
**--alias**" so it is one level deeper than "## OPTIONS" and resolves the lint
warning, and update any adjacent headings in the same block (e.g., other option
headings) to maintain consistent heading hierarchy.
In `@docs/ramalama-sandbox-opencode.1.md`:
- Line 24: The `--alias` section uses a #### heading that skips a level after
the preceding ## heading, triggering MD001; change the heading for `--alias`
from "#### **--alias**" to "### **--alias**" (or otherwise increment only one
level relative to the previous header) so header nesting is sequential and MD001
is satisfied.
In `@ramalama/plugins/runtimes/inference/vllm.py`:
- Line 100: The docstring incorrectly references llama.cpp; update the string in
the VllmPlugin method (the method whose docstring is currently """Add
llama.cpp-specific inference args to an already-created parser.""") to mention
vLLM instead (e.g., "Add vLLM-specific inference args to an already-created
parser.") so the documentation accurately reflects VllmPlugin's purpose.
In `@ramalama/transports/base.py`:
- Line 502: Normalize args.alias before assigning to chat_args.model: when
computing chat_args.model (in the code using chat_args.model = getattr(args,
'alias', None) or self.model_alias), split getattr(args, 'alias', '') on commas,
trim whitespace, take the first non-empty segment as the model identifier
(falling back to self.model_alias if none), and assign that sanitized single
identifier to chat_args.model so comma-separated alias values cannot be leaked;
clients should therefore send a single model identifier (the first alias token)
in the model field for /v1/chat/completions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 098f2a6c-bc26-4818-829a-2e97c2808d79
📒 Files selected for processing (11)
docs/ramalama-sandbox-goose.1.mddocs/ramalama-sandbox-opencode.1.mddocs/ramalama-serve.1.mdramalama/cli.pyramalama/plugins/runtimes/inference/llama_cpp.pyramalama/plugins/runtimes/inference/llama_cpp_commands.pyramalama/plugins/runtimes/inference/vllm.pyramalama/transports/base.pytest/e2e/test_run.pytest/unit/test_engine.pytest/unit/test_inference_engine_plugins.py
✅ Files skipped from review due to trivial changes (1)
- ramalama/cli.py
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/ramalama-serve.1.md
- ramalama/plugins/runtimes/inference/llama_cpp.py
- test/unit/test_engine.py
- test/e2e/test_run.py
- test/unit/test_inference_engine_plugins.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ramalama/plugins/runtimes/inference/vllm.py (1)
102-107: Clarify that--aliasis serve-only in help text.Since registration is gated by
command == "serve", consider making the CLI help explicit to avoid confusion forrun.✏️ Suggested wording tweak
parser.add_argument( "--alias", dest="alias", - help="model name alias (referenced in the requests and responses of the API)", + help="model name alias for serve (referenced in API requests/responses)", completer=suppressCompleter, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ramalama/plugins/runtimes/inference/vllm.py` around lines 102 - 107, The help text for the CLI flag added inside the if command == "serve" block should explicitly state that --alias (parser.add_argument("--alias", dest="alias")) is only valid for the serve command; update the help string to mention “serve-only” or similar wording so users don’t assume it applies to run or other commands—locate the parser.add_argument call for "--alias" inside the command == "serve" branch and modify its help argument accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ramalama/plugins/runtimes/inference/vllm.py`:
- Around line 102-107: The help text for the CLI flag added inside the if
command == "serve" block should explicitly state that --alias
(parser.add_argument("--alias", dest="alias")) is only valid for the serve
command; update the help string to mention “serve-only” or similar wording so
users don’t assume it applies to run or other commands—locate the
parser.add_argument call for "--alias" inside the command == "serve" branch and
modify its help argument accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 85f9975f-f482-4d3b-a691-f8d2428d6fe5
📒 Files selected for processing (1)
ramalama/plugins/runtimes/inference/vllm.py
9f2235c to
84cf84f
Compare
GitHub Actions
Inactive
|
/ok-to-test |
GitHub Actions
Inactive
|
/ok-to-test |
|
LGTM. @olliewalsh have your concerns been addressed? |
| # output only shows subcommands the active runtime actually supports. | ||
| # get_config().runtime is already set by Phase 1 of parse_args_from_cmd | ||
| # before configure_subcommands() is called in Phase 2. | ||
| # These include the subcommands serve and run, which are handled |
There was a problem hiding this comment.
doesn't seem relevant to this PR
There was a problem hiding this comment.
It a note for when a future developer wants to add an arg specific to serve and run; it would of helped me realize where to put the parser code!
| container_id = f"serve_with_alias_{''.join(random.choices(string.ascii_letters + string.digits, k=5))}" | ||
| alias = "my_alias" | ||
| ctx = shared_ctx | ||
| serve_cmd = ["ramalama", "serve", "--name", container_id, "--alias", alias, "--detach", test_model] |
There was a problem hiding this comment.
The unit tests already cover the llama-server cli. Could unit tests cover the model name used in the chat http requests too? If so then I don't think an expensive e2e test is really necessary.
There was a problem hiding this comment.
There is a unit test for the model name in http requests: https://github.com/Christopher-Chianelli/ramalama/blob/54afab654a65c11ae9633b3cb457a05ea7d8dd7a/test/unit/test_engine.py#L193-L210 ;
This e2e is more so to check llama.cpp does not break backwards compatibility (i.e. rename the alias argument, change the format of the /models endpoint, etc.
I usually run |
|
But |
54afab6 to
7df80b7
Compare
|
Question: were do I put the docs? It seems there was a change to the documentation structure, and when I run |
|
Found the relevant commit and made changes to the |
109e9d5 to
67f8cf9
Compare
…ponses
This commit adds an optional `--alias` argument to both `ramalama serve` and
`ramalama run`.
- For the llama.cpp runtime, this sets the --alias argument
- For the vllm runtime, this sets the --served-model-name argument
- For the mlx runtime, this argument is ignored
This argument is useful if you are testing multiple different models,
since you can use the same configuration for them provided they have
the same alias (for example, the "AI Chat" feature in Jetbrains IDEs).
Example Usage:
```bash
ramalama serve --alias local gpt-oss
```
Once the server is up, `http://localhost:8080/v1/models` will use
`local` as the model's name:
```json
{
"models": [
{
"name": "local",
"model": "local",
...
}
],
"data": [
{
"id": "local",
...
}
],
...
}
```
Fixes containers#2591.
Signed-off-by: Christopher Chianelli <christopher@timefold.ai>
Also extended the comment in `cli.py` so future people extending `run` and `serve` know where to find the applicable argument parsers. Signed-off-by: Christopher Chianelli <christopher@timefold.ai>
Signed-off-by: Christopher Chianelli <christopher@timefold.ai>
Additionally, the test now verifies the models endpoint returns the alias instead of the default name. Signed-off-by: Christopher Chianelli <christopher@timefold.ai>
Signed-off-by: Christopher Chianelli <christopher@timefold.ai>
67f8cf9 to
9537d05
Compare
GitHub Actions
Inactive
|
A friendly reminder that this PR had no activity for 30 days. |
4 participants
This commit adds an optional
--aliasargument to bothramalama serveandramalama run.--aliasjudging by the existing code)This argument is useful if you are testing multiple different models, since you can use the same configuration for them provided they have the same alias (for example, the "AI Chat" feature in Jetbrains IDEs).
Example Usage:
ramalama serve --alias local gpt-ossOnce the server is up,
http://localhost:8080/v1/modelswill uselocalas the model's name:{ "models": [ { "name": "local", "model": "local", ... } ], "data": [ { "id": "local", ... } ], ... }Fixes #2591.
Note: The change in Transport (i.e.
# Model name in the chat request must match RamalamaModelContext.alias() chat_args = copy.deepcopy(args) - chat_args.model = f"{self.model_organization}/{self.model_name}" + chat_args.model = getattr(args, 'alias', f"{self.model_organization}/{self.model_name}"))
does not seem necessary (i.e. the new tests pass and
ramalama runworks without it), since it seems the requests made do not reference the model (and the model is only relevant for responses, hence the change to health check, as thellama.cppservice check explicitly check if the model is listed in/models). I can only check thellama.cppruntime though as I run AMD hardware; it might make a difference forvllm. I kept it since the comment explicitly mentions it must match the alias.Note:
llama.cppparses--aliasas a comma seperated list; unsure howvllmdoes it. Unsure if we should fail-fast to ensure consistent support OR allow it despite potentially different behaviours betweenllama.cppandvllm.Summary by Sourcery
Add support for specifying a model alias used in API requests and health checks across run and serve commands.
New Features:
Enhancements:
Documentation:
Tests: