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.

Sign up for GitHub

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

Jump to bottom

refactor: bump pydantic to v2 using v1 API #5917

Closed
wants to merge 6 commits into from
Closed

refactor: bump pydantic to v2 using v1 API #5917

wants to merge 6 commits into from

Conversation

mjspeck
Copy link
Contributor

@mjspeck mjspeck commented Sep 29, 2023 •
edited
Loading

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 •
edited
Loading

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
silvanocerza
silvanocerza reviewed Sep 29, 2023
View reviewed changes
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 •
edited
Loading

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.

@mjspeck mjspeck force-pushed the pydantic-v2-interim branch from dd225ed to 41fa2d6 Compare September 29, 2023 18:04
@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 •
edited
Loading

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:
fastapi/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. 😕

@mjspeck @silvanocerza
refactor: update pydantic dependency to v2
8271604 
This doesn't update the code to use the pydantic v2 API, it uses
pydantic's v1 submodule as an intermediate step to allow updating the
dependency.
@mjspeck @silvanocerza
chore: reformat markdown files to follow linting rules
7539f06
@mjspeck @silvanocerza
chore: added releasenotes for pydantic bump
e940b30
@mjspeck @silvanocerza
Revert "chore: reformat markdown files to follow linting rules"
7539401 
This reverts commit ee8337ec5a4bf6ded08b04804f50973f3113468f.
@mjspeck @silvanocerza
chore: revert import sorting
89d6915
@mjspeck @silvanocerza
chore: undid formatting in pyproject.toml
60e3e5f
@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
Reviewers

@dfokina dfokina Awaiting requested review from dfokina dfokina is a code owner automatically assigned from deepset-ai/documentation

@silvanocerza silvanocerza Awaiting requested review from silvanocerza silvanocerza is a code owner automatically assigned from deepset-ai/open-source-engineering

Assignees

@silvanocerza silvanocerza

Labels
1.x topic:build/distribution topic:dependencies topic:document_store topic:elasticsearch topic:opensearch topic:pipeline topic:rest_api topic:tests type:documentation Improvements on the docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate to Pydantic V2
6 participants
@mjspeck @CLAassistant @silvanocerza @coveralls @Snuggert @masci