8000 MNT properly activate the env in the linting CI by rth · Pull Request #17177 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

MNT properly activate the env in the linting CI #17177

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 10, 2020
Merged

MNT properly activate the env in the linting CI #17177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c53a4fa
Fix linting on azure
rth May 10, 2020
7f04efd
Fix mypy errors
rth May 10, 2020
041b39e
Use np.float64(np.nan)
rth May 10, 2020
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
9 changes: 6 additions & 3 deletions azure-pipelines.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,27 +18,30 @@ jobs:
- bash: sudo chown -R $USER $CONDA
displayName: Take ownership of conda installation
- bash: |
set -ex
conda create --name flake8_env --yes python=3.8
conda activate flake8_env
source activate flake8_env
pip install flake8 mypy==0.770
displayName: Install flake8
- bash: |
set -ex
if [[ $BUILD_SOURCEVERSIONMESSAGE =~ \[lint\ skip\] ]]; then
# skip linting
echo "Skipping linting"
exit 0
else
conda activate flake8_env
source activate flake8_env
./build_tools/circle/linting.sh
fi
displayName: Run linting
- bash: |
set -ex
Copy link
Member Author

Choose a reason for hiding this comment

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

Print executed commands and exit on first failure.

if [[ $BUILD_SOURCEVERSIONMESSAGE =~ \[lint\ skip\] ]]; then
# skip linting
echo "Skipping linting"
exit 0
else
conda activate flake8_env
source activate flake8_env
mypy sklearn/ --ignore-missing-imports
fi
displayName: Run mypy
Expand Down
3 changes: 2 additions & 1 deletion sklearn/cluster/_agglomerative.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@
from ..neighbors import DistanceMetric
from ..neighbors._dist_metrics import METRIC_MAPPING

from . import _hierarchical_fast as _hierarchical
# mypy error: Module 'sklearn.cluster' has no attribute '_hierarchical_fast'
from . import _hierarchical_fast as _hierarchical # type: ignore
Comment on lines +24 to +25
Copy link
Member

Choose a reason for hiding this comment

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

mypy doesn't like Cython files?

Copy link
Member Author
@rth rth May 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, normally one currently has to write stub files for cython extensions since they don't expose annotations (see e.g. scipy/scipy#11936). I'm hoping this could be done automatically in the future though, since Cython should have all relevant information; possibly cython/cython#2552.

Edit: though actually maybe the issues with imports we are seeing are different. Could be worth investigating.

Copy link
Member

Choose a reason for hiding this comment

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

When mypy runs on the source (before anything is built), my understanding is that there is no way for mypy to find the module. Is this correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, indeed. For most Cython imports errors will be ignored due to the --ignore-missing-import flag, however not when importing whole modules for some reason,
python/mypy#8575 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Also I'm getting these errors locally with scikit-learn correctly build. E.g. without this change,

$ mypy --ignore-missing-imports sklearn/cluster/_agglomerative.py 
sklearn/cluster/_agglomerative.py:25: error: Module 'sklearn.cluster' has no attribute '_hierarchical_fast'
Found 1 error in 1 file (checked 1 source file)
$ mypy --ignore-missing-imports sklearn
Success: no issues found in 599 source files

I don't really understand why the full run succeeds..

Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand why the full run succeeds..

Probably unrelated, but lately I've found that git diff upstream/master -u -- "*.py" | flake8 --diff passes on some branches while build_tools/circle/linting.sh (used by the CI) doesn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably unrelated, but lately I've found that git diff upstream/master -u -- "*.py" | flake8 --diff passes on some branches while build_tools/circle/linting.sh (used by the CI) doesn't.

Yes, linting the diff has issues #11336 (comment) and I would like to start dealing with it once we are done with the release.

from ._feature_agglomeration import AgglomerationTransform
from ..utils._fast_dict import IntFloatDict
from ..utils.fixes import _astype_copy_false
Expand Down
3 changes: 2 additions & 1 deletion sklearn/manifold/_t_sne.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@
from ..utils.validation import _deprecate_positional_args
from ..decomposition import PCA
from ..metrics.pairwise import pairwise_distances
from . import _utils
# mypy error: Module 'sklearn.manifold' has no attribute '_utils'
from . import _utils # type: ignore
# mypy error: Module 'sklearn.manifold' has no attribute '_barnes_hut_tsne'
from . import _barnes_hut_tsne # type: ignore

Expand Down
6 changes: 3 additions & 3 deletions sklearn/utils/tests/test_uti 6D47 ls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,9 +657,9 @@ def test_print_elapsed_time(message, expected, capsys, monkeypatch):

@pytest.mark.parametrize("value, result", [(float("nan"), True),
(np.nan, True),
(np.float("nan"), True),
(np.float32("nan"), True),
(np.float64("nan"), True),
(np.float(np.nan), True),
(np.float32(np.nan), True),
(np.float64(np.nan), True),
(0, False),
(0., False),
(None, False),
Expand Down
3 changes: 2 additions & 1 deletion sklearn/utils/validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
from distutils.version import LooseVersion
from inspect import signature, isclass, Parameter

from numpy.core.numeric import ComplexWarning
# mypy error: Module 'numpy.core.numeric' has no attribute 'ComplexWarning'
from numpy.core.numeric import ComplexWarning # type: ignore
import joblib

from contextlib import suppress
Expand Down
0