-
-
Notifications
You must be signed in to change notification settings - Fork 183
Fix Python 3.14 compat with HF Datasets #724
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
Can you explain this PR a bit more? Specifically, why this belongs in |
@mmckerns Good question! I will double check. After relooking at the stack it does appear to be method def changes to Python 3.14's pickler and not specically anything that dill is doing. It could may wil be fixed via python version check in datasets. |
@Qubitium: thanks for the follow-up. So, we'll have to look at the broader potential impact of the change in python. |
@Qubitium @mmckerns thanks for the PR. However I think this should be a downstream fix. The error is caused by I put some inspection code in
For more information, see my PR for |
Thanks for the core fix! |
@Qubitium @mmckerns After more experimentation I realized that there might still be a need for upstream fix. That is, Take I'm not sure whether this is desirable. If Python decided to make this change in standard library, downstream should be encouraged to address it proactively, instead of being stuck on the old API, simply because it still "works" because of the tricks in |
The typical policy for |
@mmckerns Perhaps something like this: _MISSING = object()
def _batch_setitems(items, obj=_MISSING):
if py_version >= 3.14:
if obj is _MISSING:
raise TypeError("breaking change in Py 3.14...")
return super()._batch_setitems(items, obj)
else:
return super()._batch_setitems(items) |
Summary
TypeError: Pickler._batch_setitems() takes 2 positional arguments but 3 were given
with HF DatasetsNotables:
dill/tests/test_pickle_batch_setitems.py
new test is added to test the hfdatasets
crash fix when loading any dataset.Test to run under dill + python 3.14 + datasets to replicate the stacktrace :
For test_threads.py
This original check was removed as it never passed in Python 3.14 with or without PYTHON_GIL=0.
Checklist
Documentation and Tests
python tests/__main__.py
, and pass.R.
Release Management
Finally
@mmckerns The fix and tests were generated by codex but I did review every delta as much as possbile. I am not exactly sure why the thread is_alive() check is failing on python 3.14 (check removed). Please double check the fixes and especially the
Pickler._batch_setitems()
fix. Thanks.The pytest conversion for usability since the output and stacktrace helpers is easier to pinpoint erros. But pytest does inject some wrappers to object types which the updated test codes had to skip or nullify.