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

Attempt to fix issue #43 #44

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

erocarrera
Copy link

This PR successfully silences the error so autoclass works with Python 3.9. I have not investigated the code in depth to be cable to guarantee it as a full solution.

This PR successfully silences the error so autoclass works with Python 3.9. I have not investigated the code in depth to be cable to guarantee it as a full solution.
@@ -257,9 +257,6 @@ def execute_autodict_on_class(cls, # type: Type[T]
'__contains__',
'get',
'items',
Copy link
Owner

@smarie smarie May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removal makes the python 2 test fail since this if/else statement is specifically for python 2. We should not modify this branch but rather try to understand why the main path fails lines 234-239

try:
            cls.__bases__ = new_bases
        except TypeError:
            try:
                # maybe a metaclass issue, we can try this
cls.__bases__ = with_metaclass(type(cls), *new_bases)
``

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing me in the right direction.
Under Python3.9, the code: cls.__bases__ = new_bases throws TypeError: __bases__ assignment: 'Mapping' deallocator differs from 'object' which I'm guessing might be the exception you're trying to catch.

The cls.__bases__ = with_metaclass(type(cls), *new_bases) throws TypeError: can only assign tuple to Experiment.__bases__, not metaclass (Experiment is the name of my class where I'm using autoclass)

I naively tried to make it a tuple as cls.__bases__ = (with_metaclass(type(cls), *new_bases),) and I get the exception TypeError: __bases__ assignment: 'temporary_class' deallocator differs from 'object'

I don't know what to try next, but maybe this gives you some information?

Copy link
Owner

@smarie smarie May 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for providing this ! Well this does not look good. I understand why you ended up fixing the last part of the try/except.

Not sure that I can fix this very soon. I'll have a look in the upcoming weeks.

If you wish to have a stab at it in between, you can try to

  • revert the changes you did,
  • create a boolean version flag (PY38 = sys.version_info >= (3, 8))
  • use that flag in the last "except" (just where you did your first mod) to skip the method names that are not relevant anymore, and to not use .im_func.

Also note that a more recent lib that I consider better than autoclass for my personal use is https://smarie.github.io/python-pyfields/ . It also provides an @autoclass decorator. Instead of fiddling with the class bases, this decorator adds to_dict and from_dict methods.

Still of course I try to maintain both when possible

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Sylvain, I've moved to pyfields as you suggested, that solved the issue for me. I will not grapple with trying to address the issue in autoclass. Feel free to close the PR if you want.

@smarie
Copy link
Owner

smarie commented May 11, 2021

Thanks a lot @erocarrera ! Sorry for not spotting your PR sooner.

As commented above, it seems that you modified a try/except branch that was dedicated to a special case of python 2... So the issue is rather to understand why the code on python 3.8+ enters this branch at all, in other words to understand why

cls.__bases__ = new_bases

fails in the first place.

@smarie
Copy link
Owner

smarie commented Aug 31, 2021

(replying to #44 (comment) )

Perfect @erocarrera thanks for the feedback ! Indeed moving to pyfields when possible is probably a better choice. I did not yet found time to fix this autoclass issue, sorry :( we'll see if many users are interested or if they all move away to pyfields, attrs, dataclasses and the like

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

Successfully merging this pull request may close these issues.

None yet

2 participants