Skip to content
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

add dynamic clients for all APIs #348

Merged
merged 4 commits into from
Oct 31, 2024
Merged

add dynamic clients for all APIs #348

merged 4 commits into from
Oct 31, 2024

Conversation

ashwinb
Copy link
Contributor

@ashwinb ashwinb commented Oct 30, 2024

What does this PR do?

We have frequently bit-rotten (apis//client.py) files. They have two drawbacks:

  • we need to hand-write these client implementations (work!)
  • they often are poorly written and untested
  • they are accompanied with some useful testing code which is useful for developers but not intended for end users.

This PR is the first step towards killing these hand-written implementations. It dynamically creates Client classes for each API protocol and registers appropriate methods based on type introspection.

Test Plan

First, I ran an ollama server (ollama run llama3.2:3b-instruct-fp16) and then started a Llama Stack using --template ollama on port 5003.

Then I set up the following yaml for testing:

providers:
 - provider_id: remote
   provider_type: remote
   config: 
     host: localhost
     port: 5003

Then I ran the following set of tests:

MODEL_IDS=Llama3.2-3B-Instruct \
PROVIDER_ID=remote \
PROVIDER_CONFIG=config.yaml \
$CONDA_PREFIX/bin/pytest -s llama_stack/providers/tests/inference/test_inference.py

PROVIDER_ID=remote \
PROVIDER_CONFIG=config.yaml \
$CONDA_PREFIX/bin/pytest -s llama_stack/providers/tests/memory/test_memory.py

Then I modified the config.yaml to be:

providers:
  inference:
  - provider_id: remote
    provider_type: remote
    config:
      host: localhost
      port: 5003
  safety:
  - provider_id: remote
    provider_type: remote
    config:
      host: localhost
      port: 5003
  memory:
  - provider_id: remote
    provider_type: remote
    config:
      host: localhost
      port: 5003
  agents:
  - provider_id: remote
    provider_type: remote
    config:
      host: localhost
      port: 5003

And ran the following tests:

MODEL_ID=Llama3.2-3B-Instruct \
PROVIDER_ID=remote \
PROVIDER_CONFIG=config.yaml \
$CONDA_PREFIX/bin/pytest -s llama_stack/providers/tests/agents/test_agents.py

This test did not fully pass due to an unexpected model response from the ollama 3b-instruct llama model w.r.t. tool calling but tests which didn't exercise tool calling passed.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Oct 30, 2024
@@ -239,7 +247,9 @@ async def chat_completion(
response_format: Optional[ResponseFormat] = None,
stream: Optional[bool] = False,
logprobs: Optional[LogProbConfig] = None,
) -> Union[ChatCompletionResponse, ChatCompletionResponseStreamChunk]: ...
) -> Union[
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FINALLY! we actually type-hint it in the way it is supposed to be.

This might become an issue with our OpenAPI generator, but we will fix that downstream. Our source must be always correct.

return APIClient


async def example(model: str = None):
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 will be deleting this code later since someone is going to stumble on this and use it inadvertently again

"content": {
"text/event-stream": {
"schema": {
"$ref": "#/components/schemas/AgentTurnResponseStreamChunk"
"$ref": "#/components/schemas/Turn"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this looks bad though uh oh, need to check

@ashwinb ashwinb merged commit 37b330b into main Oct 31, 2024
2 checks passed
@ashwinb ashwinb deleted the dynamic_client branch October 31, 2024 21:46
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.

3 participants