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

Patch jsonargparse for Python >= 3.12.8 #20479

Merged
merged 4 commits into from
Dec 9, 2024
Merged

Patch jsonargparse for Python >= 3.12.8 #20479

merged 4 commits into from
Dec 9, 2024

Conversation

lantiga
Copy link
Collaborator

@lantiga lantiga commented Dec 9, 2024 •
edited
Loading

What does this PR do?

This PR is a temporary fix to omni-us/jsonargparse#641
so that users are not impacted even prior to the upstream fix being released.

This undoes pinning introduced in #20476

📚 Documentation preview 📚: https://pytorch-lightning--20479.org.readthedocs.build/en/20479/

@github-actions github-actions bot added ci Continuous Integration pl Generic label for PyTorch Lightning package labels Dec 9, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 9, 2024 •
edited
Loading

⛈️ Required checks status: Has failure 🔴

Warning
This job will need to be re-run to merge your PR. If you do not have write access to the repository, you can ask Lightning-AI/lai-frameworks to re-run it. If you push a new commit, all of CI will re-trigger.

Groups summary

🟢 pytorch_lightning: Tests workflow
Check ID Status
pl-cpu (macOS-14, lightning, 3.9, 2.1, oldest) success ✅
pl-cpu (macOS-14, lightning, 3.10, 2.1) success ✅
pl-cpu (macOS-14, lightning, 3.11, 2.2.2) success ✅
pl-cpu (macOS-14, lightning, 3.11, 2.3) success ✅
pl-cpu (macOS-14, lightning, 3.12, 2.4.1) success ✅
pl-cpu (macOS-14, lightning, 3.12, 2.5.1) success ✅
pl-cpu (ubuntu-20.04, lightning, 3.9, 2.1, oldest) success ✅
pl-cpu (ubuntu-20.04, lightning, 3.10, 2.1) success ✅
pl-cpu (ubuntu-20.04, lightning, 3.11, 2.2.2) success ✅
pl-cpu (ubuntu-20.04, lightning, 3.11, 2.3) success ✅
pl-cpu (ubuntu-22.04, lightning, 3.12, 2.4.1) success ✅
pl-cpu (ubuntu-22.04, lightning, 3.12, 2.5.1) success ✅
pl-cpu (windows-2022, lightning, 3.9, 2.1, oldest) success ✅
pl-cpu (windows-2022, lightning, 3.10, 2.1) success ✅
pl-cpu (windows-2022, lightning, 3.11, 2.2.2) success ✅
pl-cpu (windows-2022, lightning, 3.11, 2.3) success ✅
pl-cpu (windows-2022, lightning, 3.12, 2.4.1) success ✅
pl-cpu (windows-2022, lightning, 3.12, 2.5.1) success ✅
pl-cpu (macOS-14, pytorch, 3.9, 2.1) success ✅
pl-cpu (ubuntu-20.04, pytorch, 3.9, 2.1) success ✅
pl-cpu (windows-2022, pytorch, 3.9, 2.1) success ✅
pl-cpu (macOS-14, pytorch, 3.12, 2.5.1) success ✅
pl-cpu (ubuntu-22.04, pytorch, 3.12, 2.5.1) success ✅
pl-cpu (windows-2022, pytorch, 3.12, 2.5.1) success ✅

These checks are required after the changes to .actions/assistant.py, .github/workflows/ci-tests-pytorch.yml, src/lightning/pytorch/cli.py, tests/tests_pytorch/checkpointing/test_model_checkpoint.py.

🟢 pytorch_lightning: Azure GPU
Check ID Status
pytorch-lightning (GPUs) (testing Lightning | latest) success ✅
pytorch-lightning (GPUs) (testing PyTorch | latest) success ✅

These checks are required after the changes to .actions/assistant.py, src/lightning/pytorch/cli.py, tests/tests_pytorch/checkpointing/test_model_checkpoint.py.

🟢 pytorch_lightning: Benchmarks
Check ID Status
lightning.Benchmarks success ✅

These checks are required after the changes to src/lightning/pytorch/cli.py, tests/parity_fabric/test_parity_ddp.py.

🟢 pytorch_lightning: Docs
Check ID Status
docs-make (pytorch, doctest) success ✅
docs-make (pytorch, html) success ✅

These checks are required after the changes to src/lightning/pytorch/cli.py, .actions/assistant.py.

🔴 pytorch_lightning: Docker
Check ID Status
build-cuda (3.10, 2.1.2, 12.1.0) success ✅
build-cuda (3.11, 2.2.2, 12.1.0) success ✅
build-cuda (3.11, 2.3.1, 12.1.0) success ✅
build-cuda (3.11, 2.4.1, 12.1.0) success ✅
build-cuda (3.12, 2.5.1, 12.1.0) failure ❌
build-pl (3.10, 2.1, 12.1.0) success ✅
build-pl (3.11, 2.2, 12.1.0) success ✅
build-pl (3.11, 2.3, 12.1.0) success ✅
build-pl (3.11, 2.4, 12.1.0) success ✅
build-pl (3.12, 2.5, 12.1.0) success ✅

These checks are required after the changes to .actions/assistant.py.

🔴 lightning_fabric: CPU workflow
Check ID Status
fabric-cpu (macOS-14, lightning, 3.9, 2.1, oldest) success ✅
fabric-cpu (macOS-14, lightning, 3.10, 2.1) success ✅
fabric-cpu (macOS-14, lightning, 3.11, 2.2.2) success ✅
fabric-cpu (macOS-14, lightning, 3.11, 2.3) success ✅
fabric-cpu (macOS-14, lightning, 3.12, 2.4.1) success ✅
fabric-cpu (macOS-14, lightning, 3.12, 2.5.1) success ✅
fabric-cpu (ubuntu-20.04, lightning, 3.9, 2.1, oldest) success ✅
fabric-cpu (ubuntu-20.04, lightning, 3.10, 2.1) success ✅
fabric-cpu (ubuntu-20.04, lightning, 3.11, 2.2.2) success ✅
fabric-cpu (ubuntu-20.04, lightning, 3.11, 2.3) success ✅
fabric-cpu (ubuntu-22.04, lightning, 3.12, 2.4.1) success ✅
fabric-cpu (ubuntu-22.04, lightning, 3.12, 2.5.1) failure ❌
fabric-cpu (windows-2022, lightning, 3.9, 2.1, oldest) success ✅
fabric-cpu (windows-2022, lightning, 3.10, 2.1) success ✅
fabric-cpu (windows-2022, lightning, 3.11, 2.2.2) success ✅
fabric-cpu (windows-2022, lightning, 3.11, 2.3) success ✅
fabric-cpu (windows-2022, lightning, 3.12, 2.4.1) success ✅
fabric-cpu (windows-2022, lightning, 3.12, 2.5.1) success ✅
fabric-cpu (macOS-14, fabric, 3.9, 2.1) success ✅
fabric-cpu (ubuntu-20.04, fabric, 3.9, 2.1) success ✅
fabric-cpu (windows-2022, fabric, 3.9, 2.1) success ✅
fabric-cpu (macOS-14, fabric, 3.12, 2.5.1) success ✅
fabric-cpu (ubuntu-22.04, fabric, 3.12, 2.5.1) failure ❌
fabric-cpu (windows-2022, fabric, 3.12, 2.5.1) success ✅

These checks are required after the changes to .actions/assistant.py, .github/workflows/ci-tests-fabric.yml.

🟢 lightning_fabric: Azure GPU
Check ID Status
lightning-fabric (GPUs) (testing Fabric | latest) success ✅
lightning-fabric (GPUs) (testing Lightning | latest) success ✅

These checks are required after the changes to .actions/assistant.py, examples/fabric/tensor_parallel/train.py.

🟢 mypy
Check ID Status
mypy success ✅

These checks are required after the changes to .actions/assistant.py, src/lightning/pytorch/cli.py.

🟢 install
Check ID Status
install-pkg (ubuntu-22.04, fabric, 3.9) success ✅
install-pkg (ubuntu-22.04, fabric, 3.11) success ✅
install-pkg (ubuntu-22.04, pytorch, 3.9) success ✅
install-pkg (ubuntu-22.04, pytorch, 3.11) success ✅
install-pkg (ubuntu-22.04, lightning, 3.9) success ✅
install-pkg (ubuntu-22.04, lightning, 3.11) success ✅
install-pkg (ubuntu-22.04, notset, 3.9) success ✅
install-pkg (ubuntu-22.04, notset, 3.11) success ✅
install-pkg (macOS-14, fabric, 3.9) success ✅
install-pkg (macOS-14, fabric, 3.11) success ✅
install-pkg (macOS-14, pytorch, 3.9) success ✅
install-pkg (macOS-14, pytorch, 3.11) success ✅
install-pkg (macOS-14, lightning, 3.9) success ✅
install-pkg (macOS-14, lightning, 3.11) success ✅
install-pkg (macOS-14, notset, 3.9) success ✅
install-pkg (macOS-14, notset, 3.11) success ✅
install-pkg (windows-2022, fabric, 3.9) success ✅
install-pkg (windows-2022, fabric, 3.11) success ✅
install-pkg (windows-2022, pytorch, 3.9) success ✅
install-pkg (windows-2022, pytorch, 3.11) success ✅
install-pkg (windows-2022, lightning, 3.9) success ✅
install-pkg (windows-2022, lightning, 3.11) success ✅
install-pkg (windows-2022, notset, 3.9) success ✅
install-pkg (windows-2022, notset, 3.11) success ✅

These checks are required after the changes to .actions/assistant.py, src/lightning/pytorch/cli.py.

Thank you for your contribution! 💜

Note
This comment is automatically generated and updates for 60 minutes every 180 seconds. If you have any other questions, contact carmocca for help.

@github-actions github-actions bot added the fabric lightning.fabric.Fabric label Dec 9, 2024
@codecov Codecov
Copy link

codecov bot commented Dec 9, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88%. Comparing base (c09fc66) to head (918f2c6).
Report is 1 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master   #20479   +/-   ##
=======================================
- Coverage      88%      88%   -0%     
=======================================
  Files         267      267           
  Lines       23276    23284    +8     
=======================================
+ Hits        20383    20387    +4     
- Misses       2893     2897    +4

@lantiga lantiga force-pushed the luca/argparse-patch branch 2 times, most recently from 4069d3b to ce4763d Compare December 9, 2024 12:18
@lantiga lantiga force-pushed the luca/argparse-patch branch 2 times, most recently from 22e6412 to 47c36ad Compare December 9, 2024 12:31
@lantiga lantiga force-pushed the luca/argparse-patch branch from aa7d781 to 68677d3 Compare December 9, 2024 12:40
@lantiga lantiga force-pushed the luca/argparse-patch branch from 788d419 to 48b944c Compare December 9, 2024 12:57
@lantiga lantiga force-pushed the luca/argparse-patch branch from ee13a3e to 78ba069 Compare December 9, 2024 13:00
@lantiga lantiga force-pushed the luca/argparse-patch branch from 5181443 to 918f2c6 Compare December 9, 2024 13:34
@lantiga lantiga merged commit 38971a0 into master Dec 9, 2024
71 of 86 checks passed
@adamjstewart
Copy link
Contributor

Seems like this patch is actually causing things to break now that jsonargparse is updated? TorchGeo CI is not happy with the 2.5.0 release: https://github.com/microsoft/torchgeo/actions/runs/12438070046/job/34729248038?pr=2484

@adamjstewart adamjstewart mentioned this pull request Dec 20, 2024
Closed
@lantiga
Copy link
Collaborator Author

lantiga commented Dec 20, 2024

hey @adamjstewart looking into this, will issue a post release that fixes this in the next few minutes

@lantiga
Copy link
Collaborator Author

lantiga commented Dec 20, 2024

ok so should we bump jsonargparse >= 4.35.0, or apply the patch conditionally? I'm thinking that we do the latter since jsonargparse is an extra so pinning it might not have an effect in practice. wdyt @adamjstewart ?

@adamjstewart
Copy link
Contributor

My vote would be to remove the patch and allow users to use any version of jsonargparse. This is a bug in jsonargparse, not a bug in lightning. Let users complain to jsonargparse, and then let jsonargparse tell them to upgrade to newer jsonargparse if they want to use newer Python. Users on older Python can still use older jsonargparse with newer lightning, lightning has no problems with older jsonargparse.

lantiga added a commit that referenced this pull request Dec 20, 2024
@lantiga
Revert "Patch jsonargparse for Python >= 3.12.8 (#20479)"
bfc2391 
This reverts commit 38971a0.
@lantiga lantiga mentioned this pull request Dec 20, 2024
Merged
lantiga added a commit that referenced this pull request Dec 20, 2024
@lantiga
Revert "Patch jsonargparse for Python >= 3.12.8 (#20479)"
b8dc4b0 
This reverts commit 38971a0.
lantiga added a commit that referenced this pull request Dec 21, 2024
@lantiga
Revert "Patch jsonargparse for Python >= 3.12.8" (#20513)
4337f7f 
* Revert "Patch jsonargparse for Python >= 3.12.8 (#20479)"

This reverts commit 38971a0.

* Update upper bound for jsonargparse

* Ensure jsonargparse is up to date in containers

* Skip pt 2.1 tests incompatible with new jsonargparse
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ethanwharris ethanwharris ethanwharris approved these changes

@Borda Borda Awaiting requested review from Borda Borda is a code owner

@tchaton tchaton Awaiting requested review from tchaton tchaton is a code owner

@justusschock justusschock Awaiting requested review from justusschock justusschock is a code owner

Assignees
No one assigned
Labels
ci Continuous Integration fabric lightning.fabric.Fabric pl Generic label for PyTorch Lightning package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lantiga @adamjstewart @ethanwharris