Bug fix for runner timeout handling - #7390
Merged
yarikoptic merged 6 commits intoMay 21, 2023
Merged
Conversation
This commit ensures that output generated by timeouts is accessible to clients. It fixes an error, where consecutive timeouts without any other event in between where not available to clients that used a generator-protocol.
This commit modifies BatchedCommand to emit debug messages, if timeouts are encountered. Note: in order to match the old implementation, BatchedCommand uses a default timeout of 11 seconds. This holds, even when timeout=None was specified on BatchedCommand construction.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## maint #7390 +/- ##
==========================================
+ Coverage 88.18% 90.70% +2.52%
==========================================
Files 327 327
Lines 44686 44693 +7
Branches 0 5924 +5924
==========================================
+ Hits 39405 40541 +1136
+ Misses 5281 4136 -1145
- Partials 0 16 +16
☔ View full report in Codecov by Sentry. |
yarikoptic
requested changes
May 19, 2023
Member
There was a problem hiding this comment.
minor docs etc notes. Logic seems to be LGTM although I am not familiar much with this code
edit: typing issues observed:
datalad/runner/nonasyncrunner.py:561: error: Unsupported operand types for >= ("float" and "None") [operator]
datalad/runner/nonasyncrunner.py:561: note: Right operand is of type "Optional[float]"
Found 1 error in 1 file (checked 32 source files)
This commit adds a check for None. This is required to pass mypy checks. The variable in question is already checked for None, but mypy does not recognize this. TODO: there is probably a better way to inform mypy about valid assertions.
christian-monch
force-pushed
the
bf-runner-timeout-handling
branch
from
May 19, 2023 21:06
6a8d14f to
9122da0
Compare
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
Contributor
Author
I fixed the error by adding a check for |
Member
|
Thank you @christian-monch |
Collaborator
|
PR released in |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 PR ensures that output generated by timeout-handlers is accessible to clients that iterate over a result-generator. The fixes
an error, where consecutive timeouts without any other event in between them where not delivered by the result-generator.
This bug was detected during work on issue #7315, but is independent.