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

Fix iterator support for replicate.run() #383

Merged
merged 6 commits into from
Oct 28, 2024
Merged

Fix iterator support for replicate.run() #383

merged 6 commits into from
Oct 28, 2024

Conversation

aron
Copy link
Contributor

@aron aron commented Oct 24, 2024

Prior to 1.0.0 replicate.run() would return an iterator for cog models that output a type of Iterator[Any]. This would poll the predictions.get endpoint for the in progress prediction and yield any new output.

When implementing the new file interface we introduced two bugs:

  1. The iterator didn't convert URLs returned by the model into FileOutput types making it inconsistent with the non-iterator interface. This is controlled by the use_file_outputs argument.
  2. The iterator was returned without checking if we are using the new blocking API introduced by default and controlled by the wait argument.

This commit fixes these two issues, consistently applying the transform_output function to the output of the iterator as well as returning the polling iterator (prediciton.output_iterator) if the blocking API has not successfully returned a completed prediction.

The tests have been updated to exercise both of these code paths.

@aron aron requested a review from nickstenning October 24, 2024 11:49
@aron aron force-pushed the fix-run-iterable branch 2 times, most recently from 69e3773 to eb4c0df Compare October 24, 2024 14:55
}


def _version_with_schema(id: str = "v1", output_schema: Optional[object] = None):
Copy link
Member

Choose a reason for hiding this comment

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

Nice cleanup here. 👍🏼

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.

Tentative approval, with the caveat that I am not a Python programmer. 👍🏼

Love seeing all the tests. I tried following the logic in the tests, but there's so much indirection that it's Greek to me.

Prior to 1.0.0 `replicate.run()` would return an iterator for cog models
that output a type of `Iterator[Any]`. This would poll the
`predictions.get` endpoint for the in progress prediction and
yield any new output.

When implementing the new file interface we introduced two bugs:

1. The iterator didn't convert URLs returned by the model into
   `FileOutput` types making it inconsistent with the non-iterator
   interface. This is controlled by the `use_file_outputs` argument.
2. The iterator was returned without checking if we are using the new
   blocking API introduced by default and controlled by the `wait`
   argument.

This commit fixes these two issues, consistently applying the
`transform_output` function to the output of the iterator as well
as returning the polling iterator (`prediciton.output_iterator`) if
the blocking API has not successfully returned a completed prediction.

The tests have been updated to exercise both of these code paths.
@aron aron force-pushed the fix-run-iterable branch from eb4c0df to 6c427f7 Compare October 25, 2024 09:07
replicate/run.py Outdated Show resolved Hide resolved
replicate/run.py Outdated Show resolved Hide resolved
replicate/run.py Outdated Show resolved Hide resolved
replicate/run.py Outdated Show resolved Hide resolved
Copy link
Member

@nickstenning nickstenning left a comment

Choose a reason for hiding this comment

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

I've tested this locally and it fixes #381. Let's get it out and released.

@aron aron merged commit de717a0 into main Oct 28, 2024
7 checks passed
@aron aron deleted the fix-run-iterable branch October 28, 2024 10:50
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.

3 participants