fix: OpenAPI with provider get #1627

yanxi0830 · 2025-03-14T00:14:15Z

What does this PR do?

feat: add provider API for listing and inspecting provider info #1429 introduces GetProviderResponse in OpenAPI, which is not needed, and not correctly defined.

Test Plan

llama-stack-client providers list

cdoern · 2025-03-14T00:21:19Z

llama_stack/distribution/providers.py

            for p in providers:
                if p.provider_id == provider_id:
-                    ret = p
+                    return ProviderInfo(


This is different than inspect. I believe this is what list is supposed to accomplish, right?. a ProviderInfo does not contain a config, llama-stack-client provider inspect ollama will not contain the provider configuration if this change occurs

I see, I think the API endpoint here is not very clear:

since

GET /providers

lists the providers

GET /providers/{provider_id}

I would assume it will return the same information from providers list.

Can we just put config in both list and get to keep consistency?

@raghotham and I had a discussion about this on the initial PR, and decided that the GET /providers with and without a provider_id is cleaner than having providers/inspect and providers or something like that. The single /providers is leaning more towards CRUD.

I feel like putting the config in GET /providers but not using it is not ideal because users will have one experience when using the client and another when using the API directly and receive potentially unwanted information in the response. additionally we would need to add redaction to GET /providers to ensure we aren't leaking information.

I feel like putting the config in GET /providers but not using it is not ideal because users will have one experience when using the client and another when using the API directly and receive potentially unwanted information in the response. additionally we would need to add redaction to GET /providers to ensure we aren't leaking information.

Can you share an example? Are you referring to the llama-stack-client CLI v.s. API discrepancy?

My thinking is:

GET /providers --> client.providers.list() GET /providers/{provider_id} --> client.providers.get(provider_id)

This is natual and aligns with rest of CRUD operation APIs.

As for llama-stack-client:

llama-stack-client providers list llama-stack-client providers get <provider_id>

cdoern · 2025-03-14T00:25:10Z

this breaks the client:

cdoern · 2025-03-14T00:27:55Z

Without GetProvidersResponse, this functionally is llama-stack-client providers list which does return a list of ProviderInfo.

This endpoint providers inspect takes a specific provider, finds it in the stack and returns its ID, type, and config.

You could expand ProviderInfo to contain a config, but that would make llama-stack-client providers list return unprotected configuration that isn't used and isn't redacted but can be pulled out of the request.

Please let me know if I am misinterpreting something!

yanxi0830 · 2025-03-14T00:31:44Z

Without GetProvidersResponse, this functionally is llama-stack-client providers list which does return a list of ProviderInfo.

This endpoint providers inspect takes a specific provider, finds it in the stack and returns its ID, type, and config.

You could expand ProviderInfo to contain a config, but that would make llama-stack-client providers list return unprotected configuration that isn't used and isn't redacted but can be pulled out of the request.

Why can't we just use SafeConfig as well for ProviderInfo?

yanxi0830 · 2025-03-14T00:32:08Z

this breaks the client: <img alt="Screenshot 2025-03-13 at 8 24 52 PM" width="690" src="https://private-user-

The llama-stack-client-python PR will be a follow up.

yanxi0830 · 2025-03-14T00:33:39Z

@cdoern Feel free to take over the PR if you see my point. This is to fix the OpenAPI spec issue we have with GetProviderResponse only being defined once breaking SDK generation.

cdoern · 2025-03-14T00:37:40Z

@yanxi0830 would the following course of action be ok?

keep GetProviderResponse as its own type
fix the OpenAPI schema to properly define GetProviderResponse, its types, etc
keep GET /providers as a list operation that returns ProviderInfo

This solution keeps the operations distinct and separate, with the user aware of the information they are retrieving

Combining into one type seems a bit overloaded in my opinion, but I am happy to fix this in any way!

edit: we can also name it something else like ProviderInfoWithConfig

yanxi0830 · 2025-03-14T00:45:24Z

@cdoern what's your thought on:

I feel like Option 1 is much better?

Option1: single ProviderInfo with config

@json_schema_type
class ProviderInfo(BaseModel):
    api: str
    provider_id: str
    provider_type: str
    config: Dict[str, Any]

Option2 ProviderInfo + ProviderInfoWithConfig

@json_schema_type
class ProviderInfo(BaseModel):
    api: str
    provider_id: str
    provider_type: str

@json_schema_type
class ProviderInfoWithConfig(BaseModel):
    api: str
    provider_id: str
    provider_type: str
    config: Dict[str, Any]

yanxi0830 · 2025-03-14T00:49:00Z

@cdoern If you are ok with my current change, feel free to take over this PR. I can help approve and merge.

I will regen the SDK to fix the error with GetProviderResponse .

cdoern · 2025-03-14T00:49:20Z

@yanxi0830 I think both of those look good, but I am just concerned about returning the config with each list and not using it. I feel like inspect should be the only thing to return a configuration spec while list should return just information.

I think the separate ProviderInfo and ProviderInfoWithConfig (while it looks redundant in code) creates a much better API in regard to what is being returned in each request. What do you think?

Also, thanks for fixing this :)

yanxi0830 · 2025-03-14T00:54:09Z

@yanxi0830 I think both of those look good, but I am just concerned about returning the config with each list and not using it. I feel like inspect should be the only thing to return a configuration spec while list should return just information.

I think the separate ProviderInfo and ProviderInfoWithConfig (while it looks redundant in code) creates a much better API in regard to what is being returned in each request. What do you think?

Also, thanks for fixing this :)

I think list should return same object as get operation. This makes it consistent with rest of CRUD operations.

cdoern · 2025-03-14T00:55:04Z

@yanxi0830 ok, lets do it, this PR looks good to me then. Thanks for being open to the conversation!

llama_stack/apis/providers/providers.py

llama_stack/distribution/providers.py

cdoern

Looks good, just a few questions comments inline. Could we perhaps squash the commits into a message just so we can keep track of these changes? Thanks for fixing this up!

cdoern · 2025-03-14T02:38:02Z

docs/_static/llama-stack-spec.yaml

+        - api
+        - provider_id
+        - provider_type
+        - config


Is config mandatory, or if empty should it be returned as none rather than an empty dictionary?

cdoern · 2025-03-14T02:39:11Z

llama_stack/apis/providers/providers.py

-
-class GetProviderResponse(BaseModel):
-    data: Provider | None
+    config: Dict[str, Any]


Same comment as above, maybe empty should be allowed as none, but this is kind of a nit.

cdoern · 2025-03-14T02:43:32Z

There also probably needs to be a client PR to fix providers inspect to work with these changes. I can follow up with that as well as an integration test for providers inspect in the morning EST.

yanxi0830 · 2025-03-14T03:16:47Z

@cdoern cool! could you fix that as a follow up? tagged you in llamastack/llama-stack-client-python#201

@cdoern

# What does this PR do? - sync with llamastack/llama-stack#1627 [//]: # (If resolving an issue, uncomment and update the line below) [//]: # (Closes #[issue-number]) ## Test Plan ``` llama-stack-client providers list ``` cc @cdoern to fix `llama-stack-client provider get <provider_id>` CLI [//]: # (## Documentation) [//]: # (- [ ] Added a Changelog entry if the change is significant)

leseb

I want to highlight that a similar proposal was made a few weeks ago but was rejected: #1064. Or am I missing something?

Also, I believe this closes #1011?

cdoern · 2025-03-14T10:59:17Z

I want to highlight that a similar proposal was made a few weeks ago but was rejected: #1064. Or am I missing something?

Also, I believe this closes #1011?

@leseb yeah these seem similar. I was going to introduce a 'UserConfig' which would only return fields in the provider config with a JSON schema. But we decided to maybe do that in a follow up if we decided that returning the redacted provider configuration was too much. I can make a draft PR to show how it'd work if you'd like

leseb · 2025-03-14T11:04:35Z

I want to highlight that a similar proposal was made a few weeks ago but was rejected: #1064. Or am I missing something?
Also, I believe this closes #1011?

@leseb yeah these seem similar. I was going to introduce a 'UserConfig' which would only return fields in the provider config with a JSON schema. But we decided to maybe do that in a follow up if we decided that returning the redacted provider configuration was too much. I can make a draft PR to show how it'd work if you'd like

No problem, thanks for the clarification! The redacted part wasn’t really the issue in #1064, but anyway, glad it made it in!

cdoern · 2025-03-14T11:56:13Z

@yanxi0830 llamastack/llama-stack-client-python#202

yanxi0830 added 2 commits March 13, 2025 17:11

fix

d479bc5

fix

9347a64

yanxi0830 marked this pull request as ready for review March 14, 2025 00:14

yanxi0830 requested review from SLR722, ashwinb, dineshyv, dltn, ehhuang, hardikjshah, raghotham, sixianyi0721, terrytangyuan and vladimirivic as code owners March 14, 2025 00:14

facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Mar 14, 2025

yanxi0830 added 2 commits March 13, 2025 17:14

fix

2d1ceba

precommit

9e7c2a1

cdoern suggested changes Mar 14, 2025

View reviewed changes

fix

a91771f

config

194d86a

cdoern approved these changes Mar 14, 2025

View reviewed changes

booxter suggested changes Mar 14, 2025

View reviewed changes

llama_stack/apis/providers/providers.py Outdated Show resolved Hide resolved

llama_stack/distribution/providers.py Show resolved Hide resolved

llama_stack/distribution/providers.py Outdated Show resolved Hide resolved

yanxi0830 mentioned this pull request Mar 14, 2025

correct GET endpoints to not return None #1630

Closed

update

a715deb

yanxi0830 requested review from booxter and cdoern March 14, 2025 02:32

pre

2e9e831

ashwinb approved these changes Mar 14, 2025

View reviewed changes

cdoern reviewed Mar 14, 2025

View reviewed changes

yanxi0830 merged commit 33b096c into main Mar 14, 2025
9 checks passed

yanxi0830 deleted the fix_openapi_provider branch March 14, 2025 02:56

yanxi0830 mentioned this pull request Mar 14, 2025

chore: Sync updates from stainless branch: main llamastack/llama-stack-client-python#201

Merged

leseb reviewed Mar 14, 2025

View reviewed changes

fix: OpenAPI with provider get #1627

fix: OpenAPI with provider get #1627

Uh oh!

Conversation

yanxi0830 commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Test Plan

Uh oh!

cdoern Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanxi0830 Mar 14, 2025

Choose a reason for hiding this comment

Uh oh!

cdoern Mar 14, 2025

Choose a reason for hiding this comment

Uh oh!

yanxi0830 Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdoern commented Mar 14, 2025

Uh oh!

cdoern commented Mar 14, 2025

Uh oh!

yanxi0830 commented Mar 14, 2025

Uh oh!

yanxi0830 commented Mar 14, 2025

Uh oh!

yanxi0830 commented Mar 14, 2025

Uh oh!

cdoern commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanxi0830 commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanxi0830 commented Mar 14, 2025

Uh oh!

cdoern commented Mar 14, 2025 • edited by yanxi0830 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanxi0830 commented Mar 14, 2025

Uh oh!

cdoern commented Mar 14, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cdoern left a comment

Choose a reason for hiding this comment

Uh oh!

cdoern Mar 14, 2025

Choose a reason for hiding this comment

Uh oh!

cdoern Mar 14, 2025

Choose a reason for hiding this comment

Uh oh!

cdoern commented Mar 14, 2025

Uh oh!

Uh oh!

yanxi0830 commented Mar 14, 2025

Uh oh!

leseb left a comment

Choose a reason for hiding this comment

Uh oh!

cdoern commented Mar 14, 2025

Uh oh!

leseb commented Mar 14, 2025

Uh oh!

cdoern commented Mar 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

yanxi0830 commented Mar 14, 2025 •

edited

Loading

cdoern Mar 14, 2025 •

edited

Loading

yanxi0830 Mar 14, 2025 •

edited

Loading

cdoern commented Mar 14, 2025 •

edited

Loading

yanxi0830 commented Mar 14, 2025 •

edited

Loading

cdoern commented Mar 14, 2025 •

edited by yanxi0830

Loading