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

Blocking pydantic versions 2.9.0/2.9.1 in [dev] #1068

Merged
merged 4 commits into from
Sep 23, 2024

Conversation

dbirman
Copy link
Member

@dbirman dbirman commented Sep 19, 2024

This PR increases the pydantic minimum to 2.9.2 sets two versions of pydantic to ignore now that 2.9.2 is released, only on the [dev] dependencies

In versions 2.9.0 and 2.9.1 errors are thrown when model_construct() is used on models with Unions.

Closes #1058

@dbirman dbirman linked an issue Sep 19, 2024 that may be closed by this pull request
@dbirman dbirman changed the title Un-cap pydantic version Increasing pydantic minimum to 2.9.2 Sep 19, 2024
@jtyoung84
Copy link
Collaborator

In your comment, if you add Closes #1058, then it should automatically close the issue when the PR is merged.

@jtyoung84 jtyoung84 requested a review from dyf September 19, 2024 01:37
@dbirman
Copy link
Member Author

dbirman commented Sep 19, 2024

In your comment, if you add Closes #1058, then it should automatically close the issue when the PR is merged.

Huh weird, it auto-links when you generate branches from an issue but I think because I changed the name it un-linked it?

@jtyoung84
Copy link
Collaborator

In your comment, if you add Closes #1058, then it should automatically close the issue when the PR is merged.

Huh weird, it auto-links when you generate branches from an issue but I think because I changed the name it un-linked it?

Oh, maybe it will auto close the issue then. I usually create a branch locally.

@bruno-f-cruz
Copy link
Collaborator

My two cents: I don't think this pin is warranted. Sure 2.9.1 will not work with ADS 1.0.0 but this is largely (well totally if you consider the users) due to a bug in ADS that should be fixed anyway. I would suggest leaving the pin as is as it simplifies the bootstrap of dependencies for consumers. Moreover, the chances that someone pins pydantic on their side to 2.9.1 is very very small since the 2.9.2 patch was released a few days after 2.9.1 anyway.

@dbirman
Copy link
Member Author

dbirman commented Sep 19, 2024

I think != actually does work in the pyproject.toml file? Changed it to specify the versions to ignore. I guess it just won't resolve conflicts, but I'm not sure we care about that since people can at least see that we're specifying versions to skip.

@bruno-f-cruz
Copy link
Collaborator

That's even better!

However I still think that this only makes sense if people want to run tests. Once the next version of ads gets released the other problems should be fixed anyway and the pin stops making sense since it can't be applied retroactively.

I guess we can compromise and apply the specific version rejection until the unit tests get fixed.

@jtyoung84
Copy link
Collaborator

Is this an issue then with just the unit tests or is it something we'll have to modify the source code for? If it's just the unit tests, we should just update them if it's a relatively quick fix.

@dbirman
Copy link
Member Author

dbirman commented Sep 19, 2024

It would be a lot of work to fix the tests I think, when I tried the only way I could do it was by generating full definitions for the objects instead of using model_construct(). Arguably this is better since as we've learned the behavior of model_construct is unreliable, but it would take a while to update the tests.

And yes it's true that the version restrictions here are specific to running tests, so we could drop this PR now and tell everybody to update pydantic to 2.9.2.

@jtyoung84
Copy link
Collaborator

@dyf Do have an opinion? A lot of this is also related to the core.metadata module, which I'm hoping will be updated soon. Maybe we can resolve the unit tests as part of that project?

@dbirman
Copy link
Member Author

dbirman commented Sep 21, 2024

Okay one more time on this -- I moved the two pydantic versions that cause issues with running tests into the [dev] requirements. Should be fine to merge this now and then open a separate ticket for removing model_construct() in as many places as possible.

@dbirman dbirman changed the title Increasing pydantic minimum to 2.9.2 Blocking pydantic versions 2.9.0/2.9.1 in [dev] Sep 23, 2024
@dbirman dbirman added this pull request to the merge queue Sep 23, 2024
Merged via the queue into dev with commit a965558 Sep 23, 2024
5 checks passed
@dbirman dbirman deleted the 1058-breaking-changes-in-pydantic-29-part-2 branch September 23, 2024 20:59
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.

Breaking changes in pydantic 2.9 (part 2)
4 participants