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

[BUG] Equality with dict fails on MacOs #37

Closed
risicle opened this issue Nov 7, 2021 · 17 comments
Closed

[BUG] Equality with dict fails on MacOs #37

risicle opened this issue Nov 7, 2021 · 17 comments
Labels
Status: Fixed Now it works! Type: Bug Something isn't working

Comments

@risicle
Copy link

risicle commented Nov 7, 2021

On macos 10.15, with frozendict 2.0.5 or 2.0.6 and python 3.7, 3.8 or 3.9, the pickle-related tests fail for us.

Full build logs available https://hydra.nixos.org/log/qya0kdv55b2sxkfigkly17bryydbng4y-python3.8-frozendict-2.0.5.drv

Would love to know if anyone can reproduce this.

@risicle risicle added the Type: Bug Something isn't working label Nov 7, 2021
@Marco-Sulla
Copy link
Owner

Marco-Sulla commented Nov 7, 2021

So with python 3.6 works, or you don't tested it?

It's a real problem, since I don't have a Mac.

Pickle is the only problem? You can build the C extension and make the other tests?

@risicle
Copy link
Author

risicle commented Nov 7, 2021

3.6 not tested.

It's actually test_equals_dict and all the test_pickle variants. The latter of which are failing with e.g.

>               raise TypeError(f"cannot pickle {cls.__name__!r} object")
E               TypeError: cannot pickle 'dict' object

@Marco-Sulla
Copy link
Owner

This is the py version. Have you tried to install the sdist package and see if it works?

@Marco-Sulla
Copy link
Owner

Anwyay, I installed the pure py version using ./setup.py py bdist_wheel and I can't reproduce the bug on Mojave.

@risicle
Copy link
Author

risicle commented Nov 7, 2021

Well, the variants that fail are in test/test_frozendict_c.py and test/test_frozendict_c_subclass.py. test/test_frozendict.py passes fine for us too.

@Marco-Sulla
Copy link
Owner

Marco-Sulla commented Nov 7, 2021

Because you are using the py version. Those tests are for the C extension. Try also test_frozendict_subclass

@Marco-Sulla
Copy link
Owner

Marco-Sulla commented Nov 7, 2021

Anyway it's true, there's a bug in the C Extension for the equality test with dict. This is VERY STRANGE, since my modification to __eq__ is REALLY small. Furthermore on Linux works. I have to investigate

@Marco-Sulla Marco-Sulla reopened this Nov 7, 2021
@Marco-Sulla Marco-Sulla changed the title [BUG] pickle tests fail on macos [BUG] equality with dict fail on macos Nov 7, 2021
@Marco-Sulla Marco-Sulla changed the title [BUG] equality with dict fail on macos [BUG] equality with dict fails on MacOs Nov 7, 2021
@Marco-Sulla Marco-Sulla changed the title [BUG] equality with dict fails on MacOs [BUG] Equality with dict fails on MacOs Nov 7, 2021
@risicle
Copy link
Author

risicle commented Nov 7, 2021

We are building the C extension, as you can see from the build log https://hydra.nixos.org/log/kygpgpsyq90i9j3wx7rmzxndcsvy00xa-python3.8-frozendict-2.0.6.drv, and the same build procedure run on linux passes all the tests https://hydra.nixos.org/log/i1lksnv7sj1sq796dlqmd9dzf7cyc4bc-python3.8-frozendict-2.0.6.drv.

I see there's some creative trickery going on in the test suite to test the different backends - something could be diverging somewhere along the line here.

p.s. as a mostly linux person who occasionally has to fix some macos stuff like this, I can say that macos is indeed very strange.

@Marco-Sulla
Copy link
Owner

I think the problem is in Clang. Probably he does not interpret well the PyAnyDict_Check macro I defined in frozendictobject.h. I'll put a good old fprintf.

@Marco-Sulla
Copy link
Owner

Bingo. The problem is there, but I don't know why.

@Marco-Sulla
Copy link
Owner

Marco-Sulla commented Nov 7, 2021

Code is:

#define PyAnyDict_Check(ob) \
    ( \
        Py_IS_TYPE(ob, &PyDict_Type) \
        || Py_IS_TYPE(ob, &PyFrozenDict_Type) \
        || Py_IS_TYPE(ob, &PyCoold_Type) \
        || PyType_IsSubtype(Py_TYPE(ob), &PyDict_Type) \
        || PyType_IsSubtype(Py_TYPE(ob), &PyFrozenDict_Type) \
        || PyType_IsSubtype(Py_TYPE(ob), &PyCoold_Type) \
    )

@Marco-Sulla
Copy link
Owner

Fixed: 76728fc

@risicle
Copy link
Author

risicle commented Nov 7, 2021

Well done - that certainly fixes the equality issue. I suspect the pickling tests are just something to do with the way we're invoking the test suite.

@Marco-Sulla
Copy link
Owner

I have no bug with pickle. Have you tried to do a manual test?

@risicle
Copy link
Author

risicle commented Nov 18, 2021

Yes it seems to fail in a manual test too:

[nix-shell:~/nixpkgs]$ python
Python 3.8.12 (default, Nov  6 2021, 01:30:52) 
[Clang 7.1.0 (tags/RELEASE_710/final)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import frozendict
>>> frozendict.c_ext
True
>>> import pickle
>>> pickle.dumps(frozendict.frozendict({1:2}))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
_pickle.PicklingError: Can't pickle <class 'dict'>: it's not the same object as builtins.dict

@Marco-Sulla
Copy link
Owner

I can't reproduce on MacOS Mojave, Python 3.10.

@Marco-Sulla
Copy link
Owner

Marco-Sulla commented Nov 21, 2021

Can you open another bug for pickle and re-test with the latest version?

@Marco-Sulla Marco-Sulla added the Status: Fixed Now it works! label Dec 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Fixed Now it works! Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants