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

support Python3.11 #2418

Merged
merged 13 commits into from Dec 5, 2022
Merged

Conversation

skshetry
Copy link
Contributor

Motivation

When trying to import hydra in Python 3.11, it throws following error:

  File "/home/user/projects/iterative/hydra/hydra/conf/__init__.py", line 75, in JobConf
    @dataclass
     ^^^^^^^^^
  File "/home/user/.pyenv/versions/3.11-dev/lib/python3.11/dataclasses.py", line 1221, in dataclass
    return wrap(cls)
           ^^^^^^^^^
  File "/home/user/.pyenv/versions/3.11-dev/lib/python3.11/dataclasses.py", line 1211, in wrap
    return _process_class(cls, init, repr, eq, order, unsafe_hash,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/.pyenv/versions/3.11-dev/lib/python3.11/dataclasses.py", line 959, in _process_class
    cls_fields.append(_get_field(cls, name, type, kw_only))
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/.pyenv/versions/3.11-dev/lib/python3.11/dataclasses.py", line 816, in _get_field
    raise ValueError(f'mutable default {type(f.default)} for field '
ValueError: mutable default <class 'hydra.conf.JobConf.JobConfig.OverrideDirname'> for field override_dirname is not allowed: use default_factory

This is due to a recent change in dataclasses in Python 3.11 that disallows mutable to be set as a default value. This is done via check for hashability.

See python/cpython#88840. Note that I am trying to add support for Python 3.11 to hydra. This seems like a major blocker.

For reproducibility, I used the following command to search for these:

rg ".*: .* = [A-Z].*\(.*\)"

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

If all tests pass, it should be good.

Related Issues and PRs

When trying to import hydra in Python 3.11, it throws following error:
```python
  File "/home/user/projects/iterative/hydra/hydra/conf/__init__.py", line 75, in JobConf
    @DataClass
     ^^^^^^^^^
  File "/home/user/.pyenv/versions/3.11-dev/lib/python3.11/dataclasses.py", line 1221, in dataclass
    return wrap(cls)
           ^^^^^^^^^
  File "/home/user/.pyenv/versions/3.11-dev/lib/python3.11/dataclasses.py", line 1211, in wrap
    return _process_class(cls, init, repr, eq, order, unsafe_hash,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/.pyenv/versions/3.11-dev/lib/python3.11/dataclasses.py", line 959, in _process_class
    cls_fields.append(_get_field(cls, name, type, kw_only))
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/.pyenv/versions/3.11-dev/lib/python3.11/dataclasses.py", line 816, in _get_field
    raise ValueError(f'mutable default {type(f.default)} for field '
ValueError: mutable default <class 'hydra.conf.JobConf.JobConfig.OverrideDirname'> for field override_dirname is not allowed: use default_factory
```

This is due to a recent change in dataclasses in Python 3.11 that
disallows mutable to be set as a default value. This is done
via check for hashability.

See python/cpython#88840.
Note that I am trying to add support for Python 3.11 to hydra. This
seems like a major blocker.

For reproducibility, I used the following command to search for these:
```console
rg ".*: .* = [A-Z].*\(.*\)"
```
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 15, 2022
@Jasha10
Copy link
Collaborator

Jasha10 commented Oct 20, 2022

Hi @skshetry, thanks for working on Python3.11 support!

Using default_factory everywhere sounds like a good option.

The only other option I can think of is to use the decorator @dataclass(frozen=True) instead of @dataclass, which would make the dataclasses immutable. I think that option is sub-optimal because OmegaConf has special support for frozen dataclasses, using them to create read-only DictConfig instances.

We'll eventually need to make the same change for the OmegaConf codebase as for Hydra.

@Jasha10
Copy link
Collaborator

Jasha10 commented Oct 20, 2022

It might be a good idea to create some regression tests like the following (to make sure that OmegaConf and Hydra can still handle old dataclass pattern when python < 3.11):

if python_version < (3, 11):
    @dataclass
    class X:
        y: Y = Y()

@skshetry
Copy link
Contributor Author

It might be a good idea to create some regression tests like the following (to make sure that OmegaConf and Hydra can still handle old dataclass pattern when python < 3.11):

if python_version < (3, 11):
    @dataclass
    class X:
        y: Y = Y()

I am hesitant to test that, as it is dataclasses behaviour that we’ll be testing. Instantiating via default_factory or default assigned value is its responsibility. And remember, this pattern is not going to change for <3.11.

@Jasha10
Copy link
Collaborator

Jasha10 commented Oct 24, 2022

When turning a dataclass into a DictConfig, the omegaconf._utils.get_dataclass_data method inspects the fields of the dataclass and takes a different code path depending on whether a field has a default value or a default_factory.

if python_version < (3, 11):
    @dataclass
    class X:
        y: Y = Y()
    
    # Will OmegaConf choke on dataclasses defined using
    # the old pattern with default values? Currently this works:
    cfg = OmegaConf.create(X)
    # We should test it to make sure it keeps working (for python 3.6-3.10).

I'm worried that future refactoring of omegaconf._utils.get_dataclass_data could make the above code fail.



@mark.skipif(sys.version_info > (3, 11), reason="mutable defaults not allowed in 3.11")
def test_dataclass_with_assigned_mutable_default():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not sure where to add this test. Let me know where it fits.

@Jasha10
Copy link
Collaborator

Jasha10 commented Nov 15, 2022

Thanks @skshetry. I'm working on a python3.11 update for omegaconf. I'm planning to revisit this PR after the OmegaConf support is finished.

Edit: here's a link to the OmegaConf PR: omry/omegaconf#1032

@skshetry
Copy link
Contributor Author

skshetry commented Dec 1, 2022

ping @Jasha10

@Jasha10 Jasha10 changed the title use default_factory for mutable defaults support Python3.11 Dec 2, 2022
@Jasha10
Copy link
Collaborator

Jasha10 commented Dec 2, 2022

Thanks @skshetry

@Jasha10 Jasha10 linked an issue Dec 2, 2022 that may be closed by this pull request
@Jasha10
Copy link
Collaborator

Jasha10 commented Dec 2, 2022

FYI I've removed test_dataclass_with_assigned_mutable_default as it is redundant with the tests in omry/omegaconf#1032.

@Jasha10 Jasha10 marked this pull request as draft December 2, 2022 20:04
@Jasha10 Jasha10 marked this pull request as ready for review December 3, 2022 06:23
@Jasha10 Jasha10 merged commit afde761 into facebookresearch:main Dec 5, 2022
@skshetry skshetry deleted the dataclass-mutable-default branch December 5, 2022 23:33
taylormonacelli added a commit to taylormonacelli/paperdraw that referenced this pull request Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python 3.11 Support
3 participants