Remove last libexec program #1576
Conversation
rhatdan
requested review from
ericcurtin,
bmahabirbu,
maxamillion,
swarajpande5,
jhjaggars,
cgruver,
slp and
engelmi
as code owners
Reviewer's GuideThis PR removes the legacy libexec wrapper ( Class diagram for refactored argument-building logic in ModelclassDiagram
class Model {
+validate_args(args)
+build_exec_args_bench(args, model_path)
+build_exec_args_serve(args, exec_model_path, chat_template_path="", mmproj_path="")
+vllm_serve(args, exec_model_path)
+llama_serve(args, exec_model_path, chat_template_path, mmproj_path)
}
Model : -get_ramalama_core_path(args, exec_cmd) // removed
Model : -get_cmd_with_wrapper(cmd_arg) // removed (was in common.py)
Model : +vllm_serve(args, exec_model_path) // new
Model : +llama_serve(args, exec_model_path, chat_template_path, mmproj_path) // new
Model : build_exec_args_serve() calls vllm_serve() or llama_serve()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @rhatdan, 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 focuses on streamlining the application's execution model by removing the reliance on a dedicated libexec program for the llama-server component. This involves significant refactoring of how server commands are constructed and invoked, along with corresponding updates to documentation, build configurations, and deployment examples, simplifying the overall architecture.
Highlights
- Elimination of libexec dependency: I've removed the explicit handling and installation of the
ramalama-serve-coreexecutable in thelibexecdirectory. The system now expectsllama-serverto be directly available in the execution environment (e.g., PATH or container entrypoint). - Refactoring of server argument construction: The logic for building command-line arguments for both
llama-serverandvllmserver has been extracted into dedicated, more modular methods (llama_serveandvllm_serve) withinramalama/model.py. - Cleanup of argument parsing and utility functions: The
--max-model-lenargument is no longer an alias for--ctx-sizebut is now a suppressed argument. Additionally, theget_cmd_with_wrapperutility function and relatedsysconfigimport, which were used to locate thelibexecprogram, have been removed. - Documentation and deployment manifest updates: I've updated the
ramalama-serve.1.mddocumentation and the generated Kubernetes/OpenShift deployment examples inramalama/stack.pyto reflect the new command invocation structure, removing all references to thelibexecpath.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
Replace: #1504 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @rhatdan - 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/model.py:533` </location>
<code_context>
+ if args.debug:
+ exec_args += ["-v"]
+
+ if hasattr(args, "webui") and args.webui == "off":
+ exec_args.extend(["--no-webui"])
+
</code_context>
<issue_to_address>
Use of hasattr for args.webui may mask argument parsing issues.
Consider ensuring 'webui' is always set during argument parsing to avoid masking missing arguments.
Suggested implementation:
```python
if args.webui == "off":
exec_args.extend(["--no-webui"])
```
You must ensure that the argument parser (likely in your CLI or main entrypoint) always sets `args.webui`, for example by adding:
```python
parser.add_argument("--webui", choices=["on", "off"], default="on", help="Enable or disable the web UI")
```
This will guarantee that `args.webui` is always present and set to either `"on"` or `"off"`, so the direct check in this file is safe.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if args.debug: | ||
| exec_args += ["-v"] | ||
|
|
||
| if hasattr(args, "webui") and args.webui == "off": |
There was a problem hiding this comment.
suggestion (bug_risk): Use of hasattr for args.webui may mask argument parsing issues.
Consider ensuring 'webui' is always set during argument parsing to avoid masking missing arguments.
Suggested implementation:
if args.webui == "off":
exec_args.extend(["--no-webui"])You must ensure that the argument parser (likely in your CLI or main entrypoint) always sets args.webui, for example by adding:
parser.add_argument("--webui", choices=["on", "off"], default="on", help="Enable or disable the web UI")This will guarantee that args.webui is always present and set to either "on" or "off", so the direct check in this file is safe.
ramalama/model.py
Outdated
| exec_args = [ | ||
| "--model", | ||
| exec_model_path, | ||
| "--port", | ||
| args.port, | ||
| "--max-sequence-length", | ||
| f"{args.context}", | ||
| ] + args.runtime_args |
There was a problem hiding this comment.
issue (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| return exec_args | ||
|
|
||
| def llama_serve(self, args, exec_model_path, chat_template_path, mmproj_path): | ||
| exec_args = ["llama-server"] |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use named expression to simplify assignment and conditional (
use-named-expression) - Replace if statement with if expression (
assign-if-exp)
There was a problem hiding this comment.
Code Review
This pull request removes the ramalama-serve-core wrapper, inlines its logic, and cleans up related packaging, documentation, and CLI options. The changes look good overall, with improvements in code clarity and maintainability. I've provided some minor suggestions for documentation and code style.
ramalama/stack.py
Outdated
| command: [{llama_cmd}] | ||
| args: {llama_args}\ |
There was a problem hiding this comment.
Using a list for the command field is generally preferred for clarity and to avoid shell injection vulnerabilities. However, since llama_cmd is now a string, it should be enclosed in quotes to ensure it's treated as a single command.
| command: [{llama_cmd}] | |
| args: {llama_args}\ | |
| image: {self.args.image} | |
| command: ["{llama_cmd}"] |
rhatdan
force-pushed
the
chat
branch
2 times, most recently
from
9bbb194 to
6ea3caf
Compare
This fixes make validate to not complain about --ctx-size option. No reason to have this available in display, since this is only for users assuming vllm options. Signed-off-by: Daniel J Walsh <[email protected]>
Signed-off-by: Daniel J Walsh <[email protected]>
Summary by Sourcery
Eliminate the external libexec wrapper (
ramalama-serve-core) by inlining its logic into the code and cleaning up related packaging, docs, and CLI options.Enhancements:
vllm_serveandllama_servemethods in model.pybuild_exec_args_serveto delegate to the new helper methods instead of inlining logicllama-serverdirectly with separate command and args lists--max-model-lenCLI flag in favor of the unified--ctx-sizeoptionBuild:
Documentation:
llama-serverinvocation instead of the wrapperChores:
get_ramalama_core_pathandget_cmd_with_wrapperfunctionslibexec/ramalama/ramalama-serve-corebinary and its packaging entries in pyproject.toml