Support async predictors (✨ updated ✨) #2010
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
First steps towards allowing `async def predict` method signatures. This commit adds support to Worker for starting an `asyncio` event loop if the `predict` function returns an awaitable or an async generator. For now, we drop support for output capture as well as cancelation.
In an async context, attempting to intercept stream writes at the file descriptor layer is futile. We can do it, but we will have no way of associating a write made from native code with a specific prediction -- and the only reason to intercept/swap out the STDOUT/STDERR file descriptors is so that we can catch writes from native code. This commit adds an altogether simpler implementation which can work for async code with that restriction. All it does is patch `sys.stdout` and `sys.stderr` with objects that can redirect (or tee) the output to a callback function.
This implements basic cancelation for async predictors. Whereas regular predictors implement cancelation using a custom CancelationException, asyncio already has a concept of task cancelation, so we use that. When cancelation is requested, we send a `Cancel()` event down the events pipe to the child. Regular predictors ignore these, but async predictors cancel the currently-running task when they receive one. In future, these `Cancel()` events will specify which running prediction they are intended to cancel.
When a `Shutdown()` event is sent, any running prediction should be allowed to completed. For now, we implement this by awaiting any task that is tracked when we break out of the child worker's event loop.
meatballhat
changed the title
meatballhat
changed the title
evilstreak
force-pushed
the
support-async-predictors-merged
branch
from
88568d9 to
eedac64
Compare
- Use renamed _ChildWorker type - Set initial `__url__` to `None` prior to URL parsing potentially throwing `ValueError` - Declare `pid` field in `FakeChildWorker` - Do not use nested redirectors
evilstreak
force-pushed
the
support-async-predictors-merged
branch
from
8c73c7a to
2830d4d
Compare
meatballhat
added a commit
that referenced
this pull request
Oct 24, 2024
This reverts commit a86adcd.
meatballhat
added a commit
that referenced
this pull request
Oct 25, 2024
meatballhat
added a commit
that referenced
this pull request
Oct 25, 2024
meatballhat
added a commit
that referenced
this pull request
Oct 28, 2024
meatballhat
added a commit
that referenced
this pull request
Oct 28, 2024
meatballhat
added a commit
that referenced
this pull request
Oct 29, 2024
* Revert "Revert "Support async predictors (#2010)" (#2022)" This reverts commit 8333a83. * Use anync stream redirector in setup so that the sync stream redirector context is only entered once, as this is a known source of problems associated with stdout/stderr orphaning. * Do not assert that writes from C are captured during setup * Do not wrap empty data in Log events * Exclude invalid output paths from infrastructure errors (#2030) * Exclude invalid output paths from infrastructure errors Closes PLAT-380 * Fix words given pluralization Co-authored-by: F <f@replicate.com> Signed-off-by: Dan Buch <dan@meatballhat.com> --------- Signed-off-by: Dan Buch <dan@meatballhat.com> Co-authored-by: F <f@replicate.com> --------- Signed-off-by: Dan Buch <dan@meatballhat.com> Co-authored-by: F <f@replicate.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
which is #1896 with
mainmerged in with conflicts resolved plus fixes to get tests happy. I don't want to replace #1896 outright since it provides a much more easily digestible group of commits. If this PR is deemed acceptable and merged, then I think we can safely delete #1896.Connected to PLAT-259