feat: allow for multiple external provider specs #3341
Conversation
cdoern
requested review from
ashwinb,
yanxi0830,
hardikjshah,
raghotham,
ehhuang,
terrytangyuan,
leseb,
bbrowning,
reluctantfuturist,
mattf and
slekkala1
as code owners
meta-cla
bot
added
the
CLA Signed
cdoern
force-pushed
the
module-improve
branch
from
06887fe to
5324cfc
Compare
| # in the case we are building we CANNOT import this module of course because it has not been installed. | ||
| # return a partially filled out spec that the build script will populate. | ||
| registry[Api(provider_api)][provider_type] = spec | ||
| if isinstance(spec, list): |
There was a problem hiding this comment.
where is the corresponding type definition change which changes the type into a ProviderSpec | list[ProviderSpec]?
There was a problem hiding this comment.
this method lives in the external providers, so they can return anything but we will only take it if its one of these two types.
As part of API conformance, it could be a good idea to have these functions standardized.
here is an example of one: https://github.com/trustyai-explainability/llama-stack-provider-lmeval/blob/29b5c9a984733871bdc1ce09b658a93a8369afa0/src/llama_stack_provider_lmeval/provider.py#L9
llama_stack/core/distribution.py
Outdated
| # optionally allow people to pass inline and remote provider specs as a returned list. | ||
| # with the old method, users could pass in directories of specs using overlapping code | ||
| # we want to ensure we preserve that flexibility in this method. | ||
| for adapter in spec: |
There was a problem hiding this comment.
can you have three adapters? what if there is no adapter? adapter implies remote right?
There was a problem hiding this comment.
yeah so this is complicated.
Some providers return a RemoteProviderSpec, some return InlineProviderSpec but since they are all ProviderSpec, they are allowed. and only RemoteProviderSpec has an adapter spec within it.
We should probably combine all of these types to the best of our ability but for now let me add some handling
There was a problem hiding this comment.
but I guess since they both inherit from ProviderSpec, I can depend just on provider_type .
There was a problem hiding this comment.
working on #3378 inspired by this :)
cdoern
force-pushed
the
module-improve
branch
from
5324cfc to
8688d00
Compare
when using the providers.d method of installation users could hand craft their AdapterSpec's to use overlapping code meaning one repo could contain an inline and remote impl. Currently installing a provider via module does not allow for that as each repo is only allowed to have one `get_provider_spec` method with one Spec returned add an optional way for `get_provider_spec` to return a list of `ProviderSpec` where each can be either an inline or remote impl. Note: the `adapter_type` in `get_provider_spec` MUST match the `provider_type` in the build/run yaml for this to work. resolves llamastack#3226 Signed-off-by: Charlie Doern <[email protected]>
cdoern
force-pushed
the
module-improve
branch
from
8688d00 to
4b1cd0f
Compare
What does this PR do?
when using the providers.d method of installation users could hand craft their AdapterSpec's to use overlapping code meaning one repo could contain an inline and remote impl. Currently installing a provider via module does not allow for that as each repo is only allowed to have one
get_provider_specmethod with one Spec returnedadd an optional way for
get_provider_specto return a list ofProviderSpecwhere each can be either an inline or remote impl.Note: the
adapter_typeinget_provider_specMUST match theprovider_typein the build/run yaml for this to work.resolves #3226
Test Plan
once this merges we need to re-enable the external provider test and account for this functionality. Work needs to be done in the external provider repos to support this functionality.