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

Add msvcp140.dll to Windows 64 bit wheels #24631

Merged
merged 2 commits into from Oct 14, 2022

Conversation

cmarmo
Copy link
Member

@cmarmo cmarmo commented Oct 10, 2022

Reference Issues/PRs

Addresses #24612 together with #24627

What does this implement/fix? Explain your changes.

Vendor a missing library to the Windows 64bit wheel.

Any other comments?

Was previously in #24446.

@cmarmo
Copy link
Member Author

cmarmo commented Oct 10, 2022

Note that the current failure is no longer the missing library but a test failure as reported in #24446.

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.

According to #24612 (comment), there is a way to remove our dependency on msvcp140.dll. In the long term, I think that is the best option.

In the short term, I am okay with PR vendoring msvcp140.dll, so we can move forward with the Python 3.11 wheels.

The failure on Windows Python 3.10 is concerning. I see that is also happening in the Python 3.11 PR. If no one picks this up, then I'll start investigating it tomorrow.

@cmarmo
Copy link
Member Author

cmarmo commented Oct 11, 2022

The failure on Windows Python 3.10 is concerning. I see that is also happening in the Python 3.11 PR. If no one picks this up, then I'll start investigating it tomorrow.

Thanks @thomasjpfan. Please note that it is the same failure than in linux python 3.11 and MacosX python3.11.

@thomasjpfan
Copy link
Member

I was able to reproduce the Linux failure locally with debug symbols. Here is the backtrace:

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007ffff7de3859 in __GI_abort () at abort.c:79
#2  0x00007ffff7e4e26e in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7ffff7f78298 "%s\n") at ../sysdeps/posix/libc_fatal.c:155
#3  0x00007ffff7e562fc in malloc_printerr (str=str@entry=0x7ffff7f7a600 "free(): invalid next size (fast)") at malloc.c:5347
#4  0x00007ffff7e57bac in _int_free (av=0x7ffff7fadb80 <main_arena>, p=0x4893f50, have_lock=0) at malloc.c:4249
#5  0x00007ffff5afe2e1 in PyDataMem_UserFREE () from /home/thomasfan/Desktop/scikit-learn-5/.venv/lib/python3.11/site-packages/numpy/core/_multiarray_umath.cpython-311-x86_64-linux-gnu.so
#6  0x00007ffff5b0178e in array_dealloc () from /home/thomasfan/Desktop/scikit-learn-5/.venv/lib/python3.11/site-packages/numpy/core/_multiarray_umath.cpython-311-x86_64-linux-gnu.so

There is an error with malloc: "free(): invalid next size (fast)". pytest crashes with the same error, but at different places during a pytest run.

I'll continue investigating tomorrow.

@thomasjpfan
Copy link
Member

I looked into this issue more and have not found the root cause yet.

If others are interested in debugging, I created https://github.com/thomasjpfan/scikit-learn-debugging-python3.11 that details the steps for reproducing the issue on Linux.

@cmarmo
Copy link
Member Author

cmarmo commented Oct 13, 2022

For the sake of completeness, when run with scipy 1.10-dev0 at the beginning of #24446 , linux and MacosX did not fail, Windows64 failed on another test, no issues about threads there.

@thomasjpfan
Copy link
Member

thomasjpfan commented Oct 13, 2022

Thank you for the hint that SciPy dev works. The issue is in SciPy and the fix is already in SciPy's main branch. I opened scipy/scipy#17224 to track the issue. We need to wait for the fix to be backported to SciPy 1.9.3.

@cmarmo
Copy link
Member Author

cmarmo commented Oct 13, 2022

The issue is in SciPy and the fix is already in SciPy's main branch. I opened scipy/scipy#17224 to track the issue. We need to wait for the fix to be backported to SciPy 1.9.3.

Wonderful! Thanks for your investigations!
Could this one be merged in the meanwhile, so I can consistently clean #24446? Thanks!

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.

As noted in #24612 (comment), we can work toward not needing msvcp140.dll in a follow up PR.

In the short them, I am okay with vendoring it for now. LGTM

Edit: According to scipy/scipy#17191, we can not depend no SciPy to load the DLL anymore.

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.

Temporary vendoring this DLL works for me. Thank you, @cmarmo!

I think it would be nice to identify what each DLL is needed exactly for to see if we could not depend on them on the long term.

@jjerphan jjerphan merged commit 5c897f1 into scikit-learn:main Oct 14, 2022
@cmarmo cmarmo deleted the whell-winamd64 branch October 14, 2022 18:04
@thomasjpfan thomasjpfan added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label Oct 24, 2022
@thomasjpfan thomasjpfan added this to the 1.1.3 milestone Oct 24, 2022
glemaitre pushed a commit to glemaitre/scikit-learn 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 Packaging 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

3 participants