-
Notifications
You must be signed in to change notification settings - Fork 0
feat(client): first pass at .run() helper #5
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
Conversation
|
@zeke It's the same version that has been discussed in the staging repo. Do you want to already merge it as-is? |
|
what was the prompt!!? haha |
|
Added context from staging repo: https://github.com/stainless-sdks/replicate-client-python/pull/5 |
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 opening this, @dgellow
A few thoughts:
- Can you add a description to the PR body with a summary of the changes? What's covered? What's missing?
- There seem to be a lot of changes to the auto-generated code here. Are there any mitigations we can make here to minize that? Moving code to lib, adding code comments around human code, moving human code to a separate block in the file, etc.?
- Is it possible to add some tests?
I think that we already extracted to
Edit: Paraphrasing what I said in Slack: I'm really not too sure about adding region comments to delimitate human code vs generated code. The main issue I see is that there is no guarantee they will stay correct over time, it is possible that a git operation ends up with generated code between those comments. So that would be something that you have to maintain yourself. What I would instead recommend is to document branch comparison in CONTRIBUTING.md, such as https://github.com/replicate/replicate-typescript-stainless/compare/generated..next. |
|
Still a bit of work to do on tests — I'm using https://github.com/replicate/replicate-python/blob/main/tests/test_run.py for the list of test cases. |
|
Awesome! Excited to see the progress. |
|
I've created an issue to track this work: I know this PR has been tossed around a bit as folks move on and off of projects, or are on leave. @mpatankar who is the current owner of this? |
|
@RobertCraigie is the assignee and the one who will see this through! Assigned him to the issue you created |
pyproject.toml
Outdated
| "anyio>=3.5.0, <5", | ||
| "distro>=1.7.0, <2", | ||
| "sniffio", | ||
| "asyncio>=3.4.3", |
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.
this isn't quite right, asyncio is stdlib
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.
fixed!
|
Thanks for the update, @dtmeadows. Great to see this getting close! |
|
@zeke this is ready for your review! The only remaining pending item is the file upload functionality that we discussed here: https://stainless-workspace.slack.com/archives/C08H2L2E0KT/p1745613017872449 Would your preference be for us to wait and have that be code-generated (once it's added to your OpenAPI spec) or to just manually create the endpoint for now? |
examples/run_async.py
Outdated
|
|
||
| from replicate import AsyncReplicate | ||
|
|
||
| client = AsyncReplicate() |
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 we rename client to replicate here? Will that 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.
done!
(we can't change the from replicate import AsyncReplicate unfortunately though here, since the module client only returns the sync client)
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.
This is looking great. Left a few small suggestions but nothing major.
Happy to ship this without File upload support. It would probably be easier to review that in a separate PR anyway.
Sounds good! I've added file upload support on this branch: |
|
@zeke I think this is good to merge in now! You okay blessing it with your approval? |
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.
Whoop whoop! ![]()


Part of #11