-
-
Notifications
You must be signed in to change notification settings - Fork 29.2k
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
gh-87533: Expand pickle importing to support non-package C-modules #119152
base: main
Are you sure you want to change the base?
gh-87533: Expand pickle importing to support non-package C-modules #119152
Conversation
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
There have been recurring issue with PyModule_Create modules in PyTorch; when trying to serialize attributes of these C-modules, pickle fails to import the C-module because it is not a package This is the current issue that brought this to my attention: pytorch/pytorch#126154 The existing hack to this issue has been to insert the C-module into sys.modules in order to enable pickle to find them: https://github.com/pytorch/pytorch/pull/38136/files#diff-d7e90d0f94b43db763b44fba679a5c1b4cabe3668aaf34f2aee07de8e2d1b2faR524-R528 Instead of relying on this hack, we can change `pickle`'s approach to loading, which is currently equivalent to `import package.c_module`; instead, we could do `from package import c_module`, which 1) does not care if `c_module` is a package or not 2) is fully backward compatible with the previous approach and 3) slots in nicely to the `fromlist` parameter of `__import__`, which we are already using to load modules in `pickle`
9b8377e
to
52a2a20
Compare
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
f095f11
to
1831907
Compare
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Definitely some pretty interesting stuff going on in: https://github.com/python/cpython/blob/main/Lib/test/test_datetime.py Testing outside of this file, I can't seem to reproduce this behavior so I think the errors are specific to whatever is going on in EDIT: Each of |
Also remove incorrect notes and unnecessary steps
…rent Otherwise you might set a value for the module in sys.modules, but have it go unused and instead be accessed through the parent
@serhiy-storchaka can I ask for your opinion on this? I see you have been one of the most active contributors to |
There have been recurring issues with
PyModule_Create
modules in PyTorch. When trying to serialize attributes of these C-modules,pickle
fails to import the C-module because they are not a packages.This is the current issue that brought this to my attention:
dill
picklable after usingtorch.compile
pytorch/pytorch#126154The existing hack to get around this has been to insert the C-module into
sys.modules
in order to enable pickle to find them. Instead of relying on this hack, we can changepickle
's approach to loading, which is currently equivalent toimport package.c_module
.Instead, we can do what equates to
from package import c_module
when the module is not a package. This is nice because:c_module
is a package or notfromlist
parameter of__import__
, which we are already using to load modules inpickle
(albeit a bit strangely since we aren't currently using the return value of__import__
, which can also be tied us not currently usingfromlist
).We still retrieve the module from
sys.modules
instead of by attribute on its parent module when it is a package. It can lead to some rather unexpected behavior when trying to manipulatesys.modules
to control available packages as we do in some existing tests.