Remove native.py and properly type check scheduler.py
#11706
Conversation
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
| class DaemonPantsRunner(RawFdRunner): | ||
| class DaemonPantsRunner: |
There was a problem hiding this comment.
RawFdRunner is a Protocol, which means you do not need to subclass it. MyPy only checks that the required interface is implemented.
| EMPTY_DIGEST = Digest(fingerprint=_EMPTY_FINGERPRINT, serialized_bytes_length=0) | ||
| EMPTY_DIGEST: Digest = Digest(fingerprint=_EMPTY_FINGERPRINT, serialized_bytes_length=0) |
There was a problem hiding this comment.
Not sure why MyPy wanted me to do this. It complained in a callsite that it couldn't figure out the type for EMPTY_DIGEST.
| def __init__(self, **kwargs: Any) -> None: ... | ||
| pass |
There was a problem hiding this comment.
This doesn't implement __new__ in interface.rs, so this type hint was a lie.
| @@ -155,19 +156,47 @@ def __init__( | |||
| interactive_process_result=InteractiveProcessResult, | |||
| engine_aware_parameter=EngineAwareParameter, | |||
| ) | |||
| remoting_options = PyRemotingOptions( | |||
There was a problem hiding this comment.
This is copy and pasted from native.py, no changes made.
| executor=executor, | ||
| execution_options=execution_options, | ||
| types=types, | ||
| self._py_scheduler = native_engine.scheduler_create( |
There was a problem hiding this comment.
Note the rename from ._scheduler to ._py_scheduler, which clears up one of my biggest confusions: what did Scheduler._scheduler mean?
| self._session = session | ||
| self._py_session = session |
There was a problem hiding this comment.
Note this change.
| def new_run_id(self): | ||
| """Assigns a new "run id" to this Session, without creating a new Session. | ||
|
|
||
| Usually each Session corresponds to one end user "run", but there are exceptions: notably, | ||
| the `--loop` feature uses one Session, but would like to observe new values for uncacheable | ||
| nodes in each iteration of its loop. | ||
| """ | ||
| self._scheduler._native.lib.session_new_run_id(self._session) | ||
|
|
||
| def set_per_session(self): |
There was a problem hiding this comment.
set_per_session() was unused and duplicated new_run_id(). Deleted.
| def node_count(self): | ||
| return self._scheduler.graph_len() |
There was a problem hiding this comment.
Unused. The only caller was internal to this class, and we can call self._scheduler.graph_len() directly.
| returns = tuple((root, state) for root, state in roots if type(state) is Return) | ||
| throws = tuple((root, state) for root, state in roots if type(state) is Throw) |
There was a problem hiding this comment.
MyPy didn't like that type check. Replaced with isinstance().
| logger.debug( | ||
| "computed %s nodes in %f seconds. there are %s total nodes.", | ||
| len(roots), | ||
| time.time() - start_time, | ||
| self._scheduler.graph_len(), | ||
| ) | ||
|
|
||
| returns = tuple((root, state) for root, state in roots if type(state) is Return) | ||
| throws = tuple((root, state) for root, state in roots if type(state) is Throw) | ||
| return cast(Tuple[Tuple[Return, ...], Tuple[Throw, ...]], (returns, throws)) |
There was a problem hiding this comment.
This return type was wrong. The method signature is now correct, and MyPy was able to figure it out :)
| class DaemonPantsRunner(RawFdRunner): | ||
| class DaemonPantsRunner: |
There was a problem hiding this comment.
This class is the only actual usage of the RawFdRunner type: would be good to add it back to signal that.
There was a problem hiding this comment.
Two reasons I don't think we should not do that:
- Now that the protocol is defined in
native_engine.pyi, we can't import it. I couldn't think of a good spot to define that protocol in a.pyfile. - It was an abuse of
Protocol. You should not subclassProtocol, and in fact, doing so was hiding a deviation from our expected API that the protocol saidstdin_fdinstead ofstdin_fileno.
| # NB: Watchman no longer triggers events when children are created/deleted under a | ||
| # directory, so we always need to invalidate the direct parent as well. |
There was a problem hiding this comment.
The reference to watchman is, but the logic we implement to invalidate the parent directory may or may not be. We avoided changing "what" we invalidate when we switched from watchman to the notify crate, but we probably have better precision now, and could invalidate fewer things without affecting correctness.
Opened #11707
# 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]
Now that we use CPython instead of CFFI, there is no need for
native.py- it was unnecessary boilerplate.This also fixes the type hints related to
native_engine.soandscheduler.py. Before, there was too much boilerplate to justify it, but now things are fully typed.[ci skip-build-wheels]