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

Apply pyupgrade fixes for Python 3.9+ syntax #8150

Merged
merged 6 commits into from
Oct 24, 2024

Conversation

jamesobutler
Copy link
Contributor

Description

Included is a commit here applying pyupgrade changes for Python 3.8+ syntax. This should have been included in 104a360 when Python 3.7 support was dropped.

Also included is a commit here apply pyupgrade changes for Python 3.9+ syntax. This should have been included in 14b086b when Python 3.8 support was dropped.

I've also run the pre-commit autoupdate command to use the latest versions of the various pre-commit hook repos. It appears that pre-commit.ci bot has not been doing this on the quarterly schedule as expected? It appears the last time it submitted the PR to update the pre-commit hook repos was back in #6286 from April 2023.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).

@jamesobutler jamesobutler force-pushed the jamesobutler-pyupgrade-fixes branch 2 times, most recently from 43a4af4 to d14d662 Compare October 15, 2024 22:03
@KumoLiu
Copy link
Contributor

KumoLiu commented Oct 16, 2024

Hi @jamesobutler, thanks for the PR.
Could you please help take a look at the ci error: https://github.com/Project-MONAI/MONAI/actions/runs/11355141675/job/31583909844?pr=8150#step:8:13833

@KumoLiu KumoLiu requested review from ericspod and Nic-Ma October 16, 2024 03:52
@ericspod
Copy link
Member

It looks good to me, the errors popping up are a little weird and maybe something incompatible buried in Pytorch.

@jamesobutler jamesobutler force-pushed the jamesobutler-pyupgrade-fixes branch 2 times, most recently from 0a2a24b to 9b1d232 Compare October 16, 2024 14:01
@jamesobutler
Copy link
Contributor Author

jamesobutler commented Oct 16, 2024

I've attempted to avoid the typing related changes, but yeah I'm not sure what is going here with the code not being able to handle the post-upgrade. premerge-min / min-dep-pytorch (latest) check is successful, but the older pytorch status checks are not.

@jamesobutler jamesobutler force-pushed the jamesobutler-pyupgrade-fixes branch 2 times, most recently from c741d57 to e87d3f8 Compare October 23, 2024 20:55
This should have been included in Project-MONAI@104a360 when Python 3.7 support was dropped.

Changes were automatically applied by running:
pre_commit run --all-files

Signed-off-by: James Butler <[email protected]>
@jamesobutler jamesobutler force-pushed the jamesobutler-pyupgrade-fixes branch 5 times, most recently from b200633 to 1f8644d Compare October 23, 2024 23:22
@jamesobutler
Copy link
Contributor Author

I have rebased this branch against latest dev so that mass bulk changes would apply to latest code.

I don't generally like large file exclusions, but I've excluded some files from the pyupgrade pre-commit action and as a result monai tests have begun to pass again. This PR now represents an improvement and removal of the pyupgrade exceptions can come in the future along with more debugging. Maybe the issue will resolve itself upon dropping support for Python 3.9 and the typing support that was available in that version.

@jamesobutler jamesobutler requested a review from KumoLiu October 24, 2024 00:30
.pre-commit-config.yaml Outdated Show resolved Hide resolved
This should have been included in Project-MONAI@14b086b when Python 3.8 support was dropped.

Changes were automatically applied by running:
pre_commit run --all-files

Signed-off-by: James Butler <[email protected]>
Changes were automatically applied by running:
python -m isort .

Signed-off-by: James Butler <[email protected]>
@jamesobutler jamesobutler force-pushed the jamesobutler-pyupgrade-fixes branch from 1f8644d to 0cfb3e8 Compare October 24, 2024 13:45
@KumoLiu
Copy link
Contributor

KumoLiu commented Oct 24, 2024

/build

@jamesobutler
Copy link
Contributor Author

@KumoLiu Should the GPU-quick-py3 still be a required status check for the protected dev branch or should it be removed? It appears it was set to not run as of 18a671a which was well over a year ago. It is listed as a required status check, but it has been constantly been getting skipped (https://github.com/Project-MONAI/MONAI/actions/workflows/pythonapp-gpu.yml). It seems like the pythonapp-gpu.yml could be removed from the source repository so that it no longer runs or at least modified to not run on pull request opened.

https://github.com/Project-MONAI/MONAI/actions/runs/11500638643/workflow?pr=8150

{C7002F56-F6BF-4A02-86BF-4996E8628D63}

@KumoLiu
Copy link
Contributor

KumoLiu commented Oct 24, 2024

@KumoLiu Should the GPU-quick-py3 still be a required status check for the protected dev branch or should it be removed?

Hi @jamesobutler, yes it's required and the workflow has been move to the blossom, so it is run in this pipeline: https://github.com/Project-MONAI/MONAI/actions/runs/11502209707.

@KumoLiu KumoLiu merged commit 8dc3b4f into Project-MONAI:dev Oct 24, 2024
28 checks passed
@jamesobutler jamesobutler deleted the jamesobutler-pyupgrade-fixes branch October 24, 2024 15:27
@jamesobutler
Copy link
Contributor Author

@KumoLiu I see that blossom-ci is a required status check, but it doesn't seem like GPU-quick-py3 should be set as a required status check for a PR. Enforcing all status checks to pass prior to integrating into a protected branch would require it to be constantly overruled by an admin enforcement since the rule isn't being respected.

{1F186E76-66BE-4DC9-8AB7-B4915D27BB28}

@KumoLiu
Copy link
Contributor

KumoLiu commented Oct 24, 2024

@jamesobutler, Thanks for pointing out, I just updated that part, seems it's out of the date.

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.

3 participants