[TIDY FIRST] Type BaseRunner class
#10607
Open
+94
−46
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
The `BaseRunner` class is a widely inherited class, and it's untyped. Sometimes the wrong thing gets passed into the classes that inherit `BaseRunner`. The "subclasses" of `BaseRunner` generally further restrict some of the available typing, (f.x. the type of node that can be passed in). In order to properly type those subclasses, we first should type `BaseRunner`. This is the start of that.
As part of this I had to switch the typing of node from `GraphMemberNode` to `ManifestNode`. We needed to do this be because `GraphMemberNode` was _too_ wide. Specifically, `GraphMemberNode` includes the `Metric` node which doesn't inherit from `NodeInfoMixin` and thus does not have a `update_event_status` method. Unfortunately moving to `ManifestNode` _excludes_ some nodes from the allowed typing that we'll probably later on find we need to include. Maybe that means some nodes should be moved to the `ManifestNode` definition. We might find that it would be more appropriate however to add an `ExecutableNode` or `RunnableNode` protocol definition, and then use that for typing.
QMalcolm
force-pushed
the
qmalcolm--typing-base-runner
branch
from
11abfa8
to
fb6c1ba
Compare
QMalcolm
added
the
artifact_minor_upgrade
…_result` of `BaseRunner` There are two type errors in this commit. The type hinting of `RunResult.node` and the `node` argument of `_build_run_result` are _different_! I tried to fix this in a previous commit by going from `GraphMemberNode` too `ManifestNode` as `GraphMemberNode` was too broad. It seems `ManifestNode` is too narrow unfortunately. Checking the typing of `RunResult.node`, which I should have done sooner, it uses `ResultNode`, which makes sense. The second type error is that the type hinting of `RunResult.status` and the `status` arguement of `_build_run_result` are _different_. The correct typing we should be that of `RunResult.status`. Both of these typing issues will be resolved in the next commit.
… in `BaseRunner`
We did this because in `BaseRunner.compile_and_execute` we weren't getting type completion for accessing `ctx.node.node_info`. Said another way, we didn't have the type context in `compile_and_execute` to know if a `node_info` property existed on the `node` of the `ctx`. By improving type hinting for `ExecutionContext` we were able to resolve this.
Because `execute` didn't have it's return type specified, it appeared to `run` that there was no expected return value. In turn `compile_and_run` was saying that the `result` it would return would _always_ be `None`. However that is most assuredly _not_ the case, and typing `execute` solves this.
Specifically methods: - `_handle_catchable_exception` - `_handle_internal_exception` - `_handle_generic_exception` - `handle_exception` Additionally we are defining "catchable errors" twice now. Once as a union of types in `_handle_catchable_exception` and as a tuple of classes in `handle_exception`. We should look at refactoring this into a single definition sometime, as a change to one should 1-1 correspond to a change in the other.
These two methods are essentially just additional logging based for the runner. They should never return anything. Typing them as such helps guarantee that so people don't end up doing some weird things.
In adding type hinting to `on_skip`, and `do_skip`, a potential runtime error was uncovered wherein we were accessing `self.skip_cause.status` in `on_skip` when the `skip_cause` can be `None`. On that edge case, a runtime error would be raised given how we were attempting to access the `status` property. We now handle this edge case, and we only discovered it because of the additional typing.
QMalcolm
force-pushed
the
qmalcolm--typing-base-runner
branch
from
fb6c1ba
to
3b2481b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10607 +/- ##
==========================================
- Coverage 88.91% 88.86% -0.05%
==========================================
Files 180 180
Lines 22755 22767 +12
==========================================
+ Hits 20232 20233 +1
- Misses 2523 2534 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
|
emmyoop
reviewed
Aug 28, 2024
Co-authored-by: Emily Rockman <[email protected]>
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.
Resolves #10606
Problem
The BaseRunner class is largely untyped. Given the prevalence of this class, not having it typed could mean that there are potential edge cases and/or runtime exceptions we are not aware of. By typing this class, we give ourselves a lot more guarantees
Solution
Add typing. Of note, adding this typing uncovered a potential runtime exception in the
on_skipmethod. We address this in 5adce38.Checklist