Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Fix flakey pylint CI failures #16462

Merged
merged 7 commits into from
Oct 15, 2019
Merged

Fix flakey pylint CI failures #16462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0b8356a
Change pylint parallel jobs from 8 to 1
DickJC123 Oct 12, 2019
cea4ffa
Non-functional tweeks to keep pylint happy
DickJC123 Oct 13, 2019
9027fca
Disable pylint too-many-lines, duplicate-code, cyclic-import
DickJC123 Oct 13, 2019
e34e599
Trigger CI
DickJC123 Oct 13, 2019
b28f17d
Trigger CI
DickJC123 Oct 14, 2019
df2b258
Fighting unrelated CI failures. Trigger CI.
DickJC123 Oct 14, 2019
b7958d0
Fighting unrelated CI failures. Trigger CI.
DickJC123 Oct 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions ci/other/pylintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ persistent=yes
load-plugins=

# Use multiple processes to speed up Pylint.
jobs=8
jobs=1

# Allow loading of arbitrary C extensions. Extensions are imported into the
# active Python interpreter and may run arbitrary code.
Expand Down Expand Up @@ -113,7 +113,10 @@ disable=
too-many-instance-attributes,
too-many-locals,
too-many-public-methods,
too-many-statements
too-many-statements,
too-many-lines,
duplicate-code,
cyclic-import
Copy link
Contributor

Choose a reason for hiding this comment

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

Is cyclic import not a valid thing to consider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, however there are over 90 cyclic-import errors reported. Analyzing the python software architecture and possibly refactoring it to eliminate cyclic imports is beyond the scope of this PR. I'll file an issue to see if one of the python major code owners will look into this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, thanks


# disable=unicode-builtin,delslice-method,using-cmp-argument,setslice-method,dict-view-method,parameter-unpacking,range-builtin-not-iterating,print-statement,file-builtin,old-raise-syntax,basestring-builtin,execfile-builtin,indexing-exception,import-star-module-level,coerce-method,long-builtin,old-ne-operator,old-division,no-absolute-import,raw_input-builtin,old-octal-literal,oct-method,xrange-builtin,hex-method,unpacking-in-except,nonzero-method,raising-string,intern-builtin,reload-builtin,metaclass-assignment,cmp-method,filter-builtin-not-iterating,apply-builtin,map-builtin-not-iterating,next-method-called,unichr-builtin,buffer-builtin,dict-iter-method,input-builtin,coerce-builtin,getslice-method,useless-suppression,standarderror-builtin,zip-builtin-not-iterating,suppressed-message,cmp-builtin,backtick,long-suffix,reduce-builtin,round-builtin

Expand Down
5 changes: 2 additions & 3 deletions python/mxnet/numpy_op_signature.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import sys
import warnings
import inspect
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for putting the efforts on taming pylint. I'm curious why this reorganization of the code would make the pylint error disappear?

Copy link
Contributor Author

@DickJC123 DickJC123 Oct 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

So, first I should mention that as I developed the PR on my local machine, I was seeing slightly different pylint output than the upstream CI. This could be due to a different pylint version. In the comment that follows, I'll refer to the pylint command I posted above in the PR description as <pylint_cmd>:

With the import inspect statement in its original location, I saw:

$ <pylint_cmd> | grep numpy_op_signature
************* Module mxnet.numpy_op_signature
python/mxnet/numpy_op_signature.py:C0415:63,4: _register_op_signatures: Import outside toplevel (inspect)

With the import inspect statement moved, no such output line was generated. Since I was touching the file numpy_op_signature.py anyway to eliminate the E0402 error, I thought I'd make the file totally clean w.r.t pylint by moving the import inspect.

Introducing the line op_module = root_module was the fix that really mattered (eliminated the E0402 error). Since it didn't change the logic of the code materially, I view this as working around the quirks of pylint.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, pylint is pinned in some cases inside the ci folder:

install/requirements
30:pylint==2.3.1; python_version >= '3.0'

We should use just one version of pylint...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CI appears to pull in pylint 2.3.1. The most current pylint is 2.4.2.

from . import _numpy_op_doc
from . import numpy as mx_np
from . import numpy_extension as mx_npx
Expand All @@ -38,13 +39,12 @@ def _get_builtin_op(op_name):
return None

submodule_name = _get_op_submodule_name(op_name, op_name_prefix, submodule_name_list)
op_module = root_module
if len(submodule_name) > 0:
op_module = getattr(root_module, submodule_name[1:-1], None)
if op_module is None:
raise ValueError('Cannot find submodule {} in module {}'
.format(submodule_name[1:-1], root_module.__name__))
else:
op_module = root_module

op = getattr(op_module, op_name[(len(op_name_prefix)+len(submodule_name)):], None)
if op is None:
Expand All @@ -61,7 +61,6 @@ def _register_op_signatures():
.format(str(sys.version)))
return

import inspect
for op_name in dir(_numpy_op_doc):
op = _get_builtin_op(op_name)
if op is not None:
Expand Down