update python syntax >=3.6 #4585

Borda · 2021-10-10T21:50:46Z

In fact, I was just adding a new hook to pre-commit pyupgrade which generated all these changes
also I have added python_requires=">=3.6", to the seup.py

An alternative would be enable pre-commit bot and then it would be clear that most of the changes comes just from the pre-commit hooks, see https://pre-commit.ci/

facebook-github-bot · 2021-10-10T21:50:52Z

Hi @Borda!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

facebook-github-bot · 2021-10-11T00:08:12Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

NicolasHug

Thanks for the RP @Borda ,

Overall I'm happy with the changes, we tend to favour f-string over formatted strings lately, and it's nice for everything to be consistent. The only issue is that it will add another "noisy" commit to git blame on github, but IMHO it's still worth it.

One thing I'm not sure about is the commit hook, considering that some of these changes are making torchscript unhappy (see comment below), we might not be able to enforce this with a hook.

Let's also ping @pmeier @fmassa @datumbox @prabhat00155 to see what they think.

NicolasHug · 2021-10-11T08:02:24Z

torchvision/transforms/functional.py

@@ -923,7 +921,7 @@ def _get_inverse_affine_matrix(
    # Thus, the inverse is M^-1 = C * RSS^-1 * C^-1 * T^-1

    rot = math.radians(angle)
-    sx, sy = [math.radians(s) for s in shear]
+    sx, sy = (math.radians(s) for s in shear)


Looks like torchscript in unhappy here, I think we should revert to a list

datumbox · 2021-10-11T08:10:32Z

I was about to write something similar to Nicolas. I like the fact we modernize the code and think it's worth it. I wouldn't add it as a precommit for the reasons mentioned above.

Not sure if we should try to split it in parts or do it all in one go.

NicolasHug · 2021-10-11T08:19:39Z

Not sure if we should try to split it in parts or do it all in one go.

I think it's best to do it in one go to avoid too many noisy commits. As long as the tests are passing it should be OK. Hopefully we can trust pyupgrade not to do anything insane

Borda · 2021-10-11T08:50:31Z

One thing I'm not sure about is the commit hook, considering that some of these changes are making torchscript unhappy (see comment below), we might not be able to enforce this with a hook.

I may have related thought as each statement (code line) can be written in many ways, is the actual optimal, personally, I would keep any of these hook/bots and try to rewrite some small number of lines to complain of hook and also to be fine with you and torchscript... what do you think?

I was about to write something similar to Nicolas. I like the fact we modernize the code and think it's worth it. I wouldn't add it as a precommit for the reasons mentioned above.

I may add that these changes are one-time as it is applied codebase-wide... later it is the same as Flak8 when you write new code and it does not compliment you edit it, but in fact you may not ever see what code structure that author had in mind at the tie he started his contribution (compare to the final submission/PR version)

Hopefully we can trust pyupgrade not to do anything insane

if anything went wrong in future, you can just drop it 🐰

NicolasHug · 2021-10-11T09:00:13Z

I might be missing something, but I don't think the changes here are related to flake8. flake8 won't complain with non-f-strings, or when classes inherit from object, etc.

For the rest, if there is a simple and easy way to make both the hook and torchscript happy, then we can consider the hook for inclusion, provided that it runs fast, etc.

But torchscript support trumps everything else, so if having the hook means that we can't make torchscript happy, unfortunately that means we need to remove it

Borda · 2021-10-11T09:01:54Z

I might be missing something, but I don't think the changes here are related to flake8. flake8 won't complain with non-f-strings, or when classes inherit from object, etc.

not at all, I made it just as a parallel example, sorry for confusion

For the rest, if there is a simple and easy way to make both the hook and torchscript happy, then we can consider the hook for inclusion, provided that it runs fast, etc.

I will check it this week

NicolasHug · 2021-10-11T09:17:44Z

Thanks @Borda ,

Slightly related, I noticed that we started having string "that look" "like this", for example

"Expected target boxes to be of type " "Tensor, got {:}.".format(type(boxes)))

I think they come from the recent black formatting.

git grep '\" \"' torchvision references | wc -l
58

We should try to fix those before applying pyupgrade because I think it actually prevents pyupgrade from converting those to f-strings

Borda · 2021-10-11T11:59:46Z

@NicolasHug thay may be another step, so far you do not enforce much code formating, right?

Slightly related, I noticed that we started having string "that look" "like this", for example

this comes from placing a two-line string to a single line which would be otherwise caught sooner with any formatting tool like Black of YAPF... what do you think about adding any of them in future?
Mind be interesting: Thoughts on Why Code Formatting is even more important for Open-Source

NicolasHug · 2021-10-11T12:03:35Z

@NicolasHug thay may be another step, so far you do not enforce much code formating, right?

We do enforce black-formatting now, since last week.

But I think we should fix those "weird" "strings" before we apply pyupgrade because they prevent pyupgrade from working properly on those strings. To keep the noisy commits to a minimum we should also do it in the same PR (this one :) )

Borda · 2021-10-11T12:10:24Z

We do enforce black-formatting now, since last week.

do you have it also in pre-commit hooks?
I see this one

vision/.pre-commit-config.yaml

Line 5 in 221dd5f

- id: ufmt

but I don't think that it is the optimal one, rather use

  - repo: https://github.com/psf/black
    rev: 21.9b0
    hooks:
      - id: black
        name: Format code

I can add it in separate PR

NicolasHug · 2021-10-11T12:21:36Z

ufmt is a combination of black and usort, we have to rely on this (and not on the individual formatters) because of compatibility reasons with FB's internal linters.

fmassa · 2021-10-11T14:56:40Z

Ok with changing to use f-strings, although it's a pity that it will be a bit harder for git blame. But fine with me.

prabhat00155

Thanks @Borda for the PR. Reference scripts are not tested by unit tests, did you verify that everything runs fine?

references/classification/transforms.py

Borda · 2021-10-12T21:58:20Z

Reference scripts are not tested by unit tests, did you verify that everything runs fine?

seems they require multi-node/GPU machines which I do not have in my disposal :(

NicolasHug

Thanks for the PR @Borda ,

This LGTM with a minor comment below.

I checked the classification reference, it's working fine. The rest of the references should be OK as well - there's not torchscript stuff there anyway, and I think this is what was causing the few issues earlier.

I'm OK to keep the new hook for now as it runs pretty quickly. However if it starts getting in our way and forces us to fix things like in 418ad23, I think we will need to remove it.

Thanks!

.pre-commit-config.yaml

datumbox · 2021-10-13T10:15:52Z

@Borda Thanks a lot for the contribution. I believe this PR improves and modernizes the code-base.

Though I understand that these changes came from pyupgrade and it might be a good idea to run it in every PR to ensure consistency, in the past we've decided against modifying the code without ones knowledge. A good reason for why we don't want this is JIT-scriptability. Earlier versions of your PR were rewritten from the tool in a non-JIT compliant way. Of course this got caught by the tests, but still it might be dangerous.

I think it might be best to remove the hook from this PR and give us a bit more time to discuss whether we would like to revisit our past decision for automatic code modifications prior commit. This could happen on a separate PR that would be easier to invert if need be. Let me know what you think?

Borda · 2021-10-13T13:13:27Z

I think it might be best to remove the hook from this PR and give us a bit more time to discuss whether we would like to revisit our past decision for automatic code modifications prior commit. This could happen on a separate PR that would be easier to invert if need be. Let me know what you think?

resolved conflicts and commented out the hook so later you can just uncomment it back...

Borda · 2021-10-14T17:03:04Z

seems to be failing on Segmentation fault :/

datumbox · 2021-10-14T17:16:07Z

@Borda It's not your PR, our CI is toasted... It should be resolved tomorrow using the latest nightly of PyTorch. Apologies for the inconvenience.

Borda · 2021-10-26T16:16:43Z

@datumbox fixed and resolved conflicts...

references/classification/transforms.py

datumbox

I went through the entire PR quickly. Unfortunately this is very hard because of the volume of files involved. I think overall looks OK, the if/elif marked above seemed to appear only to a couple of places so perhaps it was not reverted everywhere originally.

The changes on the code are for the best; I agree it produced a much cleaner result. I did find a couple of issues with the fstrings though (see below), which might be indicating issues with the automated tool used to refactor the code. Let me know your thoughts.

setup.py

torchvision/transforms/_transforms_video.py

torchvision/transforms/transforms.py

Borda · 2021-10-26T20:16:54Z

I went through the entire PR quickly.

@datumbox thank you for your review, I have fixed all issues...

datumbox

Thanks for the changes!

NicolasHug · 2021-10-28T09:26:38Z

@Borda thanks a lot for the PR and for bearing with us. There are still some minor cases where the script didn't use f-strings. I'll take care of these manually, and then merge the PR.

No need to do anything more on your end :)

Thanks a lot!

Borda · 2021-10-28T09:36:31Z

I'll take care of these manually, and then merge the PR.
No need to do anything more on your end :)

Thank you!

NicolasHug · 2021-10-28T11:47:44Z

Failure is unrelated and will be taken care of by #4775

Merging, thanks a lot @Borda!

github-actions · 2021-10-28T11:49:43Z

Hey @NicolasHug!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

…ing pyupgrade (#4585) Summary: Co-authored-by: Nicolas Hug <[email protected]> Reviewed By: datumbox Differential Revision: D32064703 fbshipit-source-id: 5d8a0823844ec36e82d94cddaf5dd25ac18327f3

…rade (pytorch#4585) Co-authored-by: Nicolas Hug <[email protected]>

pytorch-probot bot added the ciflow/default label Oct 10, 2021

facebook-github-bot added the cla signed label Oct 10, 2021

NicolasHug reviewed Oct 11, 2021

View reviewed changes

Borda mentioned this pull request Oct 11, 2021

Pre-commit: Black #4588

Closed

Borda requested a review from NicolasHug October 11, 2021 12:26

prabhat00155 reviewed Oct 12, 2021

View reviewed changes

references/classification/transforms.py Outdated Show resolved Hide resolved

Borda mentioned this pull request Oct 13, 2021

Refactor unnecessary else / elif when if block has a return statement #4606

Closed

NicolasHug approved these changes Oct 13, 2021

View reviewed changes

.pre-commit-config.yaml Show resolved Hide resolved

Borda added 6 commits October 14, 2021 18:46

reorder

cb175cc

edit setup

6098b51

def pyupgrade

a8fe5f7

apply

15fc26c

fix " "

7d04bcf

fix tuple

2754a64

Borda added 2 commits October 14, 2021 18:46

elif

2e785df

pause hook

8e39977

Borda requested a review from prabhat00155 October 14, 2021 16:48

Borda added 2 commits October 26, 2021 18:03

Merge branch 'main' into pre-commit/pyupgrade

bf26d4c

fixing

439cbb9

datumbox reviewed Oct 26, 2021

View reviewed changes

references/classification/transforms.py Show resolved Hide resolved

datumbox reviewed Oct 26, 2021

View reviewed changes

Borda and others added 2 commits October 26, 2021 20:59

Merge branch 'main' into pre-commit/pyupgrade

8419b3b

fixing

55e3477

datumbox approved these changes Oct 27, 2021

View reviewed changes

Merge branch 'main' into pre-commit/pyupgrade

fbbb8da

NicolasHug added 3 commits October 28, 2021 10:55

Fix remaining f-strings

1da64c0

Some more weird stuff

331ac7d

revert some changes to torchvision/io/_video_opt.py - jit error

fc9aa7f

NicolasHug merged commit d367a01 into pytorch:main Oct 28, 2021

NicolasHug added code quality enhancement labels Oct 28, 2021

Borda deleted the pre-commit/pyupgrade branch October 28, 2021 12:12

cyyever pushed a commit to cyyever/vision that referenced this pull request Nov 16, 2021

Use f-strings almost everywhere, and other cleanups by applying pyupg…

7f31bd6

…rade (pytorch#4585) Co-authored-by: Nicolas Hug <[email protected]>

pmeier mentioned this pull request Dec 16, 2021

upgrade usort to 1.0.2 and black to 22.3.0 #5106

Merged

pmeier mentioned this pull request Dec 27, 2021

Update code to Python 3.7 compliance #5125

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update python syntax >=3.6 #4585

update python syntax >=3.6 #4585

Borda commented Oct 10, 2021

facebook-github-bot commented Oct 10, 2021

facebook-github-bot commented Oct 11, 2021

NicolasHug left a comment

NicolasHug Oct 11, 2021

datumbox commented Oct 11, 2021

NicolasHug commented Oct 11, 2021

Borda commented Oct 11, 2021 •

edited

Loading

NicolasHug commented Oct 11, 2021

Borda commented Oct 11, 2021 •

edited

Loading

NicolasHug commented Oct 11, 2021

Borda commented Oct 11, 2021

NicolasHug commented Oct 11, 2021

Borda commented Oct 11, 2021

NicolasHug commented Oct 11, 2021

fmassa commented Oct 11, 2021

prabhat00155 left a comment

Borda commented Oct 12, 2021

NicolasHug left a comment

datumbox commented Oct 13, 2021

Borda commented Oct 13, 2021

Borda commented Oct 14, 2021

datumbox commented Oct 14, 2021

Borda commented Oct 26, 2021

datumbox left a comment

Borda commented Oct 26, 2021

datumbox left a comment

NicolasHug commented Oct 28, 2021 •

edited

Loading

Borda commented Oct 28, 2021

NicolasHug commented Oct 28, 2021

github-actions bot commented Oct 28, 2021

update python syntax >=3.6 #4585

update python syntax >=3.6 #4585

Conversation

Borda commented Oct 10, 2021

facebook-github-bot commented Oct 10, 2021

Action Required

Process

facebook-github-bot commented Oct 11, 2021

NicolasHug left a comment

Choose a reason for hiding this comment

NicolasHug Oct 11, 2021

Choose a reason for hiding this comment

datumbox commented Oct 11, 2021

NicolasHug commented Oct 11, 2021

Borda commented Oct 11, 2021 • edited Loading

NicolasHug commented Oct 11, 2021

Borda commented Oct 11, 2021 • edited Loading

NicolasHug commented Oct 11, 2021

Borda commented Oct 11, 2021

NicolasHug commented Oct 11, 2021

Borda commented Oct 11, 2021

NicolasHug commented Oct 11, 2021

fmassa commented Oct 11, 2021

prabhat00155 left a comment

Choose a reason for hiding this comment

Borda commented Oct 12, 2021

NicolasHug left a comment

Choose a reason for hiding this comment

datumbox commented Oct 13, 2021

Borda commented Oct 13, 2021

Borda commented Oct 14, 2021

datumbox commented Oct 14, 2021

Borda commented Oct 26, 2021

datumbox left a comment

Choose a reason for hiding this comment

Borda commented Oct 26, 2021

datumbox left a comment

Choose a reason for hiding this comment

NicolasHug commented Oct 28, 2021 • edited Loading

Borda commented Oct 28, 2021

NicolasHug commented Oct 28, 2021

github-actions bot commented Oct 28, 2021

Borda commented Oct 11, 2021 •

edited

Loading

Borda commented Oct 11, 2021 •

edited

Loading

NicolasHug commented Oct 28, 2021 •

edited

Loading