feat: make slurmutils compatible with python 3.14 - #72
Conversation
eb15171 to
9eb4733
Compare
NucciTheBoss
left a comment
There was a problem hiding this comment.
We stan non-backward compatible changes in a minor version release with little to no deprecation warning ![]()
Couple of questions and suggestions around the implementation here. I think we need to make your comment more clear that __annotations__ is now lazily loaded rather than eagerly in Python 3.14 and above.
Also, this issue is making me realize that sooner rather than later we need to take a hard look at migrating the Slurm data models to pydantic in slurmutils v2. We rely heavily on how Python handles type annotations to provide additional metadata about Slurm configuration attributes, so further changes to how Python handles type hints/annotations in future releases may create even more headaches for us (also I feel like static typing is becoming more mainstream with LLM-assisted development).
| # `annotationlib` was added in Python 3.14 and replaces the old | ||
| # `__annotations__` property on all classes, so for backwards compat | ||
| # we provide a minimal shim mirroring the subset of the | ||
| # `annotationlib.get_annotations` that we use. |
There was a problem hiding this comment.
This comment seems misleading to me. To me it implies that the __annotations__ attribute was removed entirely from Python 3.14 when the real issue here is that we're relying on the old behavior of the __annotations__ attribute where it's computed eagerly, but Python 3.14 has changed to computing this value lazily.
The comment should be reworded to state something along the lines:
| # `annotationlib` was added in Python 3.14 and replaces the old | |
| # `__annotations__` property on all classes, so for backwards compat | |
| # we provide a minimal shim mirroring the subset of the | |
| # `annotationlib.get_annotations` that we use. | |
| # Python < 3.14 computed the `__annotations__` attribute eagerly, but Python >= 3.14 now | |
| # computes the value of this attribute lazily. PEP 749 encourages explicitly using the | |
| # `get_annotations` function from the `annotationlib` module to compute the | |
| # annotation dictionary rather than read the lazily-loaded `__annotations__` attribute. | |
| # `annotationlib` was added in Python 3.14, so for backwards compatibility we need to create | |
| # this minimal shim that mirrors the functionality of the `get_annotations` for Python < 3.14. |
There was a problem hiding this comment.
namespace["__annotations__"] was removed and replaced by the get_annotations API, but the new get_annotations API also brought the additional change in behaviour. Though now that I read my comment, it does read very weirdly so I'll adjust it.
Scratch that, I double checked and yeah, thought __annotations__ was deleted but it's just computed lazily now
Scratch the scratch, my comment wasn't precise enough. __annotations__ was deleted from the namespace argument of ABCMeta classes, but it is still available on normal classes.
9eb4733 to
0e3d6cd
Compare
Yes, the code is filled with very advanced Python snippets that might not be as easy to maintain in the long run. If we can migrate to pydantic ASAP I would be very happy |
NucciTheBoss
left a comment
There was a problem hiding this comment.
One last qualm and then I'm good to merge this PR.
I think we need to be more explicit about __annotations__ now being lazily evaluated in Python 3.14+, and that we're primarily introducing get_annotations with a shim for Python < 3.14 so that we can safely access annotations across all versions.
Co-authored-by: Jason Nucciarone <40342202+NucciTheBoss@users.noreply.github.com>
09b8b2f to
774074c
Compare
|
@NucciTheBoss Done! Should be ready for publishing now |
Thanks bro 🦀 |
Pre-submission checklist
Summary of changes
Between Python 3.12 and 3.14 there was a change on how annotation inspection works; you can still inspect annotations, but they're lazily resolved. As part of that,
namespace["__annotations__"]was deleted in favour ofannotationlib.get_annotation.This PR adds a shim to address this breaking change while preserving compatibility with Python 3.10-3.12
Docs
Internal change, doesn't require documentation.