[BEAM-3738] Enable py3 lint and cleanup tox.ini. by udim · Pull Request #4877 · apache/beam

udim · 2018-03-16T17:48:09Z

The important change here is renaming the lint environments such that Tox will magically use the requested Python version.

udim · 2018-03-16T18:26:53Z

R: @holdenk @cclauss @aaltay

udim · 2018-03-16T18:31:24Z

run python postcommit

udim · 2018-03-16T21:04:38Z

Missing license was fixed in #4881, retest this please

holdenk · 2018-03-16T21:10:32Z

Love the refactor, thanks @udim!

aaltay · 2018-03-16T19:55:30Z

  doLast {
    exec {
-      commandLine 'tox', '-e', 'lint', '-c', 'tox.ini'
+      commandLine 'tox', '-e', 'py27-lint', '-c', 'tox.ini'


Should we also run the py3 lint?

aaltay · 2018-03-16T21:08:28Z

    ]

-REQUIRED_SETUP_PACKAGES = [
-    'nose>=1.0',


Is not this required by setup, because test_suite references it?

Does it make sense to raise REQUIRED_PIP_VERSION = '7.0.0' to something a closer to the current 9.0.3 or do we leave that alone for now? Alternatively, should the process do pip install --upgrade pip?

@aaltay setup_requires= lists packages required to run setup.py. I couldn't see why nose is required for that.
Specifying nose in the tests_require= keyword arg is sufficient.

@cclauss I would like to leave it alone in this PR, and I don't have any objection to raising the required version.
Note that creating a new virtualenv installs (at least for me) a version of pip that's newer than the system installed version (9.0.2 vs 9.0.1).

Is there a reason for us to raise the required version?

Alternatively, should the process do pip install --upgrade pip?
Which process? As far as I know, virtualenv brings a new version of pip with it.

Python packaging has changed a lot in the past three years especially in relation to Python 3 requirements https://pip.pypa.io/en/stable/news We want to avoid compatibility issues in the migration to https://github.com/pypa/warehouse

The virtualenv does bring a pip but it can be upgraded by pip (bootstrapping).

@cclauss Are you referring to a specific issue? REQUIRED_PIP_VERSION is just the minimum version we require, we are not force downgrading to that version. If there is a known issue in compatibility we can consider raising the minimum supported version

@udim Is it required to run python setup.py test. If not we can drop it.

aaltay · 2018-03-16T21:12:02Z

+# Set [] options for pip install, e.g., pip install apache-beam[test].
+extras = test
+# Always recreate the virtual environment.
+recreate = True


Is there a need for this? Would not affect local development?

Removed. It was convenient for me but I realized it adds 15s to the run.
You can always pass --recreate on the command line.

aaltay · 2018-03-16T21:12:24Z

+# Always recreate the virtual environment.
+recreate = True
+# Pass these environment variables to the test environment.
+passenv = TRAVIS*


I believe we do not need this anymore.

aaltay · 2018-03-16T21:26:48Z

-  grpcio-tools==1.3.5
-whitelist_externals=find
 commands =
-  python --version


Printing python and pip version was very useful in debugging issues. Can we keep them?

+1 on this request.

aaltay · 2018-03-16T21:27:53Z

 commands =
  python --version
  pip --version
  time pip install -e .[test,gcp,docs]


Should we use extras instead of [test,gcp,docs]?

aaltay · 2018-03-16T21:28:25Z

+# Shared environment options.
+[testenv]
+# Set [] options for pip install, e.g., pip install apache-beam[test].
+extras = test


How are these extra packages installed? Is it a tox feature?

These are not packages, but options for pip when installing the dist tarball. I reworded the comment above.

aaltay · 2018-03-22T04:24:40Z

  {toxinidir}/run_tox_cleanup.sh
  python apache_beam/examples/complete/autocomplete_test.py
-  python setup.py test
+  #python setup.py test


aaltay · 2018-03-22T04:25:39Z

+  #python setup.py test
  {toxinidir}/run_tox_cleanup.sh

 [testenv:py27-cython]


What is py27-cython, py27-cython2, py27-cython3 ?

Please ignore. Testing.

aaltay · 2018-03-22T04:25:49Z

 # new environments will be excluded by default unless explicitly added to envlist.
-envlist = py27,py27-{gcp,cython,lint},py3-lint,docs
+#envlist = py27,py27-{gcp,cython,lint},py3-lint,docs
+envlist = py27-cython{,2,3}


left over comment?

udim · 2018-03-22T16:52:15Z

@aaltay the changes you're seeing in the second commit are my attempts at debugging Jenkins runs.

aaltay · 2018-03-22T16:53:49Z

@udim Thank you. Let me know when it is ready for review.

- Add BEAM_EXPERIMENTAL_PY3 environment variable. Currently only used to run the Tox py3-lint environment. - Rename environments to use factors, such that py27-lint and py3-lint use Python versions 2.7.x and 3.x.x respectively. - Remove some redundant package requirements such as nose from being specified in tox.ini if already present in setup.py. - Use shared environment settings in tox.ini, under "[testenv]". - Factor out *.{pyc,c,so} cleanup into a separate script.

udim · 2018-03-23T17:32:11Z

Ready for review and merging if Python checks pass.

aaltay

Thank you @udim. I asked some question for my own learning. I can merge after tests pass.

aaltay · 2018-03-23T17:36:26Z

-whitelist_externals=
-  find
-  time
+  cython==0.26.1


Curious why this version? Latest is 0.28.1.

This is the minimum version required by setup.py (REQUIRED_CYTHON_VERSION). I didn't want to increase the version beyond that and risk breaking things in this PR.

Side note: sdks/python/container/Dockerfile specifies 0.27.2. shrug
I wish all these pinned versions were synced to a single requirements.txt file. (see pip-compile)

+1 on a single requirements.txt file.

That is fair to not change the version in this PR. We should consider upgrading it.

It would be nice to have a single source of truth. Although I do not see how pip-compile can update the Dockerfile. It is worth filing a JIRA for that.

pip-compile can generate requirements.txt from setup.py, and the docker image can pull that in.
The only problem is that we have a different set of requirements depending on the extras specified (such as [test] and [gcp]), or whether cython is used.
So the solution is probably to use a script to generate a set of requirements files, using pip-compile output as an intermediate step. setup.py will be used as the single source of truth for version pinning.

aaltay · 2018-03-23T17:36:40Z

+  {toxinidir}/run_tox_cleanup.sh

-[testenv:py27cython]
+# This environment will fail in Jenkins if named "py27-cython".


For my education, do you know why it fails?

No bleeding clue. Since I don't have ssh access I can't really debug it.
Honestly I'm glad I got it working.

aaltay · 2018-03-23T17:37:58Z

-    ]
-
 REQUIRED_TEST_PACKAGES = [
+    'nose>=1.3.7',


Why are we increasing the required version for nose?

tox.ini already required 1.3.7. I thought this would be a no-op.

I also wanted to merge requirements. Now only setup.py has the nose requirement.

Sounds good.

Changes to tox.ini were done in apache/beam#4877.

Changes to tox.ini were done in apache#4877.

Changes to tox.ini were done in #4877.

udim force-pushed the tox-cleanup branch 3 times, most recently from 7e223cf to f59b8ca Compare March 16, 2018 18:23

udim changed the title ~~[BEAM-3738] Cleanup tox.ini.~~ [BEAM-3738] Enable py3 lint and cleanup tox.ini. Mar 16, 2018

udim mentioned this pull request Mar 16, 2018

[BEAM-3738] Enable py3 lint #4801

Closed

aaltay reviewed Mar 16, 2018

View reviewed changes

udim force-pushed the tox-cleanup branch 4 times, most recently from c314f70 to 7d002dc Compare March 22, 2018 02:06

aaltay reviewed Mar 22, 2018

View reviewed changes

udim force-pushed the tox-cleanup branch from 7d002dc to 4d3214d Compare March 22, 2018 16:39

udim force-pushed the tox-cleanup branch 3 times, most recently from 51a14ad to 0e9f1ba Compare March 23, 2018 01:39

udim added 2 commits March 23, 2018 09:43

Address review comments.

0282cd7

udim force-pushed the tox-cleanup branch from 0e9f1ba to edde2b0 Compare March 23, 2018 16:44

Fix py3-lint error and build.gradle.

2438aba

udim force-pushed the tox-cleanup branch from edde2b0 to 2438aba Compare March 23, 2018 17:31

aaltay approved these changes Mar 23, 2018

View reviewed changes

cclauss approved these changes Mar 23, 2018

View reviewed changes

aaltay merged commit dbadcab into apache:master Mar 23, 2018

udim deleted the tox-cleanup branch March 23, 2018 18:42

cclauss mentioned this pull request Mar 23, 2018

[BEAM-3738] Add more flake8 tests to run_pylint.sh #4798

Closed

10 tasks

tvalentyn added a commit to tvalentyn/beam-site that referenced this pull request Apr 10, 2018

Update instructions to run linter.

8cd1e01

Changes to tox.ini were done in apache/beam#4877.

tvalentyn mentioned this pull request Apr 10, 2018

Update instructions to run linter. apache/beam-site#416

Closed

asfgit pushed a commit to apache/beam-site that referenced this pull request Apr 11, 2018

Update instructions to run linter.

26bd1c3

Changes to tox.ini were done in apache/beam#4877.

kennknowles pushed a commit to kennknowles/beam that referenced this pull request Jun 1, 2018

Update instructions to run linter.

91fc71c

Changes to tox.ini were done in apache#4877.

robertwb pushed a commit to robertwb/incubator-beam that referenced this pull request Jun 5, 2018

Update instructions to run linter.

d7f71cb

Changes to tox.ini were done in apache#4877.

robertwb pushed a commit to robertwb/incubator-beam that referenced this pull request Jun 5, 2018

Update instructions to run linter.

9a386cb

Changes to tox.ini were done in apache#4877.

melap pushed a commit that referenced this pull request Jun 20, 2018

Update instructions to run linter.

523c2b9

Changes to tox.ini were done in #4877.

Uh oh!

Conversation

udim commented Mar 16, 2018

Uh oh!

udim commented Mar 16, 2018

Uh oh!

udim commented Mar 16, 2018

Uh oh!

udim commented Mar 16, 2018

Uh oh!

holdenk commented Mar 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cclauss Mar 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cclauss Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

udim commented Mar 22, 2018

Uh oh!

aaltay commented Mar 22, 2018

Uh oh!

udim commented Mar 23, 2018

Uh oh!

aaltay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cclauss Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

cclauss Mar 17, 2018 •

edited

Loading

cclauss Mar 22, 2018 •

edited

Loading

cclauss Mar 23, 2018 •

edited

Loading