-
Notifications
You must be signed in to change notification settings - Fork 41
chore: use the new CLI to run the server #171
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
Conversation
0.3.0 with llamastack/llama-stack#3625 forces us to use "llama stack run" and the server module doesn't execute the server anymore. Signed-off-by: Sébastien Han <[email protected]>
if python -c " | ||
# Determine which CLI to use based on llama-stack version | ||
VERSION_CODE=$(python -c " |
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.
can we make the script itself a shell HERE document like in
read -r -d '' detect_version_script <<EOT
....
EOT
VERSION_CODE=$(python -c "$detect_version_script")
That makes it easier with formatting etc I guess. You could also use single quote around EOT to avoid variable expansion.
btw, I'm not super happy with having shell scripts that call python code embedded in golang binaries, including hard-coded version numbers.
Also, I wonder why isn't this part of the distribution imnage as the distribution image knows best how to start up itself ?
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.
It's ok if we skip that HERE document for now as it doesn't make this hack much beautifuler anyways :)
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.
The particular change looks good to me, but I really wonder why we need to calculate the start command in the controller and don't let the distribution image startup on its own as it knows best how to do the starting ?
This handles the case where we use the ConfigMap to store the run.yaml, when this happens we need to override the entrypoint to give the path of the run.yaml file. |
I see the use case, but I don't think that this is the only or best solution. Alternatively, the startup script (or heck, even Not something that should be fixed here though, so fine with this PR. |
/lgtm +1 for moving it to distribution image. |
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.
+1 as well - if we can open a tracker for that I can take a look at doing it
Do you mind doing it for me, please? @nathan-weinberg I can work with you on what needs to be done. Thanks for your help! |
0.3.0 with llamastack/llama-stack#3625 forces us to use "llama stack run" and the server module doesn't execute the server anymore. Signed-off-by: Sébastien Han <[email protected]> (cherry picked from commit 90365e7)
0.3.0 with llamastack/llama-stack#3625 forces us to use "llama stack run" and the server module doesn't execute the server anymore.