Enh/error on timeout #683
Enh/error on timeout #683
Conversation
This avoids situations where the benchmark continues to run, thinking that the process completed successfully.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #683 +/- ##
==========================================
+ Coverage 68.15% 68.70% +0.54%
==========================================
Files 54 55 +1
Lines 6730 6749 +19
==========================================
+ Hits 4587 4637 +50
+ Misses 2143 2112 -31 ☔ View full report in Codecov by Sentry. |
| retcode = process.poll() | ||
| if retcode is None: |
There was a problem hiding this comment.
As far as I can tell, this is a reliable way to tell the process is still running. And if the process is still running at this point I think the only reason can be that the communicate function returned early, which should only happen with an activity timeout.
| # if a pipe is not ready it could be timeout or it could be end of process | ||
| # so at this point we do not know. Only after the communicate function is over do we know. | ||
| # i.e., if the process is still running it does not have a retcode. |
There was a problem hiding this comment.
Preferably I would have raised the activity timeout from the function which uses it. But at this stage we can unfortunately not detect whether the error should be raised.
| res = bench.run(args.task, args.fold) | ||
| try: | ||
| bench.setup(amlb.SetupMode[args.setup]) | ||
| except StaleProcessError as e: |
There was a problem hiding this comment.
I'll move to exception notes instead and/or revise this structure. The reason I did it this way is to communicate more clearly to the user with a final message what went wrong and how to solve it. I want to generally make errors easier to parse, as there are some issues opened that are completely solvable from the traceback, but users can't/don't try to parse those.
Closes #681
If a process "completes" due to exceeding the activity timeout, it should raise an error, and not just continue.