Xilinx / PYNQ-Metadata

PYNQ-Metadata provides an abstract description of reconfigurable system designs.
BSD 3-Clause "New" or "Revised" License
7 stars 8 forks source link

add a `__hash__` method to Vnlv dataclass for Python3.11 compatibility #10

Closed modularizer closed 1 year ago

modularizer commented 1 year ago

A breaking change in Python3.11 is to "Change field default mutability check, allowing only defaults which are hashable instead of any object which is not an instance of dict, list or set. (Contributed by Eric V. Smith in bpo-44674.)", see https://docs.python.org/3/whatsnew/3.11.html#dataclasses.

This makes it so Vnlv cannot be used as a default argument for DFXCore.vlnv and import pynqmetadata fails when using Python3.11, as does import pynq

The ways to fix are either to:

in order to avoid the error...

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/user/PYNQ-Metadata/pynqmetadata/__init__.py", line 5, in <module>
    from .models.bit_field import BitField
  File "/home/user/PYNQ-Metadata/pynqmetadata/models/__init__.py", line 9, in <module>
    from .dfx_core import DFXCore
  File "/home/user/PYNQ-Metadata/pynqmetadata/models/dfx_core.py", line 10, in <module>
    @dataclass(repr=False)
     ^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/dataclasses.py", line 1210, in wrap
    return _process_class(cls, init, repr, eq, order, unsafe_hash,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/dataclasses.py", line 958, in _process_class
    cls_fields.append(_get_field(cls, name, type, kw_only))
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/dataclasses.py", line 815, in _get_field
    raise ValueError(f'mutable default {type(f.default)} for field '
ValueError: mutable default <class 'pynqmetadata.models.vlnv.Vlnv'> for field vlnv is not allowed: use default_factory
>>> 

After either one of these changes, import pynqmetadata works in Python3.11.

modularizer commented 1 year ago

I am not sure what the better approach is: adding the __hash__ method here, or modifying DFXCore.vlnv to use dataclasses.field with a keyword argument of default_factory which takes in a function which creates a Vlnv instance

STFleming commented 1 year ago

Looks great, thanks for the contribution @modularizer. Can you change the base of the PR to v0.1.6? Then I'll add a python3.11 test to the github workflow.