Upgrade python syntax to 3.9 and newer #6376

jamesobutler · 2022-05-14T00:03:54Z

~~This continually enforces changes made in 4483cc0 and 1a85eba.~~

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

EDIT

Similarly to 1a85eba (introduced through PR #6132) and 4483cc0 (introduced through PR #5625), this PR uses pyupgrade to also automatically upgrade python syntax.

Using pyupgrade

Install: PythonSlicer -m pip install pyupgrade

Running:

On 1 file: PythonSlicer -m pyupgrade --py39-plus MyPythonFile.py

On multiple files, there are few options:

on unix system:

PythonSlicer -m pyupgrade --py39-plus $(find . -type f -name "*.py")

on any system, use @jamesobutler pyupgrade-script.py written to automate running pyupgrade across all python files in the Slicer repo. It was run by PythonSlicer pyupgrade-script.py.

# pyupgrade-script.py
import os
import subprocess

search_directory = "C:/Users/JamesButler/Documents/GitHub/Slicer"
for root, _, files in os.walk(search_directory):
    for file_item in files:
        file_path = os.path.join(root, file_item)
        if os.path.isfile(file_path) and file_path.endswith(".py"):
            subprocess.call(["PythonSlicer", "-m", "pyupgrade", "--py39-plus", file_path])

jamesobutler · 2022-05-21T15:36:11Z

It will be easier to resolve merge conflicts in this PR if #6375 is merged first.

jamesobutler · 2022-06-23T14:03:12Z

I've rebased this PR to fix merge conflicts from the recent whitespace updates.

jamesobutler · 2022-08-01T16:49:58Z

I'm following up on some of my old PRs. @jcfr Do you think we can go ahead and proceed with this one? Let me know if ENH is an appropriate commit prefix for some of this CI/utility related work - re ENH: Update GitHub Actions lint workflow to include pyupgrade

jamesobutler · 2022-09-29T21:53:34Z

Following up on this old PR here.

Any objections to proceeding with this one @jcfr, @lassoan, @pieper ?

lassoan

Thank you, looks all good, just added two small comments.

lassoan · 2022-09-29T22:05:29Z

Base/Python/slicer/util.py

@@ -2707,7 +2707,7 @@ def _messageDisplay(logLevel, text, testingReturnValue, mainWindowNeeded=False,
    if not windowTitle:
        windowTitle = slicer.app.applicationName + " " + logLevelString
    if slicer.app.testingEnabled():
-        logging.info("Testing mode is enabled: Returning %s and skipping message box [%s]." % (testingReturnValue, windowTitle))
+        logging.info(f"Testing mode is enabled: Returning {testingReturnValue} and skipping message box [{windowTitle}].")


I would prefer not to introduce usage of f-strings in logging functions, because using f-strings results in performing expensive string manipulations even if the message does not end up being printed due to the current log level.

Has pyupgrade recommended this change?

Yes pyupgrade changes it to .format() and then re-running changes it again to an f-string. The maintainer points to asottile/pyupgrade#503 (comment) in that f-string usage here is not a performance drop.

If there's a performance concern fstrings and format statements should be avoided in logging calls since the arguments will be evaluated before the logging method is invoked, so the computation will always happen regardless of log level.

I'm not going to argue with the maintainer of pyupgrade on this one, so since there are no exceptions to keep % formatting only for logging statements with pyupgrade that means I would need to remove pyupgrade from the pre-commit-config.yaml in this PR and we would have to apply all the changes in a manual review basis and every so often update the Slicer code base to get back into the compliance that we believe in.

Thanks for the link @jamesobutler, it led to some very interesting discussions. There seems to be a very strong disagreement between various Python developers on this topic and probably the correct answer depends on the use case. The insignificant performance drop was a measurement error, in the same issue further along the discussion, the actual performance drop was shown to be about 20%. So, this is a real problem. There are additional potential issues with exceptions during string conversion.

However, in our specific these cases here, there is no performance or exception-safety problems, so it should be fine to use f-strings.

My only concern is that using f-strings in logging would make this the default choice everywhere, even where performance matters. But maybe keeping the classic formatting in our log messages is not a very effective way of educating/reminding developers anyway, and if the cost is not to use pyupgrade at all or spend a lot of time with fighting with various lint tools, then it may just not worth it.

So, I'm OK with keeping the suggested changes.

Looking at https://pylint.pycqa.org/en/latest/user_guide/messages/warning/logging-format-interpolation.html, the following is indicated:

Another reasonable option is to use f-string. If you want to do that, you need to enable logging-format-interpolation and disable logging-fstring-interpolation.

Given the fact there is no significant performance impact ¹, enforcing f-string everywhere is reasonable.

That said, I think we should explicitly configure pylint with:

logging-fstring-interpolation -> disabled

logging-format-interpolation -> enabled

Footnotes

https://github.com/Slicer/Slicer/pull/6376/files#r984071600 ↩

lassoan · 2022-09-29T22:05:56Z

Base/Python/slicer/util.py

@@ -2730,7 +2730,7 @@ def messageBox(text, parent=None, **kwargs):
    import logging, qt, slicer
    if slicer.app.testingEnabled():
        testingReturnValue = qt.QMessageBox.Ok
-        logging.info("Testing mode is enabled: Returning %s (qt.QMessageBox.Ok) and displaying an auto-closing message box [%s]." % (testingReturnValue, text))
+        logging.info(f"Testing mode is enabled: Returning {testingReturnValue} (qt.QMessageBox.Ok) and displaying an auto-closing message box [{text}].")


Same as above, I would not use f-strings in logging.

jcfr · 2022-10-11T14:48:56Z

Following the integration of #6573, this should be rebased against the current main

jamesobutler · 2022-10-11T15:04:51Z

I've rebased to fix the merge conflict.

jamesobutler · 2022-10-17T14:06:42Z

I've rebased to update to the latest version of pyupgrade and re-run to be in compliance.

jcfr · 2022-10-18T03:28:41Z

Utilities/Scripts/genqrc.py

@@ -18,7 +18,7 @@ def writeFile(path, content):
            pass

    # Write file
-    with open(path, "wt") as f:
+    with open(path, "w") as f:


Since text mode (t) is the default ¹, this makes sense ✔️

Footnotes

https://stackoverflow.com/questions/23051062/open-files-in-rt-and-wt-modes ↩

jcfr · 2022-10-18T14:26:41Z

Base/Python/slicer/util.py

@@ -2707,7 +2707,7 @@ def _messageDisplay(logLevel, text, testingReturnValue, mainWindowNeeded=False,
    if not windowTitle:
        windowTitle = slicer.app.applicationName + " " + logLevelString
    if slicer.app.testingEnabled():
-        logging.info("Testing mode is enabled: Returning %s and skipping message box [%s]." % (testingReturnValue, windowTitle))
+        logging.info(f"Testing mode is enabled: Returning {testingReturnValue} and skipping message box [{windowTitle}].")


Looking at https://pylint.pycqa.org/en/latest/user_guide/messages/warning/logging-format-interpolation.html, the following is indicated:

Another reasonable option is to use f-string. If you want to do that, you need to enable logging-format-interpolation and disable logging-fstring-interpolation.

Given the fact there is no significant performance impact ¹, enforcing f-string everywhere is reasonable.

That said, I think we should explicitly configure pylint with:

logging-fstring-interpolation -> disabled

logging-format-interpolation -> enabled

Footnotes

https://github.com/Slicer/Slicer/pull/6376/files#r984071600 ↩

jamesobutler · 2022-10-18T16:03:31Z

@jcfr I've included W1203 in the ignore of .flake8 configuration which I believe utilizes the pylint codes.
https://pylint.pycqa.org/en/latest/user_guide/messages/warning/logging-fstring-interpolation.html

jcfr · 2022-10-19T05:50:25Z

After looking at this in more details, I realized that pyupgrade does not make use of pylint, so updating the setting has no effect. I will then remove the last commit.

Remove commit ENH: Ignore pylint W1203 logging-fstring-interpolation

Strictly speaking, running pyupgrade in the GitHub workflow does not really make sense because (1) we do not yet push back the changes to the open pull-request (this could be done enabling and (2) rewriting details are not reported back in the output.

Using pre-commit to run against all files may indeed be convenient and may avoid the following:

PythonSlicer -m pip install pyupgrade
PythonSlicer -m pyupgrade --py39-plus $(find . -type f -name "*.py")

and only require the following

PythonSlicer -m pip install pre-commit
PythonSlicer -m pre_commit run --all-files

Source: https://discourse.slicer.org/t/locally-execute-pre-commit-linting-locally/22837

This means that the update of the workflow without enabling service like pre-commit.ci ¹ or using action like git-auto-commit-action² does not really fulfill:

continually enforces changes made in 4483cc0 and 1a85eba.

For these reasons, I will:

update the pull-request to only integrate the changes without updating the workflow.
create an issue for adding auto-update of pull-request

jcfr · 2022-10-19T06:24:35Z

@jamesobutler Comparing with the version I obtained with pyupgrade with the one you originally introduced in https://github.com/Slicer/Slicer/tree/0baa88d681a9a4a0bdc8a42a1b6bd97cf1f9265e, I noticed that the following different. Did you explicitly update CLISerializationTest.py ?

jamesobutler · 2022-10-19T11:58:46Z

There was indeed some re-runs of pyupgrade as when you run it once and then immediately run it a second time, it will change more things the second time around. Yes, I might’ve additionally changed some things as well but I don’t remember.

Adding pyupgrade to the pre-commit run enforces the updated syntax for new PRs, but I wasn’t planning for it to be auto fixed by a bot. It would be up to the developer to see the reported issues just like if they had incorrect tab counts reported by pylint which they would manually fix and push. I would treat this as a developer responsibility similar to when someone has a commit that doesn’t pass the commit message check in a multi commit PR and it requires fixing up to pass required status checks.

Strictly speaking, running pyupgrade in the GitHub workflow does not really make sense because (1) we do not yet push back the changes to the open pull-request (this could be done enabling and (2) rewriting details are not reported back in the output.

Re (1) I suggest not to auto-fix PRs with pre-commit.ci to push changes back as mentioned in
#6573 (comment) because when working in a multi commit PR the changes would be in a single commit and not squashed down cleanly into the commit that introduced the compliance issue. Auto-fix can be helpful but will reduce the quality of the commit history with fix-me-up type commits.

Re (2) rewriting details are indeed reported back in the GitHub action workflow results (or in pre-commit.ci results) and also locally as you’ve shown in your screenshot above. This provides developers clear instructions about what something should be changed or that they can run pyupgrade locally to apply the changes and commit and squash into their appropriate commit.

jcfr · 2022-10-19T20:24:40Z

Thanks for the detailed answer, this is very helpful 🙏

I will integrate the change and publish pull-requests respectively adding (1) execution of pyupgrade (2) autoupdate ¹ through the GitHub workflow.

not planning for it to be auto fixed by a bot [...] it would be up to the developer to see the reported issues

👍

rewriting details [...] reported back in the GitHub action workflow results

The screenshot I provided was obtained after running the tool locally, I will investigate further to see how rewritten changed are displayed within the GitHub workflow as I couldn't find an example (yet)

https://github.com/Slicer/Slicer/pull/6573#issuecomment-1274839133 ↩

Changes were applied automatically by running: PythonSlicer -m pip install pyupgrade followed by the following command executed twice PythonSlicer -m pyupgrade --py39-plus MyPythonFile.py for every python file in the Slicer repository. Notes: * Running the command twice is needed to ensure that string associated with `logging` package are updated to use f-string. * Changes have been manually reviewed to revisit some of the changes automatically proposed: - removed remaining parenthesis after class definition in WebServer - removed remaining occurences of `socker.error` from WebServer - updated CLISerializationTest to use f-string Co-authored-by: Jean-Christophe Fillion-Robin <jchris.fillionr@kitware.com>

jamesobutler · 2022-10-19T21:09:28Z

Here' a screenshot from a private repo with an example of pyupgrade indicating changes:

jcfr · 2022-10-19T21:18:46Z

Thanks for sharing the example, I will follow up with a PR including your original commit updating the pre-commit configuration. 🚀

jcfr · 2022-10-20T18:35:44Z

I will follow up with a PR including your original commit updating the pre-commit configuration.

See #6605

jamesobutler force-pushed the pyupgrade-pre-commit branch from 3a66914 to 7c6c4e8 Compare May 19, 2022 13:46

jamesobutler force-pushed the pyupgrade-pre-commit branch from 7c6c4e8 to 1582298 Compare June 1, 2022 18:57

jamesobutler force-pushed the pyupgrade-pre-commit branch 2 times, most recently from 5a24763 to a737f53 Compare June 20, 2022 02:29

jamesobutler force-pushed the pyupgrade-pre-commit branch 3 times, most recently from 524e91f to d8ad3aa Compare June 23, 2022 14:01

jamesobutler requested a review from jcfr June 24, 2022 14:54

jamesobutler force-pushed the pyupgrade-pre-commit branch from d8ad3aa to d8477f0 Compare July 7, 2022 14:03

jamesobutler force-pushed the pyupgrade-pre-commit branch from d8477f0 to e1fb601 Compare July 19, 2022 13:16

jamesobutler force-pushed the pyupgrade-pre-commit branch from e1fb601 to ce67c0a Compare August 1, 2022 16:48

jamesobutler force-pushed the pyupgrade-pre-commit branch from ce67c0a to 9ed2c47 Compare August 8, 2022 21:46

jamesobutler force-pushed the pyupgrade-pre-commit branch from 9ed2c47 to 8e8d175 Compare September 18, 2022 23:30

jamesobutler force-pushed the pyupgrade-pre-commit branch from 8e8d175 to 9f7b811 Compare September 29, 2022 21:52

jamesobutler requested review from pieper and lassoan September 29, 2022 21:52

lassoan reviewed Sep 29, 2022

View reviewed changes

jamesobutler force-pushed the pyupgrade-pre-commit branch from 9f7b811 to 1c51830 Compare September 29, 2022 23:24

jamesobutler force-pushed the pyupgrade-pre-commit branch from 1c51830 to ae06131 Compare October 10, 2022 13:55

lassoan assigned jcfr Oct 11, 2022

jamesobutler force-pushed the pyupgrade-pre-commit branch from ae06131 to b7536a3 Compare October 11, 2022 15:03

jamesobutler force-pushed the pyupgrade-pre-commit branch from b7536a3 to 0baa88d Compare October 17, 2022 14:06

jcfr reviewed Oct 18, 2022

View reviewed changes

jcfr changed the title ~~Enforce latest python syntax using pyupgrade with pre-commit framework~~ Enforce latest python syntax using pyupgrade Oct 19, 2022

jcfr force-pushed the pyupgrade-pre-commit branch from 1124f40 to c63215c Compare October 19, 2022 06:10

jcfr changed the title ~~Enforce latest python syntax using pyupgrade~~ Upgrade python syntax to 3.9 and newer Oct 19, 2022

jcfr mentioned this pull request Oct 19, 2022

Improve lint workflow to run pyupgrade automatically #6599

Open

jcfr force-pushed the pyupgrade-pre-commit branch from c63215c to aeb569c Compare October 19, 2022 20:46

jcfr merged commit a4ea799 into Slicer:main Oct 19, 2022

jamesobutler deleted the pyupgrade-pre-commit branch October 19, 2022 21:04

jcfr mentioned this pull request Oct 20, 2022

ENH: Update pre-commit config to include pyupgrade #6605

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade python syntax to 3.9 and newer #6376

Upgrade python syntax to 3.9 and newer #6376

jamesobutler commented May 14, 2022 •

edited by jcfr

jamesobutler commented May 21, 2022

jamesobutler commented Jun 23, 2022

jamesobutler commented Aug 1, 2022

jamesobutler commented Sep 29, 2022

lassoan left a comment

lassoan Sep 29, 2022

jamesobutler Sep 29, 2022

pieper Sep 30, 2022

jamesobutler Sep 30, 2022 •

edited

lassoan Sep 30, 2022

jcfr Oct 18, 2022 •

edited

lassoan Sep 29, 2022

jcfr commented Oct 11, 2022

jamesobutler commented Oct 11, 2022

jamesobutler commented Oct 17, 2022

jcfr Oct 18, 2022

jcfr Oct 18, 2022 •

edited

jamesobutler commented Oct 18, 2022

jcfr commented Oct 19, 2022 •

edited

jcfr commented Oct 19, 2022

jamesobutler commented Oct 19, 2022 •

edited

jcfr commented Oct 19, 2022

jamesobutler commented Oct 19, 2022

jcfr commented Oct 19, 2022

jcfr commented Oct 20, 2022

Upgrade python syntax to 3.9 and newer #6376

Upgrade python syntax to 3.9 and newer #6376

Conversation

jamesobutler commented May 14, 2022 • edited by jcfr

Using pyupgrade

jamesobutler commented May 21, 2022

jamesobutler commented Jun 23, 2022

jamesobutler commented Aug 1, 2022

jamesobutler commented Sep 29, 2022

lassoan left a comment

Choose a reason for hiding this comment

lassoan Sep 29, 2022

Choose a reason for hiding this comment

jamesobutler Sep 29, 2022

Choose a reason for hiding this comment

pieper Sep 30, 2022

Choose a reason for hiding this comment

jamesobutler Sep 30, 2022 • edited

Choose a reason for hiding this comment

lassoan Sep 30, 2022

Choose a reason for hiding this comment

jcfr Oct 18, 2022 • edited

Choose a reason for hiding this comment

Footnotes

lassoan Sep 29, 2022

Choose a reason for hiding this comment

jcfr commented Oct 11, 2022

jamesobutler commented Oct 11, 2022

jamesobutler commented Oct 17, 2022

jcfr Oct 18, 2022

Choose a reason for hiding this comment

Footnotes

jcfr Oct 18, 2022 • edited

Choose a reason for hiding this comment

Footnotes

jamesobutler commented Oct 18, 2022

jcfr commented Oct 19, 2022 • edited

Footnotes

jcfr commented Oct 19, 2022

jamesobutler commented Oct 19, 2022 • edited

jcfr commented Oct 19, 2022

Footnotes

jamesobutler commented Oct 19, 2022

jcfr commented Oct 19, 2022

jcfr commented Oct 20, 2022

jamesobutler commented May 14, 2022 •

edited by jcfr

jamesobutler Sep 30, 2022 •

edited

jcfr Oct 18, 2022 •

edited

jcfr Oct 18, 2022 •

edited

jcfr commented Oct 19, 2022 •

edited

jamesobutler commented Oct 19, 2022 •

edited