-
Notifications
You must be signed in to change notification settings - Fork 752
pyverbs: Align code to support Cython 3.1.0 format #1615
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the motivation, but the resulting code is cumbersome, especially when these enums are used frequently.
Would it be possible to provide shorter aliases or import patterns for common enums, at least in the test code, to improve readability?
@EdwardSro, I agree its indeed cumbersome but Cython changed their behavior from C like enums to python like enums so the code can't use the existing c-like implementation which is much more convenient where all enums are just exposed without the enum prefix. I couldn't really find a reasonable way around it so indeed it became a huge change.
The only option I see here is doing something like |
Maybe we can still globalize them using something like "globals().update(MyEnum.members)"? I believe we can avoid this significant change, which will affect external Pyverbs users as well. |
@EdwardSro I did consider that option, but it feels more like a workaround than a proper solution. It essentially "hacks" Python’s global namespace to reintroduce the enums. While the change is smaller, it maintains a behavior that neither Python nor Cython officially supports—and there’s always the risk that this workaround could be removed in the future. I’m open to changing it if you strongly prefer that approach, but in my view, it’s better to stick with Python’s natural behavior and avoid relying on unsupported patterns. |
Agree this is a really big change that needs to be carefully reviewed, but as far as I see it should not break external Pyverbs users. |
9b45888
to
d489978
Compare
Hi, sorry to bother you~ I'm new with RDMA, but I met some problems when running rdma-core/tests/run_tests.py.
I think it should be I have no idea how to deal with it, cause it seems that e.XXXX is used many times. |
@Crazy-JY, Hey thats what this patch fixes. You can just cherry-pick it for now until it or a variation of it will be merged. |
Oh Okay thx~ Looking forward to it~ |
In Cython 3.1.0 named cpdef enums no longer copy their item names into the global module namespace. This means each named enum must be imported explicitly to be used in a file or else an import exception will be raised. Align pyverbs code to explicitly import enums so it will be able to execute on relatively new distributions that cython version in them is newer than 3.1.0: Ubuntu 22.04/24.04, Debian 12, RHEL 9, Rocky Linux 9 and AL2023. This change was also tested on cython 3.0.12 to verify it works on older versions. Link: https://cython.readthedocs.io/en/latest/src/changes.html#id10 Signed-off-by: Yonatan Nachum <[email protected]>
Hey @EdwardSro how do you want to proceed with this change ? its fully backwards compatible and doesn't suppose to break older Cython versions so its suppose to be safe for any pyverbs users. |
On second thought, I tend to agree that it's better to stick with Python's natural behavior...
It does. Any external Pyverbs test/app needs to adapt to the new Python-like enums. Sorry for the delay, I'll do my best to fully review this patch in the upcoming days. |
In Cython 3.1.0 named cpdef enums no longer copy their item names into the global module namespace. This means each named enum must be imported explicitly to be used in a file or else an import exception will be raised. Align pyverbs code to explicitly import enums so it will be able to execute on relatively new distributions that cython version in them is newer than 3.1.0: Ubuntu 22.04/24.04, Debian 12, RHEL 9, Rocky Linux 9 and AL2023.
This change was also tested on cython 3.0.12 to verify it works on older versions.
Link: https://cython.readthedocs.io/en/latest/src/changes.html#id10