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

Jump to bottom
Merged
merged 3 commits into from
May 10, 2020
Merged

MNT properly activate the env in the linting CI #17177

merged 3 commits into from
May 10, 2020

Conversation

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

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

@rth rth mentioned this pull request May 10, 2020
Merged
@rth
Copy link
Member Author
rth commented May 10, 2020 •
edited
Loading

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

rth
rth commented May 10, 2020
View reviewed changes
sklearn/utils/tests/test_utils.py Outdated
(np.float32("nan"), True),
(np.float64("nan"), True),
(np.float(float("nan")), True),
(np.float32(float("nan")), True),
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

azure-pipelines.yml
./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.

NicolasHug
NicolasHug reviewed May 10, 2020
View reviewed changes
Copy link
Member
@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

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?

sklearn/cluster/_agglomerative.py
Comment on lines +24 to +25
# mypy error: Module 'sklearn.cluster' has no attribute '_hierarchical_fast'
from . import _hierarchical_fast as _hierarchical # type: ignore
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 err
8000
or 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.

sklearn/utils/tests/test_utils.py Outdated
(np.float("nan"), True),
(np.float32("nan"), True),
(np.float64("nan"), True),
(np.float(float("nan")), True),
Copy link
Member

Choose a reason for hiding this comment

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

that makes me sad lol

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@rth
Copy link
Member Author
rth commented May 10, 2020

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
thomasjpfan approved these changes May 10, 2020
View 8000 reviewed changes
Copy link
Member
@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

Thanks @rth

@NicolasHug NicolasHug changed the title Fix azure linting MNT properly activate the env in the linting CI May 10, 2020
@NicolasHug 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
@rth @gio8tisu
MNT properly activate the env in the linting CI (scikit-learn#17177)
0cc17e4
@emmanuelle emmanuelle mentioned this pull request Jun 18, 2020
Draft
4 tasks
viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020
@rth @viclafargue
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
@rth @jayzed82
MNT properly activate the env in the linting CI (scikit-learn#17177)
d91d140
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicolasHug NicolasHug NicolasHug approved these changes

@thomasjpfan thomasjpfan thomasjpfan approved these changes

Assignees
No one assigned
Labels
module:cluster module:manifold module:utils
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rth @NicolasHug @thomasjpfan
0