MNT properly activate the env in the linting CI by rth · Pull Request #17177 · scikit-learn/scikit-learn

rth · 2020-05-10T15:36:10Z

Fixes linting CI job on azure.

~~Currently it always returns a green status because the env fails to activate and the error is silenly ignored.~~

The error on master is,

CommandNotFoundError: Your shell has not been properly configured to use 'conda activate'.
To initialize your shell, run

    $ conda init <SHELL_NAME>

Currently supported shells are:
  - bash
  - fish
  - tcsh
  - xonsh
  - zsh
  - powershell

See 'conda init --help' for more information and options.

IMPORTANT: You may need to close and restart your shell after running 'conda init'.

Originally fix added in #17053 see discussion there for more details.

rth · 2020-05-10T15:43:25Z

A quick review @NicolasHug ? The azure linting job is not checking anything.

rth · 2020-05-10T15:45:02Z

sklearn/utils/tests/test_utils.py

-                                           (np.float32("nan"), True),
-                                           (np.float64("nan"), True),
+                                           (np.float(float("nan")), True),
+                                           (np.float32(float("nan")), True),


The signature of these functions is,

class float64(floating, builtins.float)

it does seem to work with strings, probably because float is called on them internally, but technically that's not correct.

rth · 2020-05-10T15:45:33Z

azure-pipelines.yml

        fi
      displayName: Run linting
    - bash: |
+        set -ex


Print executed commands and exit on first failure.

NicolasHug

Thanks @rth

I'm happy with the CI update, but I can't say I'm thrilled by the need to update some perfectly working code for mypy to be happy :/

Do we actually need the changes in the code, considering that mypy was working and running fine before?

NicolasHug · 2020-05-10T15:58:46Z

sklearn/cluster/_agglomerative.py

+# mypy error: Module 'sklearn.cluster' has no attribute '_hierarchical_fast'
+from . import _hierarchical_fast as _hierarchical  # type: ignore


mypy doesn't like Cython files?

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.

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?

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)

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..

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.

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.

NicolasHug · 2020-05-10T15:59:10Z

sklearn/utils/tests/test_utils.py

-                                           (np.float("nan"), True),
-                                           (np.float32("nan"), True),
-                                           (np.float64("nan"), True),
+                                           (np.float(float("nan")), True),


that makes me sad lol

Remplaced with np.float64(np.nan) which should be better, I think?

rth · 2020-05-10T16:12:44Z

I'm happy with the CI update, but I can't say I'm thrilled by the need to update some perfectly working code for mypy to be happy :/

I know, running mypy on the full repo also worked fine. I had to make changes as I was getting errors with pre-commit (and the raised errors do make sense).

thomasjpfan

I have always been concerned with how much knowledge one needs to work around these mypy linting errors. (One can see the mypy errors we are ignoring by greping for # mypy error)

This PR is needed to get the linter to work. Thank you @rth !

NicolasHug · 2020-05-10T17:22:22Z

Thanks @rth

rth added 2 commits May 10, 2020 17:28

Fix linting on azure

c53a4fa

Fix mypy errors

7f04efd

rth mentioned this pull request May 10, 2020

[MRG] Remove class support check estimator and parametrize_with_checks #17134

Merged

github-actions bot added module:cluster module:manifold module:utils labels May 10, 2020

rth commented May 10, 2020

View reviewed changes

NicolasHug reviewed May 10, 2020

View reviewed changes

Use np.float64(np.nan)

041b39e

thomasjpfan approved these changes May 10, 2020

View reviewed changes

NicolasHug approved these changes May 10, 2020

View reviewed changes

NicolasHug changed the title ~~Fix azure linting~~ MNT properly activate the env in the linting CI May 10, 2020

NicolasHug merged commit 192109a into scikit-learn:master May 10, 2020

gio8tisu pushed a commit to gio8tisu/scikit-learn that referenced this pull request May 15, 2020

MNT properly activate the env in the linting CI (scikit-learn#17177)

0cc17e4

emmanuelle mentioned this pull request Jun 18, 2020

add type annotations in some modules in skimage.segmentation scikit-image/scikit-image#4794

Draft

4 tasks

viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020

MNT properly activate the env in the linting CI (scikit-learn#17177)

73173d2

jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020

MNT properly activate the env in the linting CI (scikit-learn#17177)

d91d140

		# mypy error: Module 'sklearn.cluster' has no attribute '_hierarchical_fast'
		from . import _hierarchical_fast as _hierarchical # type: ignore

Uh oh!

Conversation

rth commented May 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rth commented May 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rth May 10, 2020 • 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!

rth commented May 10, 2020

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

NicolasHug commented May 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rth commented May 10, 2020 •

edited

Loading

rth commented May 10, 2020 •

edited

Loading

rth May 10, 2020 •

edited

Loading