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

API spec update, client demo with Stainless SDK #58

Merged
merged 7 commits into from
Sep 9, 2024

Conversation

yanxi0830
Copy link
Contributor

@yanxi0830 yanxi0830 commented Sep 9, 2024

  1. Update openapi generator to generate *RequestWrapper & OpenAPI spec.
  2. Demo client scripts with Stainless SDK.
Screen.Recording.2024-09-09.at.11.47.25.AM.mov
Screen.Recording.2024-09-08.at.12.44.08.PM.mov

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Sep 9, 2024
@@ -468,12 +468,14 @@ def _build_operation(self, op: EndpointOperation) -> Operation:
builder = ContentBuilder(self.schema_builder)
first = next(iter(op.request_params))
request_name, request_type = first

from dataclasses import make_dataclass

if len(op.request_params) == 1 and "Request" in first[1].__name__:
Copy link
Contributor

Choose a reason for hiding this comment

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

can you just remove the if completely? Then you will get a class named FooRequestRequest but that's OK. It is an implementation detail anyway.

Copy link
Contributor Author

@yanxi0830 yanxi0830 Sep 9, 2024

Choose a reason for hiding this comment

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

I tried remove the if, but it needs some work to generate the *Wrapper class for the request param. E.g. 838ab91. So figured this may be the fastest way.

@@ -0,0 +1,40 @@
import fire
Copy link
Contributor

Choose a reason for hiding this comment

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

@hardikjshah do you have any opinions on how we should situate these files?

Copy link
Contributor Author

@yanxi0830 yanxi0830 Sep 9, 2024

Choose a reason for hiding this comment

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

Another option is to put them in the SDK repos: https://github.com/stainless-sdks/llama-stack-python/tree/yanxi0830/dev

Copy link
Contributor

@hardikjshah hardikjshah Sep 9, 2024

Choose a reason for hiding this comment

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

I think its better to move these out of llama_toolchain directory as this is not part of the toolchain package.
If i am developing in llama_toolchain and making an api change, I want to quickly test with client.py without having to worry about updating the llama_stack package ( or locally installing it )

I think, lets have a top-level examples directory in llama-stack-apps where we have can have a directory for sdk_examples and put these scripts there.

/sdk_examples
-- /inference
---- client.py
-- /agentic_system
---- client.py

So one can easily run python sdk_examples/inference/client.py [ since the developer might have just pip installed the stack sdk pip package and docker container without git cloning this repo ]

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah , the SDK repo's examples folder is probably best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (removed these scripts from the PR)! In the meantime (while SDK repo is getting ready), adding the sdk_examples to llama-stack-apps in meta-llama/llama-stack-apps#66.

Copy link
Contributor

@ashwinb ashwinb left a comment

Choose a reason for hiding this comment

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

lg

@yanxi0830 yanxi0830 merged commit 4f021de into main Sep 9, 2024
3 checks passed
@yanxi0830 yanxi0830 deleted the client_stainless_sdk branch September 9, 2024 20:09
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.

4 participants