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

Tests fail with frozendict 2.0.6 #36

Closed
matoro opened this issue Aug 19, 2021 · 9 comments · Fixed by #37
Closed

Tests fail with frozendict 2.0.6 #36

matoro opened this issue Aug 19, 2021 · 9 comments · Fixed by #37
Assignees

Comments

@matoro
Copy link

matoro commented Aug 19, 2021

Just trying to update my Synapse dependencies and noticed that one test for canonicaljson failed when using the latest release of the new frozendict.

This is testing the following versions:

  • python 3.9.6
  • canonicaljson 1.4.0
  • frozendict 2.0.6
  • simplejson 3.17.3
 * python3_9: running distutils-r1_run_phase python_test
...E...
======================================================================
ERROR: test_frozen_dict (test_canonicaljson.TestCanonicalJson)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/var/tmp/portage/dev-python/canonicaljson-1.4.0/work/python-canonicaljson-1.4.0/test_canonicaljson.py", line 110, in test_frozen_dict
    encode_canonical_json(frozendict({"a": 1})), b'{"a":1}',
  File "/var/tmp/portage/dev-python/canonicaljson-1.4.0/work/python-canonicaljson-1.4.0-python3_9/lib/canonicaljson.py", line 73, in encode_canonical_json
    s = _canonical_encoder.encode(json_object)
  File "/usr/lib/python3.9/site-packages/simplejson/encoder.py", line 296, in encode
    chunks = self.iterencode(o, _one_shot=True)
  File "/usr/lib/python3.9/site-packages/simplejson/encoder.py", line 378, in iterencode
    return _iterencode(o, 0)
  File "/var/tmp/portage/dev-python/canonicaljson-1.4.0/work/python-canonicaljson-1.4.0-python3_9/lib/canonicaljson.py", line 29, in _default
    return obj._dict
AttributeError: 'frozendict.frozendict' object has no attribute '_dict'
 
----------------------------------------------------------------------
Ran 7 tests in 0.037s
 
FAILED (errors=1)
@alex19EP
Copy link

first bad version is frozendict 2.0.4

@DMRobertson
Copy link
Contributor

I talked about this with @H-Shay. We couldn't seem to reproduce it!

I noticed that frozendict's source code contained a C extension and wondered if that might be required to see the failure.

To test if you're using the C extension, use python -c "import frozendict; print(frozendict.c_ext)". This was false in my virtual environment, so I uninstalled frozendict and reinstalled it with

pip uninstall frozendict
pip install frozendict --no-binary=:all:

Having done so, the test failed.

@matoro
Copy link
Author

matoro commented Sep 21, 2021

Confirming that my frozendict is using the C extension.

@H-Shay
Copy link
Contributor

H-Shay commented Sep 22, 2021

As @DMRobertson noted above, it appears that the frozendict object created by the C extension is incompatible with our code here:

def _default(obj):

I think the team needs to take a look at this to determine what to do in this situation, in the meantime it appears that if you avoid using the C extension it should work.

@clokep
Copy link
Member

clokep commented Sep 23, 2021

As @DMRobertson noted above, it appears that the frozendict object created by the C extension is incompatible with our code here:

The "quick" solution would probably to also determine if the C-extensions are being used and copy the dict in that case. (I'm guessing there's no equivalent version of _dict in the C-code since it is probably implement in C, not compatible with a Python dictionary.)

That's a bit hacky, but I'm not sure there are other solutions. A better fallback might be to try and copy on attribute error:

try:
    return obj._dict
except AttributeError:
    return dict(obj)

@DMRobertson
Copy link
Contributor

I wrote these notes up elsewhere but probably should have stuck them on github. For completeness:

I can see at least three implementations of frozendict.

  • V1.1 and earlier. Pure python implementation. frozendict is a brand new class with a _dict attribute.
  • V1.2--1.5, V2 without C extension. Pure python. frozendict inherits from dict and so is automatically json-encodable. That means the _default in canonicaljson isn't needed---that's why the problem wasn't reproducible at first, I think?)
  • V2 with C extension. C implementation. Not a subclass of dict, nor does it have a _dict attribute. So I'm not sure there's a nice option other than converting it to a dict beforehand?

There's also some discussion here by the current maintainer of frozendict. They were frustrated that the stdlib json module doesn't encode a generic mapping. (The rationale from the CPython core: just because something implements the mapping interface doesn't mean its entire state is exposed by that interface; we don't want to have lossy serialisation.)

@squahtx
Copy link
Contributor

squahtx commented Sep 27, 2021

V2 with C extension. C implementation. Not a subclass of dict, nor does it have a _dict attribute. So I'm not sure there's a nice option other than converting it to a dict beforehand?

A quick skim of the frozendict C extension reveals a surprising amount of hashtable implementation-y code: https://github.com/Marco-Sulla/python-frozendict/blob/master/frozendict/src/3_6/frozendictobject.c

which makes me suspect the C extension has its own dict implementation and there's no Python dict that can be pulled out to avoid taking a copy.

@DMRobertson
Copy link
Contributor

V2 with C extension. C implementation. Not a subclass of dict, nor does it have a _dict attribute. So I'm not sure there's a nice option other than converting it to a dict beforehand?

A quick skim of the frozendict C extension reveals a surprising amount of hashtable implementation-y code: https://github.com/Marco-Sulla/python-frozendict/blob/master/frozendict/src/3_6/frozendictobject.c

which makes me suspect the C extension has its own dict implementation and there's no Python dict that can be pulled out to avoid taking a copy.

Yeah, I'm guessing it's CPython's builtin dict, but with the mutability interface removed?

@squahtx
Copy link
Contributor

squahtx commented Sep 27, 2021

Yeah, I'm guessing it's CPython's builtin dict, but with the mutability interface removed?

Ah, it actually is a copy of the code!
https://github.com/Marco-Sulla/python-frozendict/blob/master/frozendict/src/3_6/frozendictobject.c
https://github.com/python/cpython/blob/main/Objects/dictobject.c

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 a pull request may close this issue.

6 participants