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

JSON serialisation fails #453

Closed
Ttayu opened this issue Nov 20, 2023 · 8 comments · Fixed by #454
Closed

JSON serialisation fails #453

Ttayu opened this issue Nov 20, 2023 · 8 comments · Fixed by #454
Labels
regression This is an inintended change for the worse.

Comments

@Ttayu
Copy link

Ttayu commented Nov 20, 2023

  • cattrs version: 23.1.2 -> 23.2.1
  • Python version: 3.10.6, 3.10.12, 3.11.5
  • Operating System:
    • WSL-Ubuntu-22.04
      Linux 5.15.133.1-microsoft-standard-WSL2
    • Ubuntu-22.04.3 LTS
      5.15.0-88-generic

Description

Please refer this.
python-lsp/python-lsp-ruff#61

I'm guess it may be that serialisation fails when dataclass is included in cattrs==23.2.1

What I Did

Paste the command(s) you ran and the output.
If there was a crash, please include the traceback here.
@Tinche
Copy link
Member

Tinche commented Nov 20, 2023

Looks like this is a regression introduced in #381 for a type annotated as Any | None.

Might need to contemplate this a little since it's not very clear to me what the correct behavior should be.

@Tinche Tinche added regression This is an inintended change for the worse. and removed more-info-needed More information required. labels Nov 20, 2023
@Tinche Tinche linked a pull request Nov 20, 2023 that will close this issue
@Tinche Tinche closed this as completed Nov 21, 2023
jvesely added a commit to jvesely/PsyNeuLink that referenced this issue Nov 22, 2023
@jvesely
Copy link

jvesely commented Nov 22, 2023

This issue still appears to be present in 23.2.2

@Tinche
Copy link
Member

Tinche commented Nov 22, 2023

Any chance of a small reproducer? OP has confirmed their issue is solved on 23.2.2 in a different ticket.

@kmantel
Copy link

kmantel commented Nov 23, 2023

Here's one.

import json
from typing import Any, Dict, Optional

import attr
import cattr
import numpy as np


def _unstructure_value_expr(o):
    try:
        return o.tolist()
    except AttributeError:
        return converter.unstructure(o)


converter = cattr.Converter()
converter.register_unstructure_hook(np.ndarray, _unstructure_value_expr)


@attr.define
class A:
    dict_field: Optional[Dict[str, Any]] = attr.field()


a = A({'arr': np.array([0])})
print(json.dumps(converter.unstructure(a)))

This gives TypeError: Object of type ndarray is not JSON serializable on 23.2.1 and 23.2.2, but gives correct output on 23.1.1 and 23.1.2

Changing the dict_field line to
dict_field: Optional[Dict[str, np.ndarray]] = attr.field()
or
dict_field: Dict[str, np.ndarray] = attr.field()

gives the correct output on each version.

while

dict_field: Dict[str, Any] = attr.field()

fails on all four.

This is on Python 3.11.6 but I don't think it depends on that

@Tinche
Copy link
Member

Tinche commented Nov 23, 2023

Ah interesting, looks like we have another regression in handling dict values. Will check it out today.

@Tinche
Copy link
Member

Tinche commented Nov 23, 2023

@kmantel can you try https://github.com/python-attrs/cattrs/tree/tin/fix-dict-any and see if it fixes your issue?

jvesely added a commit to jvesely/PsyNeuLink that referenced this issue Nov 26, 2023
@jvesely
Copy link

jvesely commented Nov 28, 2023

@kmantel can you try https://github.com/python-attrs/cattrs/tree/tin/fix-dict-any and see if it fixes your issue?

@Tinche, the above link does not work but using the top of 32.2 branch (pip install git+https://github.com/python-attrs/cattrs.git@23.2) fixes the issues in psyneulink (referenced in the above commit).
Is there a new release planned soon?

jvesely added a commit to PrincetonUniversity/PsyNeuLink that referenced this issue Nov 28, 2023
@Tinche
Copy link
Member

Tinche commented Nov 28, 2023

Yeah, in the next day or two as I chase down a different issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression This is an inintended change for the worse.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants