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

refactor: bump pydantic to v2 using v1 API #5917

Closed
wants to merge 6 commits into from

Conversation

mjspeck
Copy link
Contributor

@mjspeck mjspeck commented Sep 29, 2023

Related Issues

Proposed Changes:

This PR works as a stopgap solution to updating the pydantic dependency to V2. It uses the v1 API by using pydantic v2's v1 submodule.

How did you test it?

I started unit tests on my computer but they took too long. So per the contributing guidelines I'm waiting for tests to run in the CI pipeline.

Notes for the reviewer

All pydantic imports have just been modified to include .v1.

Checklist

@CLAassistant
Copy link

CLAassistant commented Sep 29, 2023

CLA assistant check
All committers have signed the CLA.

@mjspeck mjspeck changed the title Pydantic v2 interim chore: bump pydantic to v2 using v1 API Sep 29, 2023
@mjspeck mjspeck changed the title chore: bump pydantic to v2 using v1 API refactor: bump pydantic to v2 using v1 API Sep 29, 2023
Copy link
Contributor

@silvanocerza silvanocerza left a comment

Choose a reason for hiding this comment

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

I see lot of unrelated changes in this PR.
Given that it's an important update could you try and keep the changes at a minimum? As much as I like to keep things tidy and in order I think it's best if we don't reformat imports and other files in this case.
It makes it much harder for us to review it too.

@mjspeck
Copy link
Contributor Author

mjspeck commented Sep 29, 2023

I see lot of unrelated changes in this PR. Given that it's an important update could you try and keep the changes at a minimum? As much as I like to keep things tidy and in order I think it's best if we don't reformat imports and other files in this case. It makes it much harder for us to review it too.

Done.

@github-actions github-actions bot removed topic:DX Developer Experience proposal labels Sep 29, 2023
@mjspeck mjspeck force-pushed the pydantic-v2-interim branch 3 times, most recently from 2129917 to fb4b54c Compare September 29, 2023 18:43
@mjspeck mjspeck marked this pull request as ready for review September 29, 2023 18:46
@mjspeck mjspeck requested review from a team as code owners September 29, 2023 18:46
@mjspeck mjspeck requested review from dfokina and silvanocerza and removed request for a team September 29, 2023 18:46
@silvanocerza
Copy link
Contributor

@mjspeck can you revert the formatting for pyproject.toml too please?

@mjspeck
Copy link
Contributor Author

mjspeck commented Sep 29, 2023

@mjspeck can you revert the formatting for pyproject.toml too please?

Done

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 6384187858

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 103 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.006%) to 50.285%

Files with Coverage Reduction New Missed Lines %
pipelines/ray.py 1 21.98%
utils/context_matching.py 1 95.7%
nodes/_json_schema.py 17 87.27%
schema.py 84 68.62%
Totals Coverage Status
Change from base Build 6369977992: -0.006%
Covered Lines: 12514
Relevant Lines: 24886

💛 - Coveralls

@silvanocerza
Copy link
Contributor

@mjspeck seems like the REST API tests are failing for some reason.
I would have expected v1 through v2 to have the same behaviour but it seems it doesn't. 🤔

@mjspeck
Copy link
Contributor Author

mjspeck commented Oct 2, 2023

@mjspeck seems like the REST API tests are failing for some reason. I would have expected v1 through v2 to have the same behaviour but it seems it doesn't. 🤔

Yeah I'm surprised. I'll see what I can do this week.

@mjspeck mjspeck marked this pull request as draft October 2, 2023 22:02
@mjspeck
Copy link
Contributor Author

mjspeck commented Oct 4, 2023

After doing some more testing and digging, it seems that this is an issue with FastAPI. We can either leave this as a draft for now and wait for FastAPI to fix the issue or close it. Just a note, it looks like there's already a PR in there to address this so waiting might not be a bad option.

@silvanocerza
Copy link
Contributor

@mjspeck nice find, let's wait for that issue to be solved. 👍

@Snuggert
Copy link

That issue has been solved in the latest release of fastapi:
tiangolo/fastapi#10344

There is another dependency issue currently blocking the move to fastapi 0.104:
#6119

@masci masci changed the base branch from main to v1.x November 24, 2023 12:06
@masci masci added the 1.x label Nov 24, 2023
@silvanocerza
Copy link
Contributor

Force pushed to fix conflicts.

@silvanocerza
Copy link
Contributor

This still doesn't seem solved. 😕

@silvanocerza
Copy link
Contributor

Rebased to fix conflicts. 🤞

@silvanocerza
Copy link
Contributor

Closing, we won't do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to Pydantic V2
6 participants