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 timings #75

Merged
merged 1 commit into from
Mar 3, 2023
Merged

Add timings #75

merged 1 commit into from
Mar 3, 2023

Conversation

daanelson
Copy link
Contributor

Wanted to add the ability in the python API to return the timing information (created_at, started_at, completed_at) so that we can pull all that info right in python without hopping back and forth between curl/etc.

Didn't want to break the existing predict() -> prediction.output contract, which led me to implementing a create_prediction method that returns an async prediction object (similar, I think, to the current JS API).

Copy link
Member

@erbridge erbridge left a comment

Choose a reason for hiding this comment

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

Nice. I like this as an approach.

@bfirsh
Copy link
Member

bfirsh commented Mar 2, 2023

We should definitely get the timing info in, but I'm not sure about this new method, so maybe we shouldn't conflate the two changes.

Is this new method just for convenience, or is there a technical reason why we need it?

This is creating a new verb_noun() pattern that I don't think we use elsewhere. I don't like the idea of us having multiple naming patterns unless we really need it. I wonder if we can follow the existing nouns.verb() pattern by creating a PredictionCollection on versions? E.g.

version = replicate.versions.get("...")
version.predictions.create(...)

That would also imply version.predictions.list(), version.predictions.get(), etc.

@daanelson
Copy link
Contributor Author

@bfirsh we don't explicitly need it; I suppose I appreciate the simplicity of having one noun from which to get predictions. currently if a user wants to run a prediction method which returns the output of said prediction, they call version.predict(); but if they want to run a prediction method that returns a Prediction they call replicate.predictions.create(version=<some_version>, ...). Would be nice to have it all in one place, but not necessary.

Also a spot where the JS & python clients diverge at the moment, model.predict in JS returns a prediction.

All that said, happy to just push the timings and leave the API as is if it doesn't feel right.

@bfirsh
Copy link
Member

bfirsh commented Mar 2, 2023

Let's maybe pull that out into a separate PR for discussion. I don't want to block getting the uncontroversial change in.

I think we should think carefully about adding a new naming pattern for an API, particularly as we're in the process of trying to align the Python and JS APIs.

Signed-off-by: dan nelson <dan.nelson8@gmail.com>
@daanelson
Copy link
Contributor Author

@bfirsh works for me. This is now just timing info.

Copy link
Member

@zeke zeke left a comment

Choose a reason for hiding this comment

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

Nice

@daanelson daanelson merged commit ba9a4eb into main Mar 3, 2023
@daanelson daanelson deleted the add_timings branch March 3, 2023 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants