Skip to content

Conversation

@bhack
Copy link
Contributor

@bhack bhack commented Apr 3, 2021

This a first PR to try to supersed pylint_allowlist and custom pattern matching scripts for a less critical integration of our pylint linting with third party tools (e.g. IDE) and enable a potential pre-commit hook that we want to officially distribute for linting.

I've upgraded .pylintrc with the info that we currently have in the pattern matching:

# Report only what we care about
# Ref https://pylint.readthedocs.io/en/latest/technical_reference/features.html
# E: all errors
# W0311 bad-indentation
# W0312 mixed-indentation
# C0330 bad-continuation
# C0301 line-too-long
# C0326 bad-whitespace
# W0611 unused-import
# W0622 redefined-builtin
grep -E '(\[E|\[W0311|\[W0312|\[C0330|\[C0301|\[C0326|\[W0611|\[W0622)' ${OUTPUT_FILE} > ${ERRORS_FILE}
# Split the pylint reported errors into permitted ones and those we want to
# block submit on until fixed.
# We use `${ALLOW_LIST_FILE}` to record the errors we temporarily accept. Goal
# is to make that file only contain errors caused by difference between
# internal and external versions.
ALLOW_LIST_FILE="${SCRIPT_DIR}/pylint_allowlist"

Then I used pylint-silent to patch files.

@google-ml-butler google-ml-butler bot added the size:L CL Change Size: Large label Apr 3, 2021
@google-ml-butler google-ml-butler bot requested a review from joker-eph April 3, 2021 20:23
@google-cla google-cla bot added the cla: yes label Apr 3, 2021
@bhack bhack requested a review from caisq as a code owner April 4, 2021 20:25
@bhack
Copy link
Contributor Author

bhack commented Apr 4, 2021

/cc @mihaimaruseac @angerson

@gbaned gbaned self-assigned this Apr 5, 2021
@bhack
Copy link
Contributor Author

bhack commented Apr 5, 2021

We have now 14 residual pylint linting errors that I cannot reproduce in our official Docker devel image (for the command see #48291).
What is the Ubuntu Sanity image?

Do we want to use the official devel image in the CI to be sure that the user/contributors dev environment is aligned with the CI Sanity pass in github?

for r, g, b in rgb_flat
])
hsv_np = hsv_np.reshape(4, 4, 4, 3)
hsv_np = hsv_np.reshape(4, 4, 4, 3) # pylint: disable=too-many-function-args
Copy link
Contributor Author

Choose a reason for hiding this comment

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

E.g. We are tracking this type of notifications with #41873

Copy link

@mdanatg mdanatg left a comment

Choose a reason for hiding this comment

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

This is good, it gives us a better indication of where the flagged things are. Cleaning them up one by one (were possible), would make useful future contributions.

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Apr 5, 2021
@gbaned gbaned removed kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Apr 5, 2021
@gbaned
Copy link
Contributor

gbaned commented Apr 5, 2021

@bhack Can you please address Ubuntu Sanity errors? Thanks!

@bhack
Copy link
Contributor Author

bhack commented Apr 5, 2021

I need to understand what Is the reference Sanity Docker Image.
I prefer to have in CI our official Docker devel image so that we are sure to have the same CI and contributor/developer experience

@bhack
Copy link
Contributor Author

bhack commented Apr 26, 2021

I've opened tensorflow/build#27 in the SIG-build repository so that they can compare ci_sanity.sh VS pylint results on this branch with Github Action.

@gbaned
Copy link
Contributor

gbaned commented Apr 30, 2021

@mihaimaruseac Can you please take a look on the above comments from @bhack. Thanks!

@bhack
Copy link
Contributor Author

bhack commented Apr 30, 2021

@gbaned I've already talked with @mihaimaruseac just yesterday and he his still busy.

@bhack
Copy link
Contributor Author

bhack commented Apr 30, 2021

Also we are waiting for a comment at tensorflow/build#27 (comment) for the Ubuntu Sanity VM.

@angerson
Copy link
Contributor

I checked the environment for the machine that's running ci_sanity (you can actually see it in ci_sanity results for new PRs):

absl-py==0.8.1
appdirs==1.4.3
astroid==2.5.6
attrs==19.3.0
cachetools==3.1.1
certifi==2020.12.5
chardet==4.0.0
distlib==0.3.0
filelock==3.0.12
future==0.18.2
gast==0.2.2
google-auth==1.9.0
google-auth-oauthlib==0.4.1
grpcio==1.25.0
h5py==2.10.0
idna==2.10
isort==5.8.0
joblib==0.14.1
Keras-Preprocessing==1.1.0
lazy-object-proxy==1.6.0
Markdown==3.1.1
mccabe==0.6.1
mock==3.0.5
numpy==1.14.5
oauthlib==3.1.0
portpicker==1.3.1
protobuf==3.11.1
pyasn1==0.4.8
pyasn1-modules==0.2.7
pycurl==7.43.0
pygobject==3.20.0
pylint==2.7.2
python-apt==1.1.0b1+ubuntu0.16.4.11
PyYAML==3.11
requests==2.25.1
requests-oauthlib==1.3.0
rsa==4.0
scikit-learn==0.22
scipy==1.4.0
screen-resolution-extra==0.0.0
six==1.12.0
tb-nightly==2.1.0a20191106
tf-estimator-nightly==2.0.0.dev2019121609
toml==0.10.2
typing==3.7.4.1
unattended-upgrades==0.1
urllib3==1.26.4
virtualenv==20.0.5
Werkzeug==0.16.0
wrapt==1.12.1
xkit==0.0.0

check whether pylint is available or not.

pylint 2.7.2
astroid 2.5.6
Python 3.8.9 (default, Apr  3 2021, 01:02:10)
[GCC 5.4.0 20160609]

@bhack
Copy link
Contributor Author

bhack commented May 18, 2021

The last remaining issue for this PR is how we could align a Little bit that numpy version with the version currently in our official Docker devel https://github.com/tensorflow/tensorflow/blob/master/tensorflow/tools/dockerfiles/dockerfiles/devel-cpu.Dockerfile#L87

Cause I don't know what reference env to suggest to contributors for local linting on par with CI Sanity.

@angerson angerson added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels May 26, 2021
@angerson angerson added the ready to pull PR ready for merge process label May 26, 2021
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label May 26, 2021
@kkimdev kkimdev removed their request for review May 26, 2021 23:04
copybara-service bot pushed a commit that referenced this pull request May 27, 2021
PiperOrigin-RevId: 376070925
Change-Id: I161036fd0826d1548d9eff9a39a131ece268a95f
@gbaned
Copy link
Contributor

gbaned commented May 27, 2021

Seems auto-merge is not happening but the changes are merged into master now, so we can close this. Thank you for the PR.

@gbaned gbaned closed this May 27, 2021
@bhack bhack deleted the pylint_silent branch May 27, 2021 12:35
@angerson
Copy link
Contributor

Thank you @gbaned. I had to make a number of extra changes internally to land this, but it's in now.

@bhack
Copy link
Contributor Author

bhack commented May 27, 2021

@angerson Thanks. Let see if we could review #48371 so that we have a reference local env for developers to run reproducible liniting.

@bhack
Copy link
Contributor Author

bhack commented May 27, 2021

It was merged but the Ubuntu sanity VM is still on numpy==1.14.5. As I told in #48294 (comment) this will fail the sanity check in the CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes size:L CL Change Size: Large stat:awaiting tensorflower Status - Awaiting response from tensorflower

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants