Skip to content

Conversation

@ashwinb
Copy link
Contributor

@ashwinb ashwinb commented Nov 6, 2025

This dependency has been bothering folks for a long time (cc @leseb). We really needed it due to "library client" which is primarily used for our tests and is not a part of the Stack server. Anyone who needs to use the library client can certainly install llama-stack-client in their environment to make that work.

Updated the notebook references to install llama-stack-client additionally when setting things up.

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Meta Open Source bot. label Nov 6, 2025
Copy link
Collaborator

@leseb leseb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the extact same branch locally. 😅

You also need to edit pre-commit, it was failing for me:

      - id: provider-codegen
        name: Provider Codegen
        additional_dependencies:
          - uv==0.7.8
        entry: ./scripts/uv-run-with-index.sh run --extra client --group codegen ./scripts/provider_codegen.py

And change the test setup to use --extra client.

"sqlalchemy[asyncio]>=2.0.41", # server - for conversations
]

[project.optional-dependencies]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add the client as an optional deps

[project.optional-dependencies]
client = [
    "llama-stack-client>=0.3.0",  # Optional for library-only usage
]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leseb I don't see why this is needed. there is nothing which depends on the client, and provider codegen should not need it either.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mypy on the other hand needs so I will add it there

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also see #4097

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about when we use as a library src/llama_stack/core/library_client.py? This requires both llama-stack and llama-stack-client. So to be it'll be natural in this scenario to do pip install llama-srack[client].

No?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that means the client does get packaged together! That's what we want to avoid.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, optional dependencies do not automatically get packaged or installed with the main project.
They are only installed if the user explicitly requests them.

wdyt?

I don't consider this a blocker on my end, I'm willing to let it slip if you still strongly disagree. I tread this as a convience / nice to have type of things.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact optional dependencies DO get packaged. They just don't get installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One advantage of bundling is that we can bundle the correct version of the client that's supposed to work with this version of the Stack.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact optional dependencies DO get packaged. They just don't get installed.

Hum, only metadata is packaged, not the dependency code? I've done some tests

One advantage of bundling is that we can bundle the correct version of the client that's supposed to work with this version of the Stack.

Yes!

)
except ImportError as e:
raise ImportError(
"llama-stack-client is not installed. Please install it with `pip install llama-stack-client`."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"llama-stack-client is not installed. Please install it with `pip install llama-stack-client`."
"llama-stack-client is not installed. Please install it with `pip install llama-stack-client` 'uv pip install llama-stack[client]'"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why this is needed, I disagree.

@mergify
Copy link

mergify bot commented Nov 7, 2025

This pull request has merge conflicts that must be resolved before it can be merged. @ashwinb please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Nov 7, 2025
Copy link
Collaborator

@leseb leseb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to block this work with having llama-stack-client as a optional dep to the project. We can revisit later.

Let's just make pre-commit happy, thanks

@leseb
Copy link
Collaborator

leseb commented Nov 7, 2025

@github-actions run precommit

@leseb
Copy link
Collaborator

leseb commented Nov 7, 2025

hum has pre-commit bot started its weekend already?

@ashwinb
Copy link
Contributor Author

ashwinb commented Nov 7, 2025

@leseb we had to delete the pre-commit bot. It had a security vulnerability. many pull_request_target just have those unless one is extremely careful.

I will update the PR to do what you suggested.

@mergify mergify bot removed the needs-rebase label Nov 7, 2025
@ashwinb
Copy link
Contributor Author

ashwinb commented Nov 7, 2025

The CI failure for docker is due to Huggingface limiting downloads of the nomic model. Landing.

@ashwinb ashwinb merged commit f49cb0b into llamastack:main Nov 7, 2025
71 of 72 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants