-
Notifications
You must be signed in to change notification settings - Fork 117
Fix annotations support on 3.14 #852
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
Conversation
With this change, the tests run for me on a local build of Python 3.14. There are a lot of failures related to sys.getrefcount() but that seems to be an unrelated issue. Closes jcrist#810. Fixes jcrist#651. Fixes jcrist#795.
| if (annotations == NULL) { | ||
| if (mod->get_annotate_from_class_namespace != NULL) { | ||
| PyObject *annotate = PyObject_CallOneArg( | ||
| mod->get_annotate_from_class_namespace, info->namespace |
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.
This is the portable approach, which should continue to work on future Python versions. If you value performance over portability, you can instead inline this function https://github.com/python/cpython/blob/9ddc7c548d45b73c84131e6d75b03c26a3e8b6e8/Lib/annotationlib.py#L824 ; it's just dict operations.
I made it a separate function in CPython so that we can be free to optimize the internal representation of annotate functions in the future. For example, perhaps in 3.15 the class will store just a code object instead of a function.
| value = _eval_type(value, cls_locals, cls_globals) | ||
| if mapping is not None: | ||
| value = _apply_params(value, mapping) | ||
| if value is None: |
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 changed _eval_type so it no longer turns None into NoneType.
|
I get a copule of Do you also get those? |
|
I didn't how, are you running the test suite exactly? I get these failures which all seem related to To run it I do: That's the latest tip of the 3.14 branch. |
|
The failures you post feel like they wouldn't be related to retrieving annotations; the code I'm changing is just in gathering annotations at class creation time, and whatever is happening in those tests is after the class is already created. |
|
Oh actually this is because of a bug in b1 that I fixed (python/cpython#133701); the fix will be in b2 which is about to go out. I can reproduce it with the following change in the test: I do think that indicates a bug in msgspec; presumably it shouldn't crash even if people mess with the |
|
The failures occurred on b1 indeed. |
Fixes at least some of the failures reported in: jcrist#852 (comment) These were exposed by a bug in 3.14b1 where TypedDict reported incorrect `__annotations__` but correct `__required_keys__`. msgspec would crash in this case. The bug is reproducible on earlier Python versions by manually manipulating attributes on a TypedDict class. It's a pretty marginal bug but I would argue the extension should be robust to this sort of edge case.
|
#853 for that one. |
I opened #854 |
|
I've confirmed via local testing that this PR fixes the Python 3.14 compatibility issue I noted in lmstudio-ai/lmstudio-python#153 |
|
Ping @jcrist - we're getting close to the final release date of Python 3.14. It'd be nice to have this merged to unblock further work to add 3.14 support here and in downstream packages that use msgspec. |
|
Another ping here. I know @kumaraditya303 has a followup for this to add support for the free-threaded build which he plans to send in as soon as this PR is merged. |
|
We are also dependent on this library and eager to upgrade. |
|
Is @kumaraditya303's free-thread branch at |
Those are also to be expected for Python 3.14. There is a new GC that will count a number of references, so any unit tests depending on |
|
#854 is a draft PR to tackle the refcount tests just by allowing more relaxed measurements in general. |
Original patch in jcrist#852.
Original patch in jcrist#852.
Yes, I created #877 for adding free-threading support. |
|
I'm in talks with Jim about co-maintenance and should hopefully hear back in the next few days. If all goes well I plan to merge and release everything rapidly. |
|
|
||
|
|
||
| if sys.version_info >= (3, 10): | ||
| from inspect import get_annotations as _get_class_annotations |
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.
We can't use this module as it introduces a large regression in terms of startup overhead:
❯ docker run --rm -it python:3.13 bash
root@fe224bc3d8c0:/# pip install -qqq msgspec
root@fe224bc3d8c0:/# python -m timeit -n 1 -r 1 "import msgspec"
1 loop, best of 1: 11.7 msec per loop
root@fe224bc3d8c0:/# python -m timeit -n 1 -r 1 "import inspect"
1 loop, best of 1: 7.84 msec per loop
It's worth noting that as of 3.14 this function moved to annotationlib (which your C code imports directly) and inspect re-exports it. That module loads twice as fast but ideally we wouldn't incur any unnecessary overhead:
❯ docker run --rm -it python:3.14 bash
root@8130942e2641:/# python -m timeit -n 1 -r 1 "import annotationlib"
1 loop, best of 1: 3.34 msec per loop
How would you recommend we proceed? I'm going to merge but this will be considered a known regression and will have to be fixed in a patch release right after.
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.
Discussion in #880
|
Merging, thanks a lot! |
|
Here's the issue tracking the performance regression #880 |
With this change, the tests run for me on a local build of Python 3.14.
There are a lot of failures related to sys.getrefcount() but that seems
to be an unrelated issue.
Closes #810. Fixes #651. Fixes #795.