Refactor StreamingWorkunitHandler to be a class-based context manager
#11685
Conversation
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
| if not goals: | ||
| return PANTS_SUCCEEDED_EXIT_CODE |
There was a problem hiding this comment.
No need to have this in the SWH block, as there should be no workunits in this case.
There was a problem hiding this comment.
True, but some SWHs consume the RunTracker to report on the overall status of a run as well: should probably leave it in there.
| assert len(flattened) == 10 | ||
| assert len(flattened) == 11 |
There was a problem hiding this comment.
This is now 11 because we don't have the memoization from the original scheduler. We created a new one.
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
| if not goals: | ||
| return PANTS_SUCCEEDED_EXIT_CODE |
There was a problem hiding this comment.
True, but some SWHs consume the RunTracker to report on the overall status of a run as well: should probably leave it in there.
| @@ -180,54 +182,42 @@ def __init__( | |||
| options_bootstrapper: OptionsBootstrapper, | |||
| specs: Specs, | |||
| report_interval_seconds: float, | |||
| pantsd: bool, | |||
| max_workunit_verbosity: LogLevel = LogLevel.TRACE, | |||
| ) -> None: | |||
There was a problem hiding this comment.
So, it would still be possible to enter/exit this contextmanager multiple times, right? Using it in a with statement doesn't consume it.
One way to make that a non-issue though, maybe, would be to move the construction of the thread_runner into __enter__? That way you'd create a new one each time you called __enter__, and could fail if one already existed, etc.
There was a problem hiding this comment.
True, that's a good point that this doesn't actually prevent the problem from a language perspective.
However, my change does result in this error if you use the same handler twice:
15:00:25.87 [ERROR] threads can only be started once
Traceback (most recent call last):
File "/Users/eric/code/pants/src/python/pants/bin/daemon_pants_runner.py", line 134, in single_daemonized_run
return runner.run(start_time)
File "/Users/eric/code/pants/src/python/pants/bin/local_pants_runner.py", line 255, in run
with streaming_reporter:
File "/Users/eric/code/pants/src/python/pants/engine/streaming_workunit_handler.py", line 213, in __enter__
self.thread_runner.start()
File "/Users/eric/.pyenv/versions/3.9.1/lib/python3.9/threading.py", line 869, in start
raise RuntimeError("threads can only be started once")
RuntimeError: threads can only be started once
…ager # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
Before, it was possible to call
StreamingWorkunitHandler.session()multiple times, meaning we'd be starting and stopping the same threads during the same Pants run. While this wasn't done in practice beyond a test, it complicates future changes and is an inaccurate representation of what we expect.Even though it is still possible to do this with a class-based context manager, the class better expresses the intention than a reusable method. Further, this change will result in a RuntimeException if the user does use the context manager twice.
[ci skip-rust]
[ci skip-build-wheels]