-
Notifications
You must be signed in to change notification settings - Fork 1.1k
refactor: remove container from list of run image types #2178
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
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 a good idea to reduce the number of ways users can run a containerized distribution.
constructing the correct docker command-line is non-trivial, platform & config specific, and easy to get wrong.
this change leaves dead code in start_stack.sh, see https://github.com/meta-llama/llama-stack/blob/main/llama_stack/distribution/start_stack.sh#L114-L163
what about turning https://github.com/meta-llama/llama-stack/blob/main/llama_stack/distribution/start_stack.sh#L152 into a message about what docker command to run?
users may also want a --please-run option to run that docker command for them.
@mattf I am not sure I understand what you mean. It is only cases where users run |
|
I see so you are saying its worth keeping the ability to run containers through I can see a reason to go either way but if it is the case of keeping the functionality to the best of my knowledge there is no mention of this capability in the LS docs. Mostly when container images are concerned the examples given are docker commands. |
https://llama-stack.readthedocs.io/en/latest/distributions/building_distro.html talks about containers in the context of the "via docker" examples (e.g. https://llama-stack.readthedocs.io/en/latest/distributions/self_hosted_distro/ollama.html#via-docker, https://llama-stack.readthedocs.io/en/latest/distributions/self_hosted_distro/meta-reference-gpu.html#via-docker, ...) are inconsistent (e.g. mount handling) and lack info about selinux or when --gpus is needed. all i'm advocating for is an easy, consistent way for users to construct a docker (or podman) command line. |
@mattf As you've said it is difficult for us to provide a one command to suit all. Given the user's platform and specs they may want to run a container with different settings/configs or have a vastly different way they want their container to be configured. I understand your concerns with regards to the "via docker" examples, we can work to improving documentation there with comments on each flag where needed so people are given clearer instructions and are less likely to get confused. I'll update https://github.com/meta-llama/llama-stack/blob/main/llama_stack/distribution/start_stack.sh#L152 to include a message to use an appropriate docker command. |
it's definitely difficult for users. if we can make it easier for users, how difficult it is for us should be irrelevant. how to run these containers on a platform like k8s is even more difficult and currently left as an exercise for the reader. |
@mattf thanks for the review, my initial thought about running a containerized distro via the Going a bit further, even supporting things like conda/venv makes no sense to me. That is a llama-stack developer convenience more than something "real" users will ever do. Only So coming back to this one, I'd:
Thoughts? @Bobbins228 @mattf |
b045658
to
261b59c
Compare
When I went to write up a warning/example docker command to replace https://github.com/meta-llama/llama-stack/blob/main/llama_stack/distribution/start_stack.sh#L114-L163. I tested it first and it ran just fine. This script is creating a docker command to run the distribution rather than use I am on board with your idea to be more clear with users with regards to building docker commands but it may be out of scope for this issue? I can create a new issue and update the documentation in a follow on PR to clearly explain why the example docker commands are configured the way that they are so we can limit confusion. WDYT? |
@Bobbins228 yes, please, we can do that as a followup. But why this reluctance to delete the code to start the container from |
@leseb I have no problem with removing the code I'll update the PR. 👍 |
261b59c
to
f2309d0
Compare
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.
nits, @ashwinb I'd like your signoff on this, thanks!
f2309d0
to
b05670e
Compare
b05670e
to
93b681b
Compare
4d8ba6c
to
f6efa6f
Compare
Yes this makes sense to me. Run dockers directly. The CLI is really for the "single node" user. If the CLI auto downloaded the dockers we officially distribute, then it would make sense but that's too complicated and doesn't serve anybody's purposes. |
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.
Final nits
@mattf I'll soon merge this, PTAL to make sure all your requested changes have been addressed, thanks! |
f6efa6f
to
f2155db
Compare
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.
Thanks!
|
||
# Execute the command | ||
eval $cmd | ||
echo -e "${RED}Warning: Llama Stack no longer supports running Containers.${NC}" |
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 wording here is a bit confusing. "how come it doesn't support running containers? I just built one!" (here "Llama Stack probably stands for llama stack command
, but it may be not clear to a new user.
f2155db
to
2a39e64
Compare
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.
One nit, thanks!
2a39e64
to
7c51bf8
Compare
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.
/lgtm
stale pending review - please open an issue if any concern arises.
What does this PR do?
[Provide a short summary of what this PR does and why. Link to relevant issues if applicable.]
Removes the ability to run llama stack container images through the llama stack CLI
Closes #2110
Test Plan
[Describe the tests you ran to verify your changes with result summaries. Provide clear instructions so the plan can be easily re-executed.]
Run:
Expected outcome: