Fixes some bugs when using `dump_session()` with `byref=True` #463

leogama · 2022-04-22T14:35:13Z

Here are the proposed fixes for bugs reported in #462.

You can review the changes already, but please don't merge it yet as I'm still tracking a strange third bug in dump_session(). And I spotted yet another possible bug, but need to test it first. I'll probably append more commits in the next days.

I'm working on this feature for it to be used as the cache mechanism for the reticulate R package —knitr's Python engine. It's mostly functional by now. (Related discussion: yihui/knitr#1505)

By the way, could you clarify a thing for me? Why is it that, even when the byref parameter is set to True, after the imported objects are identified and stored in the __dill_imported object, the Pickler is called with the _byref attribute set to False. What's the rationale? Is it because the session is saved as a whole module and then pickle would try to store everything inside it by name reference?

Fixes #462

… modules When no objects are found to be imported from external modules, `_stash_modules()` should return `main_module` unmodified, and not the pair list of objects created from it (`original`).

…was imported Just calling `import multiprocessing` creates a `'__mp_main__'` entry in `sys.modules` that is simply a reference to the `__main__` module. In the old version of the test for module membership, objects in the global scope are wrongly attributed to this virtual `__mp_main__` module. And therefore `load_session()` fails. Testing for object identity instead of module name resolves the issue.

mmckerns · 2022-04-22T22:52:41Z

Yes, if we didn't set _byref=False while trying to store the main module, it will attempt to use name reference for modules, etc.

The PR looks good thus far. FYI, I expect that we will create a new release this coming week.

leogama · 2022-04-24T17:47:36Z

The PR looks good thus far. FYI, I expect that we will create a new release this coming week.

This are great news! It would be awesome to have dump_session(byref=True) working fine in this next release. I will definitely need it to be able to propose changes in the (maybe) two R packages related to the usage of dill as a cache engine for Python in R Markdown documents. Do you mind to include this PR in the 0.3.5 milestones? I'm happy if any number of these bug fixes are included, and will continue to work on this feature in a new PR after the release.

mmckerns · 2022-04-24T20:11:42Z

if we can finish the PR, it will be included in the release

…e in `dump_session(byref=TRUE)` Currently, `dump_session(byref=True)` misses some imported objects. For example: - If the session had a statement `import numpy as np`, it may find a reference to the `numpy` named as `np` in some internal module listed in `sys.resources`. But if the module was imported with a non-canonical name, like `import numpy as nump`, it won't find it at all. Mapping the objects by id in `modmap` solves the issue. Note that just types of objects usually imported under an alias must be looked up by id, otherwise common objects like singletons may be wrongly attributed to a module, and such reference in the module could change to a different object depending on its initialization and state. - If a object in the global scope is a top level module, like `math`, again `save_session` may find a reference to it in another module and it works. But if this module isn't referenced anywhere else, it won't be found because the function only looks for objects inside the `sys.resources` modules and not for the modules themselves. This commit introduces two new attributes to session modules saved by reference: - `__dill_imported_as`: a list with (module name, object name, object alias in session) - `__dill_imported_top_level`: a list with (module name, module alias in session) I did it this way for forwards (complete) and backwards (partial) compatibility. Oh, and I got rid of that nasty `exec()` call in `_restore_modules()`! ;)

Fixes RecursionWarning error where a function defined in the top level of the module being saved with `dump_session(byref=True)`, of which "globals" is a reference to the original module's `__dict__`, makes dill to try to save this instead of the modified module's `__dict__`, triggering recursion.

leogama · 2022-04-26T22:24:20Z

Hello there! My PR is ready for review. I rushed to solve all the problems I've found using dump_session(), so it could be included in the next release. The overall result seems to be satisfactory and working in both python2 and python3 (3.8 on my machine). I still don't understand fully the function saving logic, but contrary to my first belief it didn't need to be modified.

Do we need some unit tests? How can I provide them? Should it just reproduce the kinds of usage that were failing before these modification? I'm new to testing.

mmckerns · 2022-04-26T22:29:20Z

That's great. I'll review it tonight. Yes, dump_session sorely needs unit tests. Most of dill has good unit test coverage, however dump_session may have none or at least very very few. Testing is done in dill.tests. You are free to add a module in dill.tests, if one doesn't exist that tests dump_session. Then, you should include yourself as an author of the file, as an additional line to the standard file header.

leogama · 2022-04-26T22:57:03Z

Thank you! I'll take a look at the tests.

mmckerns · 2022-04-26T23:26:11Z

tests have to pass for the currently supported versions of python which are: (python 2.7, 3.7-3.10; PyPy 2.7, 3.7-3.9) and should also pass for python 3.11. We will drop support for python 2.7 / PyPy 2.7 after this upcoming release.

mmckerns · 2022-04-26T23:30:48Z

I did some random testing -- building a class and an instance of a class, then dumping a session (with and without byref), then loading, and getting the same results either way. Also, testing that things like a = Bar(); a.__class__ is Bar work before and after the load_session, regardless of byref. Things seem to work (eyeroll -- testing needed) for all supported versions of python.

mmckerns

LGTM. Would be nice to have some tests, but can't complain as there are currently none.

leogama · 2022-04-28T16:43:37Z

Added some tests. The Travis build is failing because of a warning raised in a assert statement before the proper tests. It's not critical...

leogama · 2022-04-28T17:05:28Z

Travis doesn't like warnings. Can I substitute it by a print to stderr or a call to log()? Otherwise I will just comment out the offending lines.

mmckerns · 2022-04-28T17:11:19Z

If the raising of a warning is what you want to test, then you can catch it in a try/except block, and assert True on except. If this isn't the case, you can always use an if block to skip the line on travis, or something else. It looks like you are just trying to ensure that the modules you will test are not already imported. Could you not instead delete them from sys.modules if they are already loaded?

leogama · 2022-04-28T17:23:34Z

I added the line to check if the modules that I choose to import in the tests weren't already loaded by python or dill. My assumption is that import mod may have different side effects than from mod import obj concerning the import of the module mod, its inclusion in sys.modules or any internal, implementation dependent, details. But I don't know if this precaution is necessary, and the list of default loaded modules varies from one python version to another... The actual tests are passing, this was just to catch an eventual false negative. I'm happy to suppress this line.

leogama · 2022-04-28T17:30:15Z

Actually, I remove all the non "pre-loaded" modules from sys.modules before calling load_session(), so if one of the modules was already there from the beginning, there's no way to test if load_session() successfully imported it. And I thought that removing a module loaded by dill or python itself from sys.modules could cause problems, but maybe not.

mmckerns · 2022-04-28T17:48:24Z

I would think that if you delete a module from sys.modules and from __main__, working in the test environment you control... then the module should not be available for __main__ where you are testing until you load_session. However, maybe what matters is that regardless of what's in the environment to begin with, the environment is as expected after load_session is called.

leogama · 2022-04-28T21:38:37Z

I've removed the header "pre-test" and adapted the code. Travis builds are failing with Python 3.7 and Python 2.7. This last one "is allowed to fail", and all the tests pass with Python 2.7 on my machine though (Ubuntu Focal). The error is the same: the xml.sax module was not included in sys.modules after load_session(), but the sax module object was there. It occurred in the testing of dump_session(byref=False), and didn't reach the byref=True case.

This is an artificial test setting however, if you start to think. The use case of load_session() is in a different session, not in the same one, and definitely not after tweaking sys.modules in this way. Maybe we should save the session to a file and test for correct module loading in a following, separate, test file?

tests/test_session_1_byref_false.py

tests/test_session_0.py

tests/test_session_1_byref_false.py

tests/test_session_2_byref_true.py

mmckerns

I'm fine with the changes to _dill.py -- the tests need a few changes, however are generally good. please address the comments left inline

tests/test_session_0.py

…ng in a new session

…rder

leogama · 2022-05-01T00:03:17Z

Answered all the comments. All tests passing. Ready to go.

mmckerns

LGTM

mmckerns · 2022-06-03T14:43:52Z

@leogama: In case the OP doesn't open a dill GitHub issue, you may want to ponder this: https://stackoverflow.com/questions/72486294.

leogama · 2022-06-03T15:55:37Z

@leogama: In case the OP doesn't open a dill GitHub issue, you may want to ponder this: https://stackoverflow.com/questions/72486294.

@mmckerns, take a look at my answer there. It is half a bug and half misdesign/misuse. It may need a decision about what to do.

mmckerns · 2022-06-03T17:22:30Z

yep, that's my thought exactly

leogama · 2022-06-03T21:32:06Z

I've already made up my mind. Restoring the whole namespace of a module to a different module (with a different name!) is a really bad idea, as it overwrites the second module's attributes like __name__, __path__ and __spec__ and creates orphaned classes and functions which dill itself can't deal with. I think load_session() should just keep the main parameter for backward compatibility and ignore it from now on.

The only doubt I have is whether _import_module() should support modules created at runtime if it fails to import. Should this work?:

>>> import dill, types
>>> mod = types.ModuleType('runtime')
>>> mod.var = 'spam'
>>> dill.dump_session(main=mod)

In other session:

>>> import dill
>>> mod = dill.load_session()
>>> print(mod.var)
spam

Edit

I feel it is not so hard to do the same as above without changing _import_module, if someone needs it:

>>> import dill, sys, types
>>> sys.modules['runtime'] = types.ModuleType('runtime')
>>> mod = dill.load_session()  # it could return the module anyway
>>> print(mod.var)
spam

mmckerns · 2022-06-04T14:38:43Z

I'm pretty sure that I agree with you. I also think it'd be good if modules created at runtime (as in your example) worked.

mmckerns · 2022-06-04T14:57:00Z

@leogama: the above issue is opened here: #498

leogama added 2 commits April 22, 2022 14:00

fix dump_session(byref=True) bug when no objetcts are imported from…

b98cd44

… modules When no objects are found to be imported from external modules, `_stash_modules()` should return `main_module` unmodified, and not the pair list of objects created from it (`original`).

leogama mentioned this pull request Apr 22, 2022

cache engine for knitr rstudio/reticulate#167

Open

leogama force-pushed the fix-dump-session branch from d430c2a to 67f0b78 Compare April 25, 2022 22:01

leogama force-pushed the fix-dump-session branch from 67f0b78 to c891eb5 Compare April 25, 2022 23:41

leogama force-pushed the fix-dump-session branch from c891eb5 to 8b15b99 Compare April 26, 2022 22:14

leogama marked this pull request as ready for review April 26, 2022 22:17

mmckerns self-requested a review April 26, 2022 23:31

mmckerns approved these changes Apr 26, 2022

View reviewed changes

leogama force-pushed the fix-dump-session branch from baf9ec5 to d535f70 Compare April 28, 2022 16:34

leogama force-pushed the fix-dump-session branch from d535f70 to 17b025d Compare April 28, 2022 16:45

Added tests for load_session() and dump_session()

47a060d

leogama force-pushed the fix-dump-session branch from 17b025d to 47a060d Compare April 28, 2022 21:16