Annotate WorkerState for Cythonization
#4294
Merged
Conversation
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
jakirkham
changed the title
WorkerState for CythonizationWorkerState for Cythonization
jakirkham
force-pushed
the
cy_ws
branch
4 times, most recently
from
853da21 to
ceaae08
Compare
jakirkham
commented
Dec 2, 2020
jakirkham
force-pushed
the
cy_ws
branch
2 times, most recently
from
ef11ffd to
b0764f5
Compare
jakirkham
commented
Dec 2, 2020
jakirkham
commented
Dec 2, 2020
jakirkham
force-pushed
the
cy_ws
branch
2 times, most recently
from
d4c8a9c to
adba1ba
Compare
jakirkham
changed the title
WorkerState for CythonizationWorkerState for Cythonization
Member
Author
jakirkham
commented
Dec 3, 2020
jakirkham
commented
Dec 3, 2020
jakirkham
force-pushed
the
cy_ws
branch
5 times, most recently
from
0b338b3 to
91f636c
Compare
jakirkham
force-pushed
the
cy_ws
branch
3 times, most recently
from
efa24e1 to
a069e21
Compare
Typically `bandwidth` seems to be a `float` elsewhere in the code. So coerce it to a `float` here as well to align with that practice.
Avoids confusion with the variable name used outside this closure. Also bypasses any potential leakage from the external scope.
Since these are effectively private given they are only available in Cython/C, go ahead and mark them as such. We can then add Python `@property`s on top of them to handle Python access to the original attribute names with minimal disruption.
Includes `setter`s for `occupancy` and `nbytes`.
This ensures Cython still uses `WorkerState` to annotate the variable iterated over. Otherwise it constructs a generator with its own scope where this is ignored.
Apparently `_name` is sometimes an `int`. So relax typing around it to just an `object`.
jakirkham
commented
Dec 4, 2020
| _local_directory: str | ||
| _memory_limit: Py_ssize_t | ||
| _metrics: dict | ||
| _name: object |
Member
Author
There was a problem hiding this comment.
This is an object as it could be a str or an int. See discussion in PR ( #4296 ) for more context.
Member
|
This is in. Thanks @jakirkham |
Member
Author
|
Thanks Matt! 😄 |
Member
Author
|
Found a couple minor things that needed fixing. Submitted as PR ( #4321 ). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Analogous to PR ( #4290 ) except instead of
ClientState, this is annotatingWorkerStateand usages thereof. This is a bit longer simply because of how many attributesWorkerStatehas. That said, this just uses the same strategy here as was used forClientState.