Skip to content

Conversation

@xuhdev
Copy link
Collaborator

@xuhdev xuhdev commented Oct 8, 2019

According to #27285 , seems we do not intend to use shebang as an indication of Python version, thus
we enable EXE001 flake8 check.
For violations, we either remove shebang from non-executable Python scripts or grant them executable permission.

According to pytorch#27285 , seems we do not intend to use shebang as an indication of Python version, thus
we enable EXE001 flake8 check.
For violations, we either remove shebang from non-executable Python scripts or grant them executable permission.
@pytorchbot pytorchbot added caffe2 module: ci Related to continuous integration module: cpp Related to C++ API module: docs Related to our documentation, both in docs/ and docblocks module: lint Issues related to our Python/C++ lint rules (run by Travis) module: rocm AMD GPU support for Pytorch labels Oct 8, 2019
@xuhdev xuhdev requested review from ezyang and pietern October 8, 2019 19:02
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

Thanks, @xuhdev!

Before merging, I want to get an ack from @ezyang as well.

The idea here is: hashbang means you have a main function and mode 755, no hashbang mode 644.

@ezyang
Copy link
Contributor

ezyang commented Oct 9, 2019

Sorry, there is some hidden back story here, which makes it a little tricky to do this.

In Facebook internally, we have a lint check that essentially complains if you don't have Python 3 compatibility imports. There are a few ways to suppress it, but the one that is suggested by the linter is adding a shebang line to the top that explicitly says python3, even if the intent is never to actually run the file as an executable.

Andres helpfully gave some more tips about the linter though:

  • This linter will not complain if it finds a .python3 or .python2 file up the parent dirs. This also affects how flake8 treats your files.
  • This linter will not complain if it finds any future import in a file (so you could do from future import print and it'll stop complaining).
  • This linter will not complain if it finds a versioned python shebang in a file (e.g. #!/usr/bin/env python2.7, #!/usr/bin/env python2, #!/usr/bin/env python3, basically anything with that matches ^#!.*python[23])
  • This linter will not complain if the file has @lint-ignore-evert PYTHON3COMPATIMPORTS.

I landed a PR that puts .python2 in our top level directory, which should shut this up. But that's the reason why people keep adding these shebangs.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@pietern
Copy link
Contributor

pietern commented Oct 9, 2019

Sweet! Thanks for the context, @ezyang! For future reference, the PR is #27214.

@xuhdev xuhdev deleted the shebang branch October 9, 2019 16:57
@xuhdev
Copy link
Collaborator Author

xuhdev commented Oct 9, 2019

Thanks for the explanation!

@facebook-github-bot
Copy link
Contributor

@ezyang merged this pull request in 987e37b.

thiagocrepaldi pushed a commit to thiagocrepaldi/pytorch that referenced this pull request Feb 4, 2020
Summary:
According to pytorch#27285 , seems we do not intend to use shebang as an indication of Python version, thus
we enable EXE001 flake8 check.
For violations, we either remove shebang from non-executable Python scripts or grant them executable permission.
Pull Request resolved: pytorch#27560

Differential Revision: D17831782

Pulled By: ezyang

fbshipit-source-id: 6282fd3617b25676a6d959af0d318faf05c09b26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

caffe2 Merged module: ci Related to continuous integration module: cpp Related to C++ API module: docs Related to our documentation, both in docs/ and docblocks module: lint Issues related to our Python/C++ lint rules (run by Travis) module: rocm AMD GPU support for Pytorch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants