Vine: Improve how errors are propagated with FuturesExecutor
#3922
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
In Python 3.9 and older, concurrent.futures used its own TimeoutError class rather than the builtin. TimeoutError is aliased to the builtin in layer Python versions, so for backwards compatibility we import it in the test file. See python/cpython#30197
|
@BarrySlyDelgado what do you think? |
|
Thanks! What should be done about the missing |
|
#3929 should be fix |
|
Great! FWIW none of my applications use task cancelling so that won't block me (many other executors have limited support for task cancelling). |
BarrySlyDelgado
approved these changes
Sep 10, 2024
dthain
approved these changes
Sep 16, 2024
colinthomas-z80
pushed a commit
to colinthomas-z80/cctools
that referenced
this pull request
Oct 15, 2024
…rative-computing-lab#3922) * raise task exceptions in python task futures * wrap FutureFunctionCall tracebacks in FunctionCallNoResult * Raise TimeoutError in Future when task result is pending * yield futures and raise TimeoutError in as_completed * Improve error handling in wait * Import TimeoutError for Python <3.10 compatibility In Python 3.9 and older, concurrent.futures used its own TimeoutError class rather than the builtin. TimeoutError is aliased to the builtin in layer Python versions, so for backwards compatibility we import it in the test file. See python/cpython#30197
btovar
pushed a commit
that referenced
this pull request
Oct 21, 2024
* raise task exceptions in python task futures * wrap FutureFunctionCall tracebacks in FunctionCallNoResult * Raise TimeoutError in Future when task result is pending * yield futures and raise TimeoutError in as_completed * Improve error handling in wait * Import TimeoutError for Python <3.10 compatibility In Python 3.9 and older, concurrent.futures used its own TimeoutError class rather than the builtin. TimeoutError is aliased to the builtin in layer Python versions, so for backwards compatibility we import it in the test file. See python/cpython#30197
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.
Proposed Changes
Improves how task errors and timeouts are propagated in
Future,wait(), andas_completed()when using theFuturesExecutor(see #3920). The commits are in chronological order, each addressing a single class/function, if you prefer to review it that way.A couple notes:
TimeoutErrorsare raised inas_completed, I changed it to return a generator which will reduce latency on the consumer side compared to waiting to construct an iterator from a fully constructed list of futures.FutureFunctionCall.output()/FuturePythonTask.output()were inconsistent in when they raised vs returned task exceptions. I changed those methods to always return task exceptions, and then left the responsibility of raising exceptions to theFuture.result().FutureFunctionCall.output()seems to only have access to the string representation of the exception (not the pickled exception like inFuturePythonTask). I construct aFunctionCallNoResultto return containing the string representation of the task's exception. This has the downside of not letting the user catch specific types of task exceptions (e.g., exceptions they might expect to occur), but they will at least see the true exception message and not just an emptyFunctionCallNoResult.Warning: This PR only fixes four of the five items in #3920. I didn't immediately see how to fix
Future.done(). It callsManager.task_state()which then callscvine.vine_task_statebut this function was removed in c1f1cd7. CouldFuture.done()be implemented by callingFuture.result(timeout=0)? It's probably not very efficient to do it that way---maybe there's an alternative to the removecvine.vine_task_state?Please feel free to push to my branch if you want to alter anything yourself.
Merge Checklist
The following items must be completed before PRs can be merge.
Check these off to verify you have completed all steps.
make testRun local tests prior to pushing.make formatFormat source code to comply with lint policies. Note that some lint errors can only be resolved manually (e.g., Python)make lintRun lint on source code prior to pushing.