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

Upgrade the required python version to python>=3.11 #337

Merged
merged 10 commits into from
Sep 15, 2024

Conversation

westford14
Copy link
Contributor

@westford14 westford14 commented Sep 5, 2024

PR to update the python required version to be >= 3.10 -- this required unpinning the thop dependency as well as changing the ModelEnum.time_distributed extraction of the string to ModelEnum.time_distributed.value.

Closes #310
Closes #192

Copy link

netlify bot commented Sep 5, 2024

Deploy Preview for silly-keller-664934 ready!

Name Link
🔨 Latest commit 20083ec
🔍 Latest deploy log https://app.netlify.com/sites/silly-keller-664934/deploys/66e67223865ed600087c6636
😎 Deploy Preview https://deploy-preview-337--silly-keller-664934.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@westford14
Copy link
Contributor Author

@ejm714 -- let me know if this is the right direction for this PR

@ejm714
Copy link
Collaborator

ejm714 commented Sep 5, 2024

@westford14 this is the right direction. Let's see if we can set the floor at 3.11 instead and then use 3.11 and 3.12 in the tests. You'll need to update the version here for the tests for github actions:

python-version: [3.8, 3.9]

Once that's changed, I'll approve and run the github workflows for this PR

@westford14
Copy link
Contributor Author

@westford14 this is the right direction. Let's see if we can set the floor at 3.11 instead and then use 3.11 and 3.12 in the tests. You'll need to update the version here for the tests for github actions:

python-version: [3.8, 3.9]

Once that's changed, I'll approve and run the github workflows for this PR

@ejm714 -- thanks for the heads up on the github actions, i've now pinned them to 3.11 / 3.12

Copy link
Collaborator

@ejm714 ejm714 left a comment

Choose a reason for hiding this comment

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

Couple small things for user clarity

README.md Outdated Show resolved Hide resolved
docs/docs/index.md Outdated Show resolved Hide resolved
docs/docs/install.md Outdated Show resolved Hide resolved
docs/docs/install.md Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
@westford14
Copy link
Contributor Author

thanks for the comments @ejm714 -- i've updated accordingly

@westford14
Copy link
Contributor Author

@ejm714 -- sorry i fixed the matrix sequence in the github actions file

@ejm714
Copy link
Collaborator

ejm714 commented Sep 9, 2024

Thanks. That got past the initial hurdle so we actually get to run the tests on this PR however they're failing due to some installs for the densepose tests. You should be able to run the tests locally with make test for faster debugging. There's some details here: https://zamba.drivendata.org/docs/stable/contribute/#running-the-zamba-test-suite

@westford14
Copy link
Contributor Author

westford14 commented Sep 10, 2024

looking at the pytorch issues, only python 3.12 support is only available in pytorch >== https://github.com/pytorch/pytorch/releases/tag/v2.4.0 which breaks the densepose library dependency, i guess the best path forward here is to pin to python 3.11

@ejm714 i've updated the code accordingly

pyproject.toml Outdated Show resolved Hide resolved
@westford14
Copy link
Contributor Author

@ejm714 -- pinned to 3.11

@ejm714 ejm714 changed the title Upgrade the required python version to python>=3.10 Upgrade the required python version to python 3.11 Sep 11, 2024
Copy link

codecov bot commented Sep 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.3%. Comparing base (498bca1) to head (20083ec).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #337     +/-   ##
========================================
- Coverage    87.4%   87.3%   -0.1%     
========================================
  Files          26      26             
  Lines        2191    2191             
========================================
- Hits         1915    1913      -2     
- Misses        276     278      +2     
Files with missing lines Coverage Δ
zamba/models/config.py 97.5% <100.0%> (ø)

... and 1 file with indirect coverage changes

@westford14
Copy link
Contributor Author

hey @ejm714 -- any changes I need to make to this PR? looks like all the tests would have passed (the macOS 3.11 was cancelled)

@westford14
Copy link
Contributor Author

wow codecov is really ratelimiting hard here

@ejm714
Copy link
Collaborator

ejm714 commented Sep 12, 2024

hey @ejm714 -- any changes I need to make to this PR? looks like all the tests would have passed (the macOS 3.11 was cancelled)

It's unclear. I want to make sure all tests are passing on all operating systems but as you noted, codecov is really ratelimiting and I haven't had a chance to look into why/how to fix that. If you want to speed merging along, then you could look into that. Otherwise, feel free to put this down until I request anything specifically

@westford14
Copy link
Contributor Author

hey @ejm714 -- any changes I need to make to this PR? looks like all the tests would have passed (the macOS 3.11 was cancelled)

It's unclear. I want to make sure all tests are passing on all operating systems but as you noted, codecov is really ratelimiting and I haven't had a chance to look into why/how to fix that. If you want to speed merging along, then you could look into that. Otherwise, feel free to pull this down until I request anything specifically

yea i'll take a look at the ratelimit thing there -- though it does seem like the tests are passing

@westford14
Copy link
Contributor Author

@ejm714 -- was doing some research into the codecov issue and it seems to be a pretty common issue for forked repos, but in this thread someone mentioned that upgrading to the v4 github action resolved it (codecov/feedback#358 (comment))

figured this was worth a shot before fully re-architecting this

Copy link
Collaborator

@ejm714 ejm714 left a comment

Choose a reason for hiding this comment

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

Looks like we can actually support python 3.12

I was looking into this more and we need to remove the ceiling on torch in the densepose installs. The minimum torch version is 2.2.0 which is why we were getting the failures with python 3.12 and the densepose install. With the latest torch, the densepose tests are passing locally with python 3.12.

Thanks for the codecov update -- that seems to have resolved the timeouts. Once you've made the changes to support 3.11 and 3.12, I'll run the tests again and we should be good to go

@westford14
Copy link
Contributor Author

Looks like we can actually support python 3.12

I was looking into this more and we need to remove the ceiling on torch in the densepose installs. The minimum torch version is 2.2.0 which is why we were getting the failures with python 3.12 and the densepose install. With the latest torch, the densepose tests are passing locally with python 3.12.

Thanks for the codecov update -- that seems to have resolved the timeouts. Once you've made the changes to support 3.11 and 3.12, I'll run the tests again and we should be good to go

awesome -- i'll do that and let you know when it's ready to rerun the tests

@westford14
Copy link
Contributor Author

@ejm714 -- just removed the cap on the densepose pytorch version, tests passed locally for me in both 3.11 and 3.12

pyproject.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@ejm714 ejm714 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@ejm714 ejm714 changed the title Upgrade the required python version to python 3.11 Upgrade the required python version to python>=3.11 Sep 15, 2024
@ejm714 ejm714 merged commit 2ba2dc6 into drivendataorg:master Sep 15, 2024
15 checks passed
@westford14 westford14 deleted the westford14/python-version branch September 16, 2024 05:44
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.

Update python versions Unpin thop once onnx import is resolved
2 participants