Skip to content
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

cannot pickle dump object from child idl file class #215

Closed
truebit opened this issue Jun 14, 2023 · 6 comments · Fixed by #249 · May be fixed by #295
Closed

cannot pickle dump object from child idl file class #215

truebit opened this issue Jun 14, 2023 · 6 comments · Fixed by #249 · May be fixed by #295

Comments

@truebit
Copy link
Contributor

truebit commented Jun 14, 2023

add below code in the end of tests.test_hook.test_load test method:

    # load with sub_module name and it can be pickled
    list_item = ab_2.container.ListItem()
    assert list_item == pickle.loads(pickle.dumps(list_item))

fail log is:

> FAILED                                           [100%]
> tests/test_hook.py:31 (test_load)
> def test_load():
>         ab_1 = thriftpy2.load("addressbook.thrift")
>         ab_2 = thriftpy2.load("addressbook.thrift",
>                               module_name="addressbook_thrift")
>     
>         assert ab_1.__name__ == "addressbook"
>         assert ab_2.__name__ == "addressbook_thrift"
>     
>         # load without module_name can't do pickle
>         with pytest.raises(pickle.PicklingError):
>             pickle.dumps(ab_1.Person(name='Bob'))
>     
>         # load with module_name set and it can be pickled
>         person = ab_2.Person(name='Bob')
>         assert person == pickle.loads(pickle.dumps(person))
>     
>         # load with sub_module name and it can be pickled
>         list_item = ab_2.container.ListItem()
> \>       assert list_item == pickle.loads(pickle.dumps(list_item))
> E       _pickle.PicklingError: Can't pickle <class 'container.ListItem'>: attribute lookup ListItem on container failed
> 
> test_hook.py:50: PicklingError
@aisk
Copy link
Member

aisk commented Jun 14, 2023

This is not a bug, it's caused by pickle's behavior.

The error is raised from here: https://github.com/python/cpython/blob/main/Modules/_pickle.c#L3695 , as you can see, during the pickle.dumps process, pickle will try to import the module.

In your example, there is a missleading problem, the module ab_1.container is tests/container.thrift actually, and there is a test/container.py file exists beside the thrift file, and it's actually broken (missing the ListItem type), which we should fixed in the future (PR is welcomed!).

Delete the container.py, the actually error will be raised:

PicklingError: Can't pickle <class 'container.ListItem'>: import of module 'container' failed

We can figure out that pickle is complain that the module container is not exist. It's true because we're generating the module on the fly. And the generated module will not insert into the interpreter's import cache (sys.modules) if you load it without giving a name, and the module is a "wild module".

By load the module with thriftpy2.load("container.thrift", module_name = "container_thrift"), the generated module will be inserted into sys.modules, which will be used as a cache while importing. So the code will be worked as expected.

@truebit
Copy link
Contributor Author

truebit commented Jun 14, 2023 via email

@aisk
Copy link
Member

aisk commented Jun 16, 2023

I think we should add an option in the load function that enables automatic specification of the module name. For now, you can load the child thrift files beforehand as a workaround.

@truebit
Copy link
Contributor Author

truebit commented Jun 19, 2023

I tried to fix this issue, but it seems that only executing test_hook itself would pass the test; when with the whole test suite, test_hook would fail:

diff --git a/tests/test_hook.py b/tests/test_hook.py
index e5cc7c5..91e40d0 100644
--- a/tests/test_hook.py
+++ b/tests/test_hook.py
@@ -45,6 +45,10 @@ def test_load():
     person = ab_2.Person(name='Bob')
     assert person == pickle.loads(pickle.dumps(person))
 
+    # load with first level sub_module name and it can be pickled
+    list_item = ab_2.container.ListItem()
+    assert list_item == pickle.loads(pickle.dumps(list_item))
+
 
 def test_load_module():
     ab = thriftpy2.load_module("addressbook_thrift")
diff --git a/thriftpy2/parser/__init__.py b/thriftpy2/parser/__init__.py
index cb3e4b8..18ad1e4 100644
--- a/thriftpy2/parser/__init__.py
+++ b/thriftpy2/parser/__init__.py
@@ -12,6 +12,7 @@ from __future__ import absolute_import
 import os
 import sys
 import types
+import inspect
 
 from .parser import parse, parse_fp, incomplete_type, _cast
 from .exc import ThriftParserError
@@ -34,11 +35,17 @@ def load(path,
     """
     real_module = bool(module_name)
     thrift = parse(path, module_name, include_dirs=include_dirs,
-                   include_dir=include_dir)
+                   include_dir=include_dir, encoding=encoding)
     if incomplete_type:
         fill_incomplete_ttype(thrift, thrift)
     if real_module:
         sys.modules[module_name] = thrift
+        for item in dir(thrift):
+            element = getattr(thrift, item)
+            if inspect.ismodule(element):
+                if item not in sys.modules:
+                    sys.modules[item] = element
+
     return thrift

@helpau
Copy link
Contributor

helpau commented Aug 5, 2023

I tried to fix it in #219

@aisk
Copy link
Member

aisk commented Nov 29, 2023

Sorry, I've lost the context. does #219 fixed the problem?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants