omnibus actual concurrency and major refactor #1530
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
technillogue
force-pushed
the
async
branch
2 times, most recently
from
55c0468
to
cc246ac
Compare
technillogue
force-pushed
the
syl/more-refactor
branch
from
2fd12fe
to
f68a968
Compare
technillogue
force-pushed
the
async
branch
3 times, most recently
from
85d2814
to
f57474d
Compare
yorickvP
reviewed
Feb 13, 2024
technillogue
force-pushed
the
syl/more-refactor
branch
2 times, most recently
from
8f7a594
to
3563178
Compare
technillogue
force-pushed
the
syl/more-refactor
branch
2 times, most recently
from
9bc6ece
to
f390777
Compare
technillogue
force-pushed
the
async
branch
3 times, most recently
from
1e8c300
to
335f67b
Compare
technillogue
force-pushed
the
syl/more-refactor
branch
2 times, most recently
from
d965185
to
644d1cd
Compare
Signed-off-by: technillogue <technillogue@gmail.com>
Signed-off-by: technillogue <technillogue@gmail.com>
Signed-off-by: technillogue <technillogue@gmail.com>
Signed-off-by: technillogue <technillogue@gmail.com>
…busy state to runner Signed-off-by: technillogue <technillogue@gmail.com>
Signed-off-by: technillogue <technillogue@gmail.com>
Signed-off-by: technillogue <technillogue@gmail.com>
Signed-off-by: technillogue <technillogue@gmail.com>
Signed-off-by: technillogue <technillogue@gmail.com>
…point return the same result and fix tests somewhat Signed-off-by: technillogue <technillogue@gmail.com>
Signed-off-by: technillogue <technillogue@gmail.com>
There was a problem hiding this comment.
This PR is (as advertised, to be fair) a bit of a mishmash of changes with different intents and scopes, and as such it's a bit hard to review.
If I understand it correctly the gist of the effort here is to update the Worker API to support the management of multiple predictions, indexed by their IDs. That seems sensible, but I don't understand why we moved 80% of Worker to PredictionRunner to do that.
I'm also more than a little bit suspicious at the fact that the CI checks on this branch are all green, especially given that by far the most substantive part of the cog test suite (test_worker.py) is apparently not running at all.
technillogue
force-pushed
the
syl/more-refactor
branch
7 times, most recently
from
bd69678
to
7098fde
Compare
…ancelation and validation Signed-off-by: technillogue <technillogue@gmail.com>
Signed-off-by: technillogue <technillogue@gmail.com>
technillogue
force-pushed
the
syl/more-refactor
branch
from
7098fde
to
40bdb54
Compare
This was referenced May 17, 2024
Merged
technillogue
added a commit
that referenced
this pull request
Jun 19, 2024
* add concurrency to config * more descriptive names for predict functions * don't cancel from signal handler if a loop is running. expose worker busy state to runner * move handle_event_stream to PredictionEventHandler * make setup and canceling work * keep track of multiple runner prediction tasks to make idempotent endpoint return the same result and fix tests somewhat * drop Runner._result, comments * move create_event_handler into PredictionEventHandler.__init__ * break out Path.validate into value_to_path and inline get_filename and File.validate * split out URLPath into BackwardsCompatibleDataURLTempFilePath and URLThatCanBeConvertedToPath with the download part of URLFile inlined * let's make DataURLTempFilePath also use convert and move value_to_path back to Path.validate * drop should_cancel * prediction->request * split up predict/inner/prediction_ctx into enter_predict/exit_predict/prediction_ctx/inner_async_predict/predict/good_predict as one way to do it. however, exposing all of those for runner predict enter/coro exit still sucks, but this is still an improvement * bigish change: inline predict_and_handle_errors * inline make_error_handler into setup * move runner.setup into runner.Runner.setup * add concurrency to config in go * try explicitly using prediction_ctx __enter__ and __exit__ * relax setup argument requirement to str * glom worker into runner * add logging message * fix prediction retry and improve logging * split out handle_event * use CURL_CA_BUNDLE for file upload * dubious upload fix * skip worker and webhook tests since those were erroring on removed imports. fix or xfail runner tests * validate prediction response to raise errors, but return the unvalidated output to avoid converting urls to File/Path * expose concurrency in healthcheck * mediocre logging that works like print * COG_DISABLE_CANCEL to ignore cancelations * COG_CONCURRENCY_OVERRIDE * add ready probe as an http route * encode webhooks only after knowing they will be sent, and bail our of upload type checks early for strs * don't validate outputs * add AsyncConcatenateIterator * should_exit is not actually used by http * format * codecov * describe the remaining problems with this PR and add comments about cancelation and validation * add a test * fix test (#1669) * fix config schema * allow setting both max and target concurrency in cog.yaml (#1672) * drop default_target (#1685) --------- Signed-off-by: technillogue <technillogue@gmail.com> Co-authored-by: Mattt <mattt@replicate.com>
Closed
technillogue
added a commit
that referenced
this pull request
Jun 19, 2024
* add concurrency to config * more descriptive names for predict functions * don't cancel from signal handler if a loop is running. expose worker busy state to runner * move handle_event_stream to PredictionEventHandler * make setup and canceling work * keep track of multiple runner prediction tasks to make idempotent endpoint return the same result and fix tests somewhat * drop Runner._result, comments * move create_event_handler into PredictionEventHandler.__init__ * break out Path.validate into value_to_path and inline get_filename and File.validate * split out URLPath into BackwardsCompatibleDataURLTempFilePath and URLThatCanBeConvertedToPath with the download part of URLFile inlined * let's make DataURLTempFilePath also use convert and move value_to_path back to Path.validate * drop should_cancel * prediction->request * split up predict/inner/prediction_ctx into enter_predict/exit_predict/prediction_ctx/inner_async_predict/predict/good_predict as one way to do it. however, exposing all of those for runner predict enter/coro exit still sucks, but this is still an improvement * bigish change: inline predict_and_handle_errors * inline make_error_handler into setup * move runner.setup into runner.Runner.setup * add concurrency to config in go * try explicitly using prediction_ctx __enter__ and __exit__ * relax setup argument requirement to str * glom worker into runner * add logging message * fix prediction retry and improve logging * split out handle_event * use CURL_CA_BUNDLE for file upload * dubious upload fix * skip worker and webhook tests since those were erroring on removed imports. fix or xfail runner tests * validate prediction response to raise errors, but return the unvalidated output to avoid converting urls to File/Path * expose concurrency in healthcheck * mediocre logging that works like print * COG_DISABLE_CANCEL to ignore cancelations * COG_CONCURRENCY_OVERRIDE * add ready probe as an http route * encode webhooks only after knowing they will be sent, and bail our of upload type checks early for strs * don't validate outputs * add AsyncConcatenateIterator * should_exit is not actually used by http * format * codecov * describe the remaining problems with this PR and add comments about cancelation and validation * add a test * fix test (#1669) * fix config schema * allow setting both max and target concurrency in cog.yaml (#1672) * drop default_target (#1685) --------- Signed-off-by: technillogue <technillogue@gmail.com> Co-authored-by: Mattt <mattt@replicate.com> Signed-off-by: technillogue <technillogue@gmail.com>
technillogue
added a commit
that referenced
this pull request
Jun 19, 2024
* add concurrency to config * more descriptive names for predict functions * don't cancel from signal handler if a loop is running. expose worker busy state to runner * move handle_event_stream to PredictionEventHandler * make setup and canceling work * keep track of multiple runner prediction tasks to make idempotent endpoint return the same result and fix tests somewhat * drop Runner._result, comments * move create_event_handler into PredictionEventHandler.__init__ * break out Path.validate into value_to_path and inline get_filename and File.validate * split out URLPath into BackwardsCompatibleDataURLTempFilePath and URLThatCanBeConvertedToPath with the download part of URLFile inlined * let's make DataURLTempFilePath also use convert and move value_to_path back to Path.validate * drop should_cancel * prediction->request * split up predict/inner/prediction_ctx into enter_predict/exit_predict/prediction_ctx/inner_async_predict/predict/good_predict as one way to do it. however, exposing all of those for runner predict enter/coro exit still sucks, but this is still an improvement * bigish change: inline predict_and_handle_errors * inline make_error_handler into setup * move runner.setup into runner.Runner.setup * add concurrency to config in go * try explicitly using prediction_ctx __enter__ and __exit__ * relax setup argument requirement to str * glom worker into runner * add logging message * fix prediction retry and improve logging * split out handle_event * use CURL_CA_BUNDLE for file upload * dubious upload fix * skip worker and webhook tests since those were erroring on removed imports. fix or xfail runner tests * validate prediction response to raise errors, but return the unvalidated output to avoid converting urls to File/Path * expose concurrency in healthcheck * mediocre logging that works like print * COG_DISABLE_CANCEL to ignore cancelations * COG_CONCURRENCY_OVERRIDE * add ready probe as an http route * encode webhooks only after knowing they will be sent, and bail our of upload type checks early for strs * don't validate outputs * add AsyncConcatenateIterator * should_exit is not actually used by http * format * codecov * describe the remaining problems with this PR and add comments about cancelation and validation * add a test * fix test (#1669) * fix config schema * allow setting both max and target concurrency in cog.yaml (#1672) * drop default_target (#1685) --------- Signed-off-by: technillogue <technillogue@gmail.com> Co-authored-by: Mattt <mattt@replicate.com> Signed-off-by: technillogue <technillogue@gmail.com>
technillogue
added a commit
that referenced
this pull request
Jun 19, 2024
* add concurrency to config * more descriptive names for predict functions * don't cancel from signal handler if a loop is running. expose worker busy state to runner * move handle_event_stream to PredictionEventHandler * make setup and canceling work * keep track of multiple runner prediction tasks to make idempotent endpoint return the same result and fix tests somewhat * drop Runner._result, comments * move create_event_handler into PredictionEventHandler.__init__ * break out Path.validate into value_to_path and inline get_filename and File.validate * split out URLPath into BackwardsCompatibleDataURLTempFilePath and URLThatCanBeConvertedToPath with the download part of URLFile inlined * let's make DataURLTempFilePath also use convert and move value_to_path back to Path.validate * drop should_cancel * prediction->request * split up predict/inner/prediction_ctx into enter_predict/exit_predict/prediction_ctx/inner_async_predict/predict/good_predict as one way to do it. however, exposing all of those for runner predict enter/coro exit still sucks, but this is still an improvement * bigish change: inline predict_and_handle_errors * inline make_error_handler into setup * move runner.setup into runner.Runner.setup * add concurrency to config in go * try explicitly using prediction_ctx __enter__ and __exit__ * relax setup argument requirement to str * glom worker into runner * add logging message * fix prediction retry and improve logging * split out handle_event * use CURL_CA_BUNDLE for file upload * dubious upload fix * skip worker and webhook tests since those were erroring on removed imports. fix or xfail runner tests * validate prediction response to raise errors, but return the unvalidated output to avoid converting urls to File/Path * expose concurrency in healthcheck * mediocre logging that works like print * COG_DISABLE_CANCEL to ignore cancelations * COG_CONCURRENCY_OVERRIDE * add ready probe as an http route * encode webhooks only after knowing they will be sent, and bail our of upload type checks early for strs * don't validate outputs * add AsyncConcatenateIterator * should_exit is not actually used by http * format * codecov * describe the remaining problems with this PR and add comments about cancelation and validation * add a test * fix test (#1669) * fix config schema * allow setting both max and target concurrency in cog.yaml (#1672) * drop default_target (#1685) --------- Signed-off-by: technillogue <technillogue@gmail.com> Co-authored-by: Mattt <mattt@replicate.com> Signed-off-by: technillogue <technillogue@gmail.com>
technillogue
added a commit
that referenced
this pull request
Jul 3, 2024
* add concurrency to config * more descriptive names for predict functions * don't cancel from signal handler if a loop is running. expose worker busy state to runner * move handle_event_stream to PredictionEventHandler * make setup and canceling work * keep track of multiple runner prediction tasks to make idempotent endpoint return the same result and fix tests somewhat * drop Runner._result, comments * move create_event_handler into PredictionEventHandler.__init__ * break out Path.validate into value_to_path and inline get_filename and File.validate * split out URLPath into BackwardsCompatibleDataURLTempFilePath and URLThatCanBeConvertedToPath with the download part of URLFile inlined * let's make DataURLTempFilePath also use convert and move value_to_path back to Path.validate * prediction->request * split up predict/inner/prediction_ctx into enter_predict/exit_predict/prediction_ctx/inner_async_predict/predict/good_predict as one way to do it. however, exposing all of those for runner predict enter/coro exit still sucks, but this is still an improvement * bigish change: inline predict_and_handle_errors * inline make_error_handler into setup * move runner.setup into runner.Runner.setup * add concurrency to config in go * try explicitly using prediction_ctx __enter__ and __exit__ * relax setup argument requirement to str * glom worker into runner * add logging message * fix prediction retry and improve logging * split out handle_event * use CURL_CA_BUNDLE for file upload * dubious upload fix * skip worker and webhook tests since those were erroring on removed imports. fix or xfail runner tests * validate prediction response to raise errors, but return the unvalidated output to avoid converting urls to File/Path * expose concurrency in healthcheck * mediocre logging that works like print * COG_DISABLE_CANCEL to ignore cancelations * COG_CONCURRENCY_OVERRIDE * add ready probe as an http route * encode webhooks only after knowing they will be sent, and bail our of upload type checks early for strs * don't validate outputs * add AsyncConcatenateIterator * should_exit is not actually used by http * format * codecov * describe the remaining problems with this PR and add comments about cancelation and validation * add a test * fix test (#1669) * fix config schema * allow setting both max and target concurrency in cog.yaml (#1672) * drop default_target (#1685) --------- Signed-off-by: technillogue <technillogue@gmail.com> Co-authored-by: Mattt <mattt@replicate.com>
technillogue
added a commit
that referenced
this pull request
Jul 18, 2024
* add concurrency to config * more descriptive names for predict functions * don't cancel from signal handler if a loop is running. expose worker busy state to runner * move handle_event_stream to PredictionEventHandler * make setup and canceling work * keep track of multiple runner prediction tasks to make idempotent endpoint return the same result and fix tests somewhat * drop Runner._result, comments * move create_event_handler into PredictionEventHandler.__init__ * break out Path.validate into value_to_path and inline get_filename and File.validate * split out URLPath into BackwardsCompatibleDataURLTempFilePath and URLThatCanBeConvertedToPath with the download part of URLFile inlined * let's make DataURLTempFilePath also use convert and move value_to_path back to Path.validate * prediction->request * split up predict/inner/prediction_ctx into enter_predict/exit_predict/prediction_ctx/inner_async_predict/predict/good_predict as one way to do it. however, exposing all of those for runner predict enter/coro exit still sucks, but this is still an improvement * bigish change: inline predict_and_handle_errors * inline make_error_handler into setup * move runner.setup into runner.Runner.setup * add concurrency to config in go * try explicitly using prediction_ctx __enter__ and __exit__ * relax setup argument requirement to str * glom worker into runner * add logging message * fix prediction retry and improve logging * split out handle_event * use CURL_CA_BUNDLE for file upload * dubious upload fix * skip worker and webhook tests since those were erroring on removed imports. fix or xfail runner tests * validate prediction response to raise errors, but return the unvalidated output to avoid converting urls to File/Path * expose concurrency in healthcheck * mediocre logging that works like print * COG_DISABLE_CANCEL to ignore cancelations * COG_CONCURRENCY_OVERRIDE * add ready probe as an http route * encode webhooks only after knowing they will be sent, and bail our of upload type checks early for strs * don't validate outputs * add AsyncConcatenateIterator * should_exit is not actually used by http * format * codecov * describe the remaining problems with this PR and add comments about cancelation and validation * add a test * fix test (#1669) * fix config schema * allow setting both max and target concurrency in cog.yaml (#1672) * drop default_target (#1685) --------- Signed-off-by: technillogue <technillogue@gmail.com> Co-authored-by: Mattt <mattt@replicate.com>
mattt
pushed a commit
that referenced
this pull request
Jul 18, 2024
* add concurrency to config * more descriptive names for predict functions * don't cancel from signal handler if a loop is running. expose worker busy state to runner * move handle_event_stream to PredictionEventHandler * make setup and canceling work * keep track of multiple runner prediction tasks to make idempotent endpoint return the same result and fix tests somewhat * drop Runner._result, comments * move create_event_handler into PredictionEventHandler.__init__ * break out Path.validate into value_to_path and inline get_filename and File.validate * split out URLPath into BackwardsCompatibleDataURLTempFilePath and URLThatCanBeConvertedToPath with the download part of URLFile inlined * let's make DataURLTempFilePath also use convert and move value_to_path back to Path.validate * prediction->request * split up predict/inner/prediction_ctx into enter_predict/exit_predict/prediction_ctx/inner_async_predict/predict/good_predict as one way to do it. however, exposing all of those for runner predict enter/coro exit still sucks, but this is still an improvement * bigish change: inline predict_and_handle_errors * inline make_error_handler into setup * move runner.setup into runner.Runner.setup * add concurrency to config in go * try explicitly using prediction_ctx __enter__ and __exit__ * relax setup argument requirement to str * glom worker into runner * add logging message * fix prediction retry and improve logging * split out handle_event * use CURL_CA_BUNDLE for file upload * dubious upload fix * skip worker and webhook tests since those were erroring on removed imports. fix or xfail runner tests * validate prediction response to raise errors, but return the unvalidated output to avoid converting urls to File/Path * expose concurrency in healthcheck * mediocre logging that works like print * COG_DISABLE_CANCEL to ignore cancelations * COG_CONCURRENCY_OVERRIDE * add ready probe as an http route * encode webhooks only after knowing they will be sent, and bail our of upload type checks early for strs * don't validate outputs * add AsyncConcatenateIterator * should_exit is not actually used by http * format * codecov * describe the remaining problems with this PR and add comments about cancelation and validation * add a test * fix test (#1669) * fix config schema * allow setting both max and target concurrency in cog.yaml (#1672) * drop default_target (#1685) --------- Signed-off-by: technillogue <technillogue@gmail.com> Co-authored-by: Mattt <mattt@replicate.com>
mattt
pushed a commit
that referenced
this pull request
Jul 19, 2024
* add concurrency to config * more descriptive names for predict functions * don't cancel from signal handler if a loop is running. expose worker busy state to runner * move handle_event_stream to PredictionEventHandler * make setup and canceling work * keep track of multiple runner prediction tasks to make idempotent endpoint return the same result and fix tests somewhat * drop Runner._result, comments * move create_event_handler into PredictionEventHandler.__init__ * break out Path.validate into value_to_path and inline get_filename and File.validate * split out URLPath into BackwardsCompatibleDataURLTempFilePath and URLThatCanBeConvertedToPath with the download part of URLFile inlined * let's make DataURLTempFilePath also use convert and move value_to_path back to Path.validate * prediction->request * split up predict/inner/prediction_ctx into enter_predict/exit_predict/prediction_ctx/inner_async_predict/predict/good_predict as one way to do it. however, exposing all of those for runner predict enter/coro exit still sucks, but this is still an improvement * bigish change: inline predict_and_handle_errors * inline make_error_handler into setup * move runner.setup into runner.Runner.setup * add concurrency to config in go * try explicitly using prediction_ctx __enter__ and __exit__ * relax setup argument requirement to str * glom worker into runner * add logging message * fix prediction retry and improve logging * split out handle_event * use CURL_CA_BUNDLE for file upload * dubious upload fix * skip worker and webhook tests since those were erroring on removed imports. fix or xfail runner tests * validate prediction response to raise errors, but return the unvalidated output to avoid converting urls to File/Path * expose concurrency in healthcheck * mediocre logging that works like print * COG_DISABLE_CANCEL to ignore cancelations * COG_CONCURRENCY_OVERRIDE * add ready probe as an http route * encode webhooks only after knowing they will be sent, and bail our of upload type checks early for strs * don't validate outputs * add AsyncConcatenateIterator * should_exit is not actually used by http * format * codecov * describe the remaining problems with this PR and add comments about cancelation and validation * add a test * fix test (#1669) * fix config schema * allow setting both max and target concurrency in cog.yaml (#1672) * drop default_target (#1685) --------- Signed-off-by: technillogue <technillogue@gmail.com> Co-authored-by: Mattt <mattt@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.
this should be reviewable at last. I'm mostly interested in whether the changes are comprehensible/legible and what comments I can add. there's a grab-bag of random changes like a /ready route, predictor.log, cancellation fixes, etc and the core change of moving worker into runner. I'm very open to reconsidering this (e.g. call it worker instead and maybe move some of it into http), but not right now. once this is merged, the plan is to gradually cut small changes from the async branch to merge into main, and review those changes more thoroughly.
original description
I originally tried to split up my work in #1499 and #1508 as "refactor runner + add concurrency" and "fix uploads/downloads" but ended up interleaving these changes. this PR will just be the overall changeset for now, and hopefully as this coalesces more it'll be clear how to carve it up into separate changesetsmajor points:
concurrencyto cog.yaml