Skip to content
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

CI Azure: try removing setuptools workaround on windows #22905

Conversation

jeremiedbb
Copy link
Member

Let's see if we still trigger the issue

@thomasjpfan
Copy link
Member

I suspect this fails randomly on windows. For example the scheduled wheel builder job sometimes succeeds and sometimes fails on windows: https://github.com/scikit-learn/scikit-learn/actions/workflows/wheels.yml?query=branch%3Amain+event%3Aschedule

@jeremiedbb
Copy link
Member Author

I think it's slightly different. The env var was to tell setuptools to use the legacy behavior of distutils because distutils introduced a breaking change without taking into account the packages that monkeypatch it like numpy. The issue we expected was not random: numpy/numpy#17216. Distutils released a fallback mechanism but in the mean time we needed this env var.

If I'm right, it does not mean that the windows build is fixed. It only means that the fallback mechanism works, but we might still be building with the not thread safe CCompiler, (i.e. we still need #22693)

@jeremiedbb
Copy link
Member Author

The PR that introduced this is #22049 which seems to have been to quickly fix the CI because it was failing in every PR, which confirms it being not a random failure

@thomasjpfan
Copy link
Member

thomasjpfan commented Mar 20, 2022

I think there are two part of this issue:

  1. setup.py running at all: The original error came from setuptools and numpy both monkeypatching disutils that were incompatible. This should be fixed since distuils added a faillback mechanism for the 'env' argument, which is vendored by setuptools.
  2. setup.py compiling without failure on windows: From my understanding, we need a NumPy with this patch: BUG: Support env argument in CCompiler.spawn numpy/numpy#20650 to avoid rc.exe randomly not found (race condition?) pypa/setuptools#2212 .

Edit: This is exactly what you said 😅

@jeremiedbb
Copy link
Member Author

Now I don't understand why we have deprecation warnings from distutils that we don't have in other jobs...
E DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.

@ogrisel
Copy link
Member

ogrisel commented Mar 22, 2022

This deprecation warning is raised by:

C:\Miniconda\envs\testvenv\lib\site-packages\joblib\backports.py:36: in make_memmap

so this is a problem in joblib (not scikit-learn directly). I fixed it in a recently merged PR by backporting LooseVersion in joblib: https://github.com/joblib/joblib/pull/1272/files#diff-01a388465308c78a2759807da9e4aec087311a155205d483b957bb54382b0561 but this is not released yet.

@jeremiedbb
Copy link
Member Author

But if it's not released yet, why don't we trigger that in the other CI jobs ?

@jeremiedbb
Copy link
Member Author

@thomasjpfan I think we can merge this one

@thomasjpfan thomasjpfan merged commit 38132f2 into scikit-learn:main Apr 13, 2022
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Apr 29, 2022
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.

None yet

3 participants