Cython fixes for MemoryState
#4761
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
Cython complains if we use `object.__new__` here. Besides this class is marked `final`. So there is no other inheritance. Thus this can just use `MemoryState` directly for object creation.
As Cython attributes don't support access through `getattr`, this code doesn't really work in Cython. So just do a simple `for`-loop and spell out each attribute that is being added.
For some reason these additional decorators create properties that don't behave particularly well in Cython. Instead of being able to access the property as expected, we see exceptions about `builtin_function_or_method`. Admittedly there may be a Cython bug involved. However things like `ccall` don't make much sense when applied to a `property` (as it shouldn't create a Python and C function for it). Same story with `inline` (though we can just access the `_*` name for this affect). `nogil` may make sense for the code itself. However `@property` access does involve some Python GIL using code. So it's probably best to drop it as well.
jakirkham
force-pushed
the
cy_fixes_memst
branch
from
c65066b to
68cbb06
Compare
Collaborator
|
Not sure I understand the rationale? What is it that Cython does not cope with? |
Member
Author
|
Please see the linked issue in the OP |
jrbourbeau
reviewed
Apr 28, 2021
Member
jrbourbeau
left a comment
jrbourbeau
left a comment
There was a problem hiding this comment.
Thanks for the updates here @jakirkham. I tried this PR out locally and can confirm it resolves #4760
Let's handle making sure CI is cythonizing properly in a separate PR (xref #4764)
jrbourbeau
approved these changes
Apr 28, 2021
Member
jrbourbeau
left a comment
jrbourbeau
left a comment
There was a problem hiding this comment.
Let's merge this in after CI finishes
Member
Author
|
Thanks James! 😄 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
black distributed/flake8 distributed/isort distributedThere were some issues that surfaced when using the Cythonized Scheduler's dashboard with the recently added
MemoryStateclass. One issue was around the use ofobject.__new__. However fixing that led to other issues. This fixes them basically until the reproducer in the issue linked above is able to render the dashboard.cc @crusaderky @jrbourbeau @quasiben