cloudpipe / cloudpickle

Extended pickling support for Python objects
Other
1.64k stars 167 forks source link

Fix NamedTuple issues on Python 3.9 #492

Closed RyanClark2k closed 1 year ago

RyanClark2k commented 1 year ago

This PR fixes issue #460. Two changes were required. First, if __module__ was present in obj.__dict__, we need to pass it along to type_kwargs. See error message below.

cls = <class 'typing.NamedTupleMeta'>, typename = 'MyTuple', bases = (<class 'typing.NamedTuple'>,), ns = {'__orig_bases__': (<function NamedTuple at 0x7fc0780f9310>,), '__slots__': ()}

    def __new__(cls, typename, bases, ns):
        assert bases[0] is _NamedTuple
        types = ns.get('__annotations__', {})
        default_names = []
        for field_name in types:
            if field_name in ns:
                default_names.append(field_name)
            elif default_names:
                raise TypeError(f"Non-default namedtuple field {field_name} "
                                f"cannot follow default field"
                                f"{'s' if len(default_names) > 1 else ''} "
                                f"{', '.join(default_names)}")
        nm_tpl = _make_nmtuple(typename, types.items(),
                               defaults=[ns[n] for n in default_names],
>                              module=ns['__module__'])
E       KeyError: '__module__'

Second, if we pass __slots__ and __module__ to type_kwargs then we get the following error:

cls = <class 'typing.NamedTupleMeta'>, typename = 'MyTuple', bases = (<class 'typing.NamedTuple'>,)
ns = {'__module__': 'tests.cloudpickle_test', '__orig_bases__': (<function NamedTuple at 0x7fa300131310>,), '__slots__': ()}

    def __new__(cls, typename, bases, ns):
        assert bases[0] is _NamedTuple
        types = ns.get('__annotations__', {})
        default_names = []
        for field_name in types:
            if field_name in ns:
                default_names.append(field_name)
            elif default_names:
                raise TypeError(f"Non-default namedtuple field {field_name} "
                                f"cannot follow default field"
                                f"{'s' if len(default_names) > 1 else ''} "
                                f"{', '.join(default_names)}")
        nm_tpl = _make_nmtuple(typename, types.items(),
                               defaults=[ns[n] for n in default_names],
                               module=ns['__module__'])
        # update from user namespace without overriding special namedtuple attributes
        for key in ns:
            if key in _prohibited:
>               raise AttributeError("Cannot overwrite NamedTuple attribute " + key)
E               AttributeError: Cannot overwrite NamedTuple attribute __slots__

/Users/ryanc/opt/anaconda3/lib/python3.9/typing.py:1884: AttributeError

To resolve this, I deleted the lines passing __slots__ to type_kwargs. Our unit test test_instance_with_slots still passes with this change. The deleted __slots__ lines were written 4 years ago and are possibly no longer useful. If there is reason to believe removing it could cause a regression, we should at least add a unit test that properly tests the functionality provided by these lines.

I've run all unit tests locally with Python 3.6, 3.7, 3.8, 3.9, and 3.10 and verified non-regression. The new NamedTuple test fails on develop with Python 3.9 and 3.10 but passes on this branch. I'm happy to iterate here if there are changes needed.

ryancdotorg commented 1 year ago

Highly recommend not merging this until the commit is modified.

RyanClark2k commented 1 year ago

@ogrisel do you mind taking a look at this?

ryancdotorg commented 1 year ago

the committer email is still tied to my account, but the author is fixed...

ogrisel commented 1 year ago

Hum, I wonder why the CI is no longer running on this repo...

Meanwhile, could you please improve test_instance_with_slots to check that the __slots__ attribute of the class of loaded instance has the same value as the slot specification of the original class?

ogrisel commented 1 year ago

I merged master to try to get the CI running.

codecov[bot] commented 1 year ago

Codecov Report

Base: 83.42% // Head: 83.28% // Decreases project coverage by -0.14% :warning:

Coverage data is based on head (4823850) compared to base (68a4230). Patch coverage: 50.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #492 +/- ## ========================================== - Coverage 83.42% 83.28% -0.14% ========================================== Files 4 4 Lines 718 718 Branches 157 157 ========================================== - Hits 599 598 -1 Misses 95 95 - Partials 24 25 +1 ``` | [Impacted Files](https://codecov.io/gh/cloudpipe/cloudpickle/pull/492?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudpipe) | Coverage Δ | | |---|---|---| | [cloudpickle/cloudpickle\_fast.py](https://codecov.io/gh/cloudpipe/cloudpickle/pull/492?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudpipe#diff-Y2xvdWRwaWNrbGUvY2xvdWRwaWNrbGVfZmFzdC5weQ==) | `91.43% <50.00%> (-0.31%)` | :arrow_down: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudpipe). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudpipe)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

RyanClark2k commented 1 year ago

Closing in favor of #495 due to committer email mixup.