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

Update requirements.txt to update pydantic minimum version and put py.typed in MANIFEST.in #1767

Merged
merged 6 commits into from
Sep 5, 2023

Conversation

Andrew-S-Rosen
Copy link
Contributor

@Andrew-S-Rosen Andrew-S-Rosen commented Sep 2, 2023

  • [] I have added the tests to cover my changes.
  • I have updated the documentation and CHANGELOG accordingly.
  • I have read the CONTRIBUTING document.

Closes #1765. Essentially, the existing minimum version for Pydantic causes the UI not to render appropriately. Pydantic > 2 is needed now for Covalent. As such, I have set the minimum version to 2.1.1. I chose 2.1.1 intentionally because: a) I tested it and it works; b) FastAPI strictly does not support 2.0.0, 2.0.1, or 2.1.0 so we can't use 2.0 through 2.1.0 anyway.

@Andrew-S-Rosen Andrew-S-Rosen requested a review from a team as a code owner September 2, 2023 19:07
@codecov
Copy link

codecov bot commented Sep 2, 2023

Codecov Report

Merging #1767 (a8f4646) into develop (c06893b) will not change coverage.
Report is 2 commits behind head on develop.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1767   +/-   ##
========================================
  Coverage    88.21%   88.21%           
========================================
  Files          191      191           
  Lines         8222     8222           
  Branches       151      151           
========================================
  Hits          7253     7253           
  Misses         864      864           
  Partials       105      105           
Flag Coverage Δ
Dispatcher 86.71% <ø> (ø)
Functional_Tests 56.22% <ø> (ø)
SDK 91.49% <ø> (ø)
UI_Backend 91.15% <ø> (ø)
UI_Frontend 72.64% <ø> (ø)

@santoshkumarradha
Copy link
Member

@arosen93 thanks a lot for identifying the exact cause ! This could have been quite nasty to debug sans UI logs !

@kessler-frost can you take a look at this please 🙏🏻

@kessler-frost
Copy link
Member

Upgrading Pydantic might break certain things I believe (hopefully I'm wrong), but let's see what happens. This PR looks gtg from my side.

@Andrew-S-Rosen
Copy link
Contributor Author

🙏

@Andrew-S-Rosen Andrew-S-Rosen changed the title Update requirements.txt to fix pydantic version minimum Update requirements.txt to update pydantic minimum version and put py.typed in MANIFEST.in Sep 4, 2023
@Andrew-S-Rosen
Copy link
Contributor Author

Andrew-S-Rosen commented Sep 4, 2023

@kessler-frost: I added the py.typed file to MANIFEST.in as well here. I also don't think the Pydantic version change will be an issue since the test suite likely was already using Pydantic > 2.

@wjcunningham7
Copy link
Member

Just a heads up to the team Pydantic 2 has been known to not work on AWS Lambda, so keep an eye out for this during the next round of QA
https://twitter.com/pydantic/status/1678298418983124992
https://stackoverflow.com/questions/76650856/no-module-named-pydantic-core-pydantic-core-in-aws-lambda-though-library-is-i

CC @AlejandroEsquivel

@kessler-frost kessler-frost merged commit 55a1ea9 into AgnostiqHQ:develop Sep 5, 2023
11 checks passed
@Andrew-S-Rosen Andrew-S-Rosen deleted the pydantic branch September 5, 2023 15:16
@Prasy12
Copy link
Collaborator

Prasy12 commented Sep 6, 2023

There also is a case where pydantic 2 has been supported only for fastapi versions > 100. So we might need to tweak the fastapi limits in the requirements as well. (Currently <100 also can be installed)

Also when we install fastapi, pydantic gets installed automatically with a fastapi compatible version. So we might need to set both the fastapi and pydantic version correctly to avoid any failures.

cc : @kessler-frost @arosen93 @wjcunningham7

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.

Minimum version of Pydantic needs to be updated in setup.py to allow UI to work properly
5 participants