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

Fixes for non-stdlib dataclass-like types #480

Merged
merged 44 commits into from
Apr 17, 2024

Conversation

nkrishnaswami
Copy link
Contributor

What does this PR do?

This PR includes a few small, related fixes for non-stdlib (possibly annotated) dataclasses and 3.12-style type aliases:

  • Exclude non-init fields from attrs class instantiation.
  • Exclude non-init fields from Pydantic 2 dataclass instantiation.
  • Fix initialization for Pydantic 2 dataclasses with default values/factories.
  • Look at underlying type for Annotated types where needed.
  • Look at target for new TypeAliasType where needed. (This and the prior are commonly used with FastAPI)
  • Tests showing the above were broken and are now fixed.

Before submitting

  • Did you read the contributing guideline?
  • Did you update the documentation? (readme and public docstrings)
  • Did you write unit tests such that there is 100% coverage on related code? (required for bug fixes and new features)
  • Did you verify that new and existing tests pass locally?
  • Did you make sure that all changes preserve backward compatibility?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

jsonargparse/_common.py Fixed Show fixed Hide fixed
jsonargparse_tests/test_dataclass_like.py Fixed Show fixed Hide fixed
jsonargparse/_parameter_resolvers.py Fixed Show fixed Hide fixed
Copy link
Contributor Author

@nkrishnaswami nkrishnaswami left a comment

Choose a reason for hiding this comment

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

A few annotations.

jsonargparse/_optionals.py Show resolved Hide resolved
jsonargparse/_parameter_resolvers.py Outdated Show resolved Hide resolved
jsonargparse/_parameter_resolvers.py Outdated Show resolved Hide resolved
jsonargparse/_parameter_resolvers.py Show resolved Hide resolved
jsonargparse/_parameter_resolvers.py Outdated Show resolved Hide resolved
@nkrishnaswami nkrishnaswami force-pushed the fixes-for-dataclass-likes branch from c2bd19c to 87c8493 Compare March 28, 2024 15:48
@nkrishnaswami nkrishnaswami force-pushed the fixes-for-dataclass-likes branch from 1507ada to 8cc32d3 Compare March 28, 2024 17:44
@nkrishnaswami
Copy link
Contributor Author

Hmm, seeing errors on pydantic < 2.6; how far back do you need to support?

@nkrishnaswami
Copy link
Contributor Author

nkrishnaswami commented Mar 29, 2024

Hmm, seeing errors on pydantic < 2.6; how far back do you need to support?

Sorted this out for 2.0+, but I am not sure how to set up the Pydantic v1 test environment

Copy link
Member

@mauvilsa mauvilsa left a comment

Choose a reason for hiding this comment

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

@nkrishnaswami thank you for contributing! I haven't had time to look at this in detail, so I will comment more later. For the moment, I see that there is a test failing in test-py310-pydantic-v1. The changes introduced in pydantic v2 are big, which means that many people will still be using v1 for a long time. Please make sure that the test passes because there is no plan yet to drop support for pydantic v1.

@nkrishnaswami
Copy link
Contributor Author

nkrishnaswami commented Mar 31, 2024

For the moment, I see that there is a test failing in test-py310-pydantic-v1. ... Please make sure that the test passes because there is no plan yet to drop support for pydantic v1.

@mauvilsa Gotcha. I was having trouble running the tests in my virtual env, but I found the commands to use in the circleci config; I'll get this fixed shortly!

@nkrishnaswami
Copy link
Contributor Author

@mauvilsa got the tests and CI passing! Thanks!

@nkrishnaswami nkrishnaswami requested a review from mauvilsa April 2, 2024 18:12
Copy link
Member

@mauvilsa mauvilsa left a comment

Choose a reason for hiding this comment

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

It is looking good. But some work is needed. See the comments.

jsonargparse_tests/test_dataclass_like.py Outdated Show resolved Hide resolved
jsonargparse_tests/test_dataclass_like.py Outdated Show resolved Hide resolved
jsonargparse_tests/test_dataclass_like.py Outdated Show resolved Hide resolved
jsonargparse_tests/test_dataclass_like.py Outdated Show resolved Hide resolved
jsonargparse_tests/test_dataclass_like.py Outdated Show resolved Hide resolved
jsonargparse_tests/test_dataclass_like.py Outdated Show resolved Hide resolved
jsonargparse_tests/test_dataclass_like.py Outdated Show resolved Hide resolved
jsonargparse_tests/test_dataclass_like.py Outdated Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
jsonargparse/_parameter_resolvers.py Outdated Show resolved Hide resolved
@nkrishnaswami
Copy link
Contributor Author

It is looking good. But some work is needed. See the comments.

Thanks for the feedback! I believe I have address these in the latest commit(s).

@mauvilsa mauvilsa added bug Something isn't working enhancement New feature or request labels Apr 12, 2024
@nkrishnaswami
Copy link
Contributor Author

nkrishnaswami commented Apr 12, 2024

That last set of fixes broke nested dataclasses on 3.12, and init=False on 3.7 for Pydantic dataclasses. (The version dependency is a surprise). I'll take a look at those tonight (UTC-5)

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@mauvilsa mauvilsa merged commit 3069fbf into omni-us:main Apr 17, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants