-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: Fixing prompts import warning #3455
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
fix: Fixing prompts import warning #3455
Conversation
Signed-off-by: Francisco Javier Arceo <farceo@redhat.com>
c2ac317 to
9fc0696
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.
Can you clarify why this is the right fix? Thanks!
16f7686 to
9fc0696
Compare
Let me know if i missed something though! |
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! This mechanism of providable_apis is going to get tricky as we expand, I wonder if we should have a way on the API definition itself to set an API as "providable" or not rather than having the logic here, but that can be done in a follow up.
llama_stack/core/distribution.py
Outdated
| def providable_apis() -> list[Api]: | ||
| routing_table_apis = {x.routing_table_api for x in builtin_automatically_routed_apis()} | ||
| return [api for api in Api if api not in routing_table_apis and api != Api.inspect and api != Api.providers] | ||
| internal_apis = {Api.inspect, Api.providers, Api.prompts} |
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.
yeah, I have this in a branch as well. This is the right fix! thanks
+1 |
| def test_internal_apis_excluded(self): | ||
| """Test that internal APIs are excluded and other APIs are included in providable APIs.""" | ||
| apis = providable_apis() | ||
| internal_apis = {Api.inspect, Api.providers, Api.prompts} | ||
|
|
||
| for internal_api in internal_apis: | ||
| assert internal_api not in apis, f"Internal API {internal_api} should not be in providable_apis" | ||
|
|
||
| included_apis = {Api.inference, Api.safety, Api.agents} | ||
| for api in included_apis: | ||
| assert api in apis, f"API {api} should be in providable_apis" | ||
|
|
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.
while this verifies the change in this PR, this test does not prevent new providers from running into the same issue. I wonder if we can add a test that catches the current issue that's fixed by this PR?
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.
How about a global that can be imported and tested against?
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.
like a fixture? yea i think that can work
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.
mind taking a look at the latest change? if you're good with it, feel free to merge. I confirmed it'll fail the test by raising the import error for the registry (since APIs of this nature should not have a registry).
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 for the fix! One comment on testing
Signed-off-by: Francisco Javier Arceo <farceo@redhat.com>
# What does this PR do?
Fixes this warning in llama stack build:
```bash
WARNING 2025-09-15 15:29:02,197 llama_stack.core.distribution:149 core: Failed to import module prompts: No module named
'llama_stack.providers.registry.prompts'"
```
## Test Plan
Test added
---------
Signed-off-by: Francisco Javier Arceo <farceo@redhat.com>
What does this PR do?
Fixes this warning in llama stack build:
WARNING 2025-09-15 15:29:02,197 llama_stack.core.distribution:149 core: Failed to import module prompts: No module named 'llama_stack.providers.registry.prompts'"Test Plan
Test added