Skip to content

MNT Use conda-forge instead of the main channel in CI #21337

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.

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 17 commits into from
Oct 18, 2021

Conversation

adrinjalali
Copy link
Member

@adrinjalali adrinjalali commented Oct 15, 2021

Somehow anaconda has patched python in the main channel that it breaks our CI. We should be using conda-forge instead, maintainers don't maintain what goes on the main channel.

Fixes #21324, #21325, #21326, #21327
Closes #21329

@adrinjalali
Copy link
Member Author

@glemaitre seems like our CI uses the main channel and not conda-forge, testing conda-forge here. We shouldn't be using the main channel anyway.

@adrinjalali adrinjalali changed the title [DEBUG] testing CI conda channels Use conda-forge instead of the main channel in CI Oct 15, 2021
@adrinjalali adrinjalali changed the title Use conda-forge instead of the main channel in CI MNT Use conda-forge instead of the main channel in CI Oct 15, 2021
@ogrisel
Copy link
Member

ogrisel commented Oct 15, 2021

I am fine to only run the docstests on conda-forge CI jobs if that allows to not waste time trying to understand the cause of the scientific notations in the doctests (#21324).

However, it's a problem if we no longer run the scikit-learn tests at all against numpy and scipy from the Anaconda main channel as I suspect a significant fraction of our users still do and problems such as #21326 are more serious than a wrong print format.

@adrinjalali
Copy link
Member Author

Anaconda main channel is not maintained by maintainers of the packages, we have pretty much no control over them. I don't want us to spend any energy on a system which is a black box and we don't control. Unless we're paid to maintain against packages in the main channel, I don't think we should be spending time and energy on it.

@adrinjalali
Copy link
Member Author

@glemaitre
Copy link
Member

Regarding the numpy validation, I added the PR of @thomasjpfan but without merging main before merging. In the meanwhile, I assume that we added a function that does not pass the numpydoc validation. @ogrisel is doing a PR.

@adrinjalali
Copy link
Member Author

So should we merge this and @ogrisel 's PR pulls this to make the CI green, or the other way around?

@glemaitre
Copy link
Member

So should we merge this and @ogrisel 's PR pulls this to make the CI green, or the other way around?

Merge @ogrisel PR first, "rebase" this one, and try to understand the segfault:
https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=33615&view=logs&jobId=78a0bf4f-79e5-5387-94ec-13e67d216d6e&j=cdb9ac20-7056-5f9a-0c7e-7c443f61f8a0&t=9e067a35-42f8-5d7a-14d3-926d2bc32e73

This one is fresh and I never seen it before.

@adrinjalali
Copy link
Member Author

I think I've seen that crash, and a restart of the CI fixed the issue. But we'll check once Olivier's PR is pulled here.

@adrinjalali
Copy link
Member Author

should we merge this one, w/o scipy1.2 upgrade, and fix that issue separately?

This reverts commit 139fde4.
@adrinjalali
Copy link
Member Author

Note that both failures are on py3.7 which we're dropping for the next release

@lesteve
Copy link
Member

lesteve commented Oct 15, 2021

I can reproduce the test_rank_deficient_design failure locally on Python 3.9 see #21340.

@ogrisel
Copy link
Member

ogrisel commented Oct 15, 2021

Some of the original issues have gone away magically (probably caused by a transient problem in the conda main channel packages). I am not sure this PR is needed anymore.

@adrinjalali
Copy link
Member Author

I would be happier if we actually test against conda-forge, something people can use for free without having to pay, rather than conda/main. I think our duty is to those users more than conda/main users. Happy to have conda/main tests in a cron job to report issues to anaconda if we want, but I think we should be testing against conda-forge.

@ogrisel
Copy link
Member

ogrisel commented Oct 15, 2021

We do already test against conda-forge dependencies (and pypi.org dependencies) in other builds.

I think it's good to test against various popular package providers to shake our test assertions and make our test less brittle.

I agree that from time to time this causes our CI to fail for transient reasons outside of our control but I think it's rare enough we can bear it, no?

@adrinjalali
Copy link
Member Author

I must be missing something, because I don't see anywhere us checking against conda-forge envs.

@jjerphan
Copy link
Member

I just have observed this problem with scientific notation for #20254, see this log.

@ogrisel
Copy link
Member

ogrisel commented Oct 15, 2021

I just have observed this problem with scientific notation for #20254, see this log.

I re-ran the failed job and it went away. This build probably failed just before the transient problem went away.

@ogrisel
Copy link
Member

ogrisel commented Oct 16, 2021

#21324 is happening again...

It is happening again

@ogrisel
Copy link
Member

ogrisel commented Oct 16, 2021

Instead of hardcoding conda-forge in the install.sh script, I updated the yaml config. I also took this opportunity to rename the entries of the build matrix to be more consistent and informative.

@ogrisel
Copy link
Member

ogrisel commented Oct 16, 2021

The segfault on pylatest_pip_openblas_pandas has already been reported here: #21339.

@ogrisel
Copy link
Member

ogrisel commented Oct 16, 2021

Weirdly enough we did not get the #21345 failure on this one...

@adrinjalali
Copy link
Member Author

@ogrisel I think your import of sklearn is too early in script here

@glemaitre glemaitre self-assigned this Oct 18, 2021
DISTRIB: 'conda'
CONDA_CHANNEL: 'defaults' # Anaconda main channel
Copy link
Member

@ogrisel ogrisel Oct 18, 2021

Choose a reason for hiding this comment

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

We need to use the anaconda main channel to test against scipy='min' (that is 1.1.0 currently) because the version shipped in conda-forge has a segfault in cdist.

It's probably not worth wasting time debugging or reporting this segfault because this is a really old version of scipy that we probably won't support anymore once we bump the dependency on Python 3.8 for instance.

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

+1 for merge.

@adrinjalali
Copy link
Member Author

I'm happy with the changes, thanks @ogrisel .

@glemaitre feel free to merge if you're happy.

@ogrisel ogrisel merged commit 5c1ad3f into scikit-learn:main Oct 18, 2021
@adrinjalali adrinjalali deleted the channel-test branch October 18, 2021 12:23
@glemaitre glemaitre mentioned this pull request Oct 23, 2021
10 tasks
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Oct 23, 2021
…1337)

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
glemaitre added a commit that referenced this pull request Oct 25, 2021
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
…1337)

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI docstring failure due to NumPy switch to scientific notation
5 participants