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

Upgrade to Python 3.11 #22

Merged
merged 6 commits into from
Aug 2, 2023
Merged

Upgrade to Python 3.11 #22

merged 6 commits into from
Aug 2, 2023

Conversation

bnewm0609
Copy link

This PR addresses an update to the dataclasses module in python 3.11, which prevents adding mutable fields to dataclasses by default. There were a few places where this appears.

First, the springs.cli.Flag dataclass. This change was addressed by implementing a __hash__ method.

Second, in the configs in tests/fixtures/configs. Here, the PR makes dataclasses hashable by passing unsafe_hash=True to the dataclass wrapper function.

There are other approaches to this outlined here:

  • using default_factory instead of directly instantiating the. This led to an error within omegaconf: Dict[Any, Any] cannot be assigned type Field. (or something like that)
  • using the sp.fobj method to wrap the . This led to an omegaconf error cannot pickle 'mappingproxy' object (might be related to deepcopying a dataclass)
  • explicitly defining __hash__ methods on all of the configs in tests/fixtures/configs. This seemed like too much work compared to just using unsafe_hash=True.

@@ -20,4 +20,4 @@ def test_sanitize(self):
self.assertEqual(c.c, "___fooo___")
self.assertEqual(c.d, c.b)
self.assertEqual(c.e, "_f")
self.assertEqual(c.f, " fooo")
self.assertEqual(c.f, "fooo")
Copy link
Author

Choose a reason for hiding this comment

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

This change is unrelated to dataclass issues. It looks like the sanatize_filename function behavior is different from expected.

@soldni
Copy link
Owner

soldni commented Aug 2, 2023

Thank you @bnewm0609! I hadn't researched options myself, so this was very helpful. I decided to go with the default_factory= option as it is (a) in line with what omegaconf library recommends, and (b) safer.

@soldni soldni merged commit 24e58dd into soldni:main Aug 2, 2023
5 checks passed
@soldni soldni mentioned this pull request Aug 2, 2023
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 this pull request may close these issues.

None yet

2 participants