8000 Add msvcp140.dll to Windows 64 bit wheels by cmarmo · Pull Request #24631 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

Add msvcp140.dll to Windows 64 bit wheels #24631

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 2 commits into from
Oct 14, 2022
Merged

Add msvcp140.dll to Windows 64 bit wheels #24631

merged 2 commits into from
Oct 14, 2022

Conversation

cmarmo
Copy link
Contributor
@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 cmarmo mentioned this pull request Oct 10, 2022
Merged
@cmarmo
Copy link
Contributor 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.

@thomasjpfan thomasjpfan mentioned this pull request Oct 10, 2022
Closed
thomasjpfan
thomasjpfan reviewed Oct 11, 2022
View reviewed changes
Copy link
Member
@thomasjpfan thomasjpfan left a comment •
edited
Loading

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
Contributor 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
Contributor 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 •
edited
Loading

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
Contributor Author
cmarmo commented Oct 13, 2022 •
edited
Loading

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!

thomasjpfan
thomasjpfan approved these changes Oct 13, 2022
View reviewed changes 8000
Copy link
Member
@thomasjpfan thomasjpfan left a comment •
edited
Loading

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.

jjerphan
jjerphan approved these changes Oct 14, 2022
View reviewed changes
Copy link
Member
@jjerphan jjerphan left a comment •
edited
Loading

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
@glemaitre glemaitre mentioned this pull request Oct 18, 2022
Closed
@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
@cmarmo @glemaitre
Add msvcp140.dll to Windows 64 bit wheels (scikit-learn#24631)
f442184
glemaitre pushed a commit that referenced this pull request Oct 26, 2022
@cmarmo @glemaitre
Add msvcp140.dll to Windows 64 bit wheels (#24631)
c01919b
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Oct 31, 2022
@cmarmo @glemaitre
Add msvcp140.dll to Windows 64 bit wheels (scikit-learn#24631)
e656747
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@jjerphan jjerphan jjerphan approved these changes

Assignees
No one assigned
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
Milestone
1.1.3
Development

Successfully merging this pull request may close these issues.
3 participants
@cmarmo @thomasjpfan @jjerphan
0