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

Pin pylint version 2.2.2 #2698

Merged
merged 1 commit into from
Mar 1, 2019
Merged

Pin pylint version 2.2.2 #2698

merged 1 commit into from
Mar 1, 2019

Conversation

ghost
Copy link

@ghost ghost commented Feb 28, 2019

pylint is evolving, new versions of pylint add new diagnostics.
Pinning the pylint version ensures that the pylint behaviour in a
build environment is consistent between developers and CI and the
environment itself is re-creatable.

As pylint evolves the TVM project will need to periodically 'clean'
the code base w.r.t a new current version of pylint and then update
the pinned version.

@ghost
Copy link
Author

ghost commented Feb 28, 2019

These two commits need to land together because pylint expectation w.r.t indentation in this particular case changed somewhere between 1.9.6 and 2.2.2. The old pylint gripes about the new layout and the new pylint gripes about he old layout....

@ghost
Copy link
Author

ghost commented Feb 28, 2019

@tqchen I think this PR is self consistent, it fails in CI because the layout change necessary for pylint 2.2.2 is applied which fails for the current CI pylint==1.9.2 I had hoped that CI would pick up the change to Dockerfile.ci_lint automatically and install the new pylint, apparently that does not happen. When changes are made to the Dockerfile(s) and their dependent install scripts, how do the CI docker images get rebuilt?

@tqchen
Copy link
Member

tqchen commented Feb 28, 2019

The docker files get built and updated manually in a periodical way, so that images can be cached and reused. Please keep the change to be only the DOCKER script.

It is unfortunate to see that pylint gives non-backward compatible checks. We might need to find a way to get the files consistent to both pylint versions

Can you try to consolidate the following PR into one set of DOCKER changes?

Thanks

pylint is evolving, new version of pylint add new diagnostics.
Pinning the pylint version ensures that the pylint behaviour in a
build environment is consistent between developers and CI and the
environment itself is recreatable.

As pylint evolves the TVM project will need to periodically 'clean'
the code base w.r.t a new current version of pylint and then update
the pinned version.
@tqchen tqchen merged commit cab5af2 into apache:master Mar 1, 2019
@tqchen
Copy link
Member

tqchen commented Mar 1, 2019

This is merged, I will update the thread when the CI docker get updated

@ghost ghost deleted the pin-pylint branch March 1, 2019 08:33
@ghost
Copy link
Author

ghost commented Mar 1, 2019

@tqchen this revised version of the PR includes the pinning the pylint version in ubuntu_install_python_package.sh rather than removing the install completely. I've just realized this won't work because it pins a python2 and a python3 install of pylint, but pylint support in python 2 stopped at 1.9.4

Possible ways forward:

  1. If we don't actually need pylint via this script (I didn't see an explicit use case), then drop the install. This route hinges around your earlier concern that we may have demo? docker images dependent on having pylint...

  2. Pin TVM use of pylint at 1.9.4 in all three locations, until such time as the project decides to drop python2 support.

Unfortunately I don;t think we can pursue a direction in which python3 gets current pylint and python2 gets locked to 1.9.4 (with slighly more limited diagnostic capability) because we have already seen an instance of code layout where 1.9.4 and 2.2.2 are incompatible.

Thoughts?

@tqchen
Copy link
Member

tqchen commented Mar 1, 2019

Let us pin pylint to 1.9.4 for now then. The current lint info is relatively sufficient for most cases. We can also revive a python3 timeline discussion topic, but it will might a while

@yzhliu yzhliu mentioned this pull request Mar 2, 2019
28 tasks
@ghost
Copy link
Author

ghost commented Mar 4, 2019

New PR #2727 pins pylint to 1.9.4

bwasti pushed a commit to facebookexperimental/tvm that referenced this pull request Mar 6, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Mar 9, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Mar 12, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Mar 12, 2019
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.

2 participants