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 Remove Windows 32 bit support #24627

Merged
merged 6 commits into from Oct 13, 2022
Merged

Conversation

thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Oct 10, 2022

Reference Issues/PRs

Addresses #24612 for Windows 32 bit

What does this implement/fix? Explain your changes.

Using PIP_PREFER_BINARY=1 configures pip to prefer older binary packages over building from source.

Any other comments?

This PR only fixes the Windows 32 bit issue and not the Windows 64 bit issue.

@thomasjpfan thomasjpfan changed the title FIX Prefer binary when building scikit-learn wheels FIX Prefer binary when building scikit-learn wheels to fix Windows 32 bit wheels Oct 10, 2022
@cmarmo
Copy link
Member

cmarmo commented Oct 10, 2022

Thanks @thomasjpfan .... I guess this is the answer to my question (see comment)

Please note that the amd64 Windows issue is fixed by adding a new dll as in https://github.com/scikit-learn/scikit-learn/pull/24446/files#diff-e0a07d85249fa03c06895043f2d57076b4a1a30b195631e1682b5470db6a5ac3 : should I offer a separate PR for that?

@thomasjpfan
Copy link
Member Author

Please note that the amd64 Windows issue is fixed by adding a new dll as in #24446 (files) : should I offer a separate PR for that?

Yea, I like the idea of opening a separate PR to fix the current Windows 64bit wheels. It should also make the Python 3.11 PR smaller.

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.

LGTM. Thanks for the fix.

@ogrisel
Copy link
Member

ogrisel commented Oct 10, 2022

Although in retrospect, won't this hide problems faced by our users that use the pip commandline with the default configuration?

Still getting a more stable wheel CI is nice. So still +0 for merging this.

@cmarmo
Copy link
Member

cmarmo commented Oct 10, 2022

Although in retrospect, won't this hide problems faced by our users that use the pip commandline with the default configuration?

This is an issue when producing scikit-learn wheels for windows 32.
The cleanest way to fix it is to specify dependencies in the pyproject.toml, but I couldn't find a way to specify the bitness of the system.
Let merge this and let windows 32bit wheels slowly die as scipy is doing.

@thomasjpfan
Copy link
Member Author

Although in retrospect, won't this hide problems faced by our users that use the pip commandline with the default configuration?

Here are some options:

  1. Document the work around it
  2. Introduce an upper bound for SciPy on Windows 32bit
  3. Wait for --only-binary be the default: Speculative: --only-binary by default? pypa/pip#9140
  4. Drop Windows 32bit. Since SciPy 1.9.2 no ships Windows 32bit wheels, then we should not either.

I can get behind Option 4 and dropping Windows 32bit.

@cmarmo
Copy link
Member

cmarmo commented Oct 10, 2022

I can get behind Option 4 and dropping Windows 32bit.

+1

@thomasjpfan thomasjpfan changed the title FIX Prefer binary when building scikit-learn wheels to fix Windows 32 bit wheels CI Remove Windows 32 bit support Oct 11, 2022
@thomasjpfan
Copy link
Member Author

I updated this PR to remove Windows 32 bit support. If SciPy is not building for it anymore, then I think we can follow them.

Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

There are Windows 32-specific adaptation left in some part of the code-base.

For instance, there are references and specific setup:

  • in conda lock-files' fixtures:
    # The Windows 32bit build use 3.8.10. No cross-compilation support for
    # pip-compile, we are going to assume the pip lock file on a Linux
    # 64bit machine gives appropriate versions
    "python_version": "3.8.10",
    "package_constraints": {
    "scipy": "1.9.1", # 1.9.2 not available for 32 bit Windows
    },
  • in Windows wheels testing:
    if [[ "$BITNESS" == "32" ]]; then
    # 32-bit architectures use the regular
    # test command (outside of the minimal Docker container)
    cp $CONFTEST_PATH $CONFTEST_NAME
    python -c "import sklearn; sklearn.show_versions()"
    pytest --pyargs sklearn
  • for the 32bit OpenBLAS which seems to use Windows (inferred from pip-windows)?
    py38_pip_openblas_32bit:
    DISTRIB: 'pip-windows'
    PYTHON_VERSION: '3.8'
    PYTHON_ARCH: '32'
    LOCK_FILE: ./build_tools/azure/py38_pip_openblas_32bit_lock.txt
    SKLEARN_TESTS_GLOBAL_RANDOM_SEED: '8' # non-default seed

    elif [[ "$DISTRIB" == "pip-windows" ]]; then
    source $VIRTUALENV/Scripts/activate
  • for compilers flags for handling OpenMP:
    if sys.platform == "win32" and ("icc" in compiler or "icl" in compiler):
    return ["/Qopenmp"]
    elif sys.platform == "win32":
    return ["/openmp"]
  • for this test adaptation:
    if sys.platform == "win32": # fake parallelism for win32
    import joblib
    _mp = joblib.parallel.multiprocessing
    joblib.parallel.multiprocessing = None
    try:
    spca = MiniBatchSparsePCA(
    n_components=3, n_jobs=2, alpha=alpha, random_state=0
    )
    U2 = spca.fit(Y).transform(Y)
    finally:
    joblib.parallel.multiprocessing = _mp
  • for a test skipping reason:
    elif sys.platform.startswith("win32"):
    reason = (
    "doctests are not run for Windows because numpy arrays "
    "repr is inconsistent across platforms."
    )

Should some of them be removed?

@thomasjpfan
Copy link
Member Author

I think "win32" means "Windows" and does not specify the bitness. In Python, the platform is defined here: https://github.com/python/cpython/blob/0895c2a066c64c84cab0821886dfa66efc1bdc2f/PC/pyconfig.h#L288-L292

@jjerphan
Copy link
Member

How confusing. 🤷

Then…

Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

I am fine with the 32 bit windows support as long as scipy does not support it anymore.

I think we should still have at least one 32 bit runtime in our CI build (but this is the case with linux) to make sure that our Cython code is not bitness dependent to be able to support platforms such as WASM in the future.

azure-pipelines.yml Show resolved Hide resolved
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.

LGTM!

@ogrisel ogrisel merged commit 16f4fb8 into scikit-learn:main Oct 13, 2022
@glemaitre glemaitre added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label Oct 26, 2022
@glemaitre glemaitre added this to the 1.1.3 milestone Oct 26, 2022
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Oct 26, 2022
glemaitre pushed a commit that referenced this pull request Oct 26, 2022
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build / CI To backport PR merged in master that need a backport to a release branch defined based on the milestone.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants