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

GH-45278: [Python][Packaging] Updated delvewheel install command and updated flags used with delvewheel repair #45323

Merged
merged 5 commits into from
Jan 27, 2025

Conversation

protokoul
Copy link
Contributor

@protokoul protokoul commented Jan 21, 2025

Rationale for this change

It is already explained in the issue.

What changes are included in this PR?

This PR installs delvewheel by using its latest version instead of using a github branch. The new flag --with-mangle introduced in the latest version of delvewheel is used with delvewheel repair command. Removed comments that referred to the use of the github branch for delvewheel installation.

Are these changes tested?

No, these changes are not tested because to run Windows containers I will need Windows 11 Pro or Enterprise. I do not have a machine that satisfies this requirement.

Copy link

⚠️ GitHub issue #45278 has been automatically assigned in GitHub to PR creator.

@github-actions github-actions bot added the awaiting review Awaiting review label Jan 21, 2025
@raulcd
Copy link
Member

raulcd commented Jan 21, 2025

@github-actions crossbow submit wheel-windows-cp39-cp39-amd64

Copy link

Revision: 9308397

Submitted crossbow builds: ursacomputing/crossbow @ actions-3fbb6f8b60

Task Status
wheel-windows-cp39-cp39-amd64 GitHub Actions

@raulcd
Copy link
Member

raulcd commented Jan 21, 2025

@github-actions crossbow submit wheel-windows-cp39-cp39-amd64

Copy link

Revision: d7c2e48

Submitted crossbow builds: ursacomputing/crossbow @ actions-c8bb220277

Task Status
wheel-windows-cp39-cp39-amd64 GitHub Actions

@raulcd
Copy link
Member

raulcd commented Jan 21, 2025

From what I understand this is currently not mangling msvcp40.dll because we are copying it on the wheel. I think we have to remove this copy:

@REM Bundle the C++ runtime
cp C:\Windows\System32\msvcp140.dll pyarrow\

@protokoul
Copy link
Contributor Author

protokoul commented Jan 21, 2025

Sure, let me push this change. Also, would it be better if I also add delvewheel show as discussed here if it helps to see any changes?

@raulcd
Copy link
Member

raulcd commented Jan 21, 2025

@github-actions crossbow submit wheel-windows-cp39-cp39-amd64

Copy link

Revision: 4135fb9

Submitted crossbow builds: ursacomputing/crossbow @ actions-13055f6c30

Task Status
wheel-windows-cp39-cp39-amd64 GitHub Actions

@raulcd

This comment was marked as duplicate.

Copy link
Member

@raulcd raulcd left a comment

Choose a reason for hiding this comment

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

Great! This seems to work but it's copying the dll on pyarrow.libs/ an this was not expected to be previously:

 C:\>C:\Python39\python C:\arrow\ci\scripts\python_wheel_validate_contents.py --path C:\arrow\python\repaired_wheels   || exit /B 1 
Traceback (most recent call last):
  File "C:\arrow\ci\scripts\python_wheel_validate_contents.py", line 48, in <module>
    main()
  File "C:\arrow\ci\scripts\python_wheel_validate_contents.py", line 44, in main
    validate_wheel(args.path)
  File "C:\arrow\ci\scripts\python_wheel_validate_contents.py", line 35, in validate_wheel
    assert not outliers, f"Unexpected contents in wheel: {sorted(outliers)}"
AssertionError: Unexpected contents in wheel: ['pyarrow.libs/', 'pyarrow.libs/.load-order-pyarrow-20.0.0.dev14', 'pyarrow.libs/msvcp140-a4b2ab3dc6cde85d4f2c0e24432d66e1.dll']

We might want to update our python_wheel_validate_contents.py script to allow this folder to be available on the Windows wheel.

@protokoul protokoul requested a review from raulcd January 22, 2025 11:05
@raulcd
Copy link
Member

raulcd commented Jan 22, 2025

@github-actions crossbow submit wheel-windows-cp39-cp39-amd64

Copy link

Revision: 657d971

Submitted crossbow builds: ursacomputing/crossbow @ actions-3a4d87d41c

Task Status
wheel-windows-cp39-cp39-amd64 GitHub Actions

Copy link
Member

@raulcd raulcd left a comment

Choose a reason for hiding this comment

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

Thanks for taking the effort @protokoul
This seems to work now with the newer delvewheel version:

2025-01-22T13:31:33.2489094Z External dependencies to copy into the wheel are
2025-01-22T13:31:33.2489352Z {'msvcp140.dll'}
...
2025-01-22T13:31:33.2497910Z copying DLLs into pyarrow.libs
2025-01-22T13:31:33.2498831Z copying C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.28.29333\bin\HostX64\x64\msvcp140.dll -> C:\Users\ContainerAdministrator\AppData\Local\Temp\tmpgwwi5dn3\pyarrow.libs\msvcp140.dll
...
2025-01-22T13:31:33.2509034Z repairing msvcp140.dll -> msvcp140-a4b2ab3dc6cde85d4f2c0e24432d66e1.dll
...

My only concern is that it seems to add C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.28.29333\bin\HostX64\x64\msvcp140.dll to the wheel instead of the downloaded C:\Windows\System32\msvcp140.dll.
@pitrou from what I understand we were manually upgrading it because the system provided one was an old version, is this an issue that prevents us from using the released delvewheel?
I suppose the solution would be to build our own Image and use a newer Microsoft Runtime Library but that's a bigger effort.

@raulcd raulcd self-requested a review January 22, 2025 14:40
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jan 22, 2025
@pitrou
Copy link
Member

pitrou commented Jan 22, 2025

@pitrou from what I understand we were manually upgrading it because the system provided one was an old version, is this an issue that prevents us from using the released delvewheel?

As long as we're using the same one that we've built against, it should be ok.

I suppose the solution would be to build our own Image and use a newer Microsoft Runtime Library but that's a bigger effort.

Definitely, but it's desirable anyway: #45156

@protokoul
Copy link
Contributor Author

Thank you @raulcd for reviewing the changes and helping with the PR.

Copy link
Member

@raulcd raulcd left a comment

Choose a reason for hiding this comment

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

As we are not using the downloaded msvcp140.dll on the built wheel, I would probably remove the following to avoid leading us to think this is used:

@REM Install a more recent msvcp140.dll in C:\Windows\System32
choco install -r -y --no-progress vcredist140
choco upgrade -r -y --no-progress vcredist140
dir C:\Windows\System32\msvcp140.dll

Once this is done, I am happy with this changes

@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jan 24, 2025
@raulcd
Copy link
Member

raulcd commented Jan 24, 2025

@github-actions crossbow submit wheel-windows-cp39-cp39-amd64

Copy link

Revision: 4cb4791

Submitted crossbow builds: ursacomputing/crossbow @ actions-b334eba5b6

Task Status
wheel-windows-cp39-cp39-amd64 GitHub Actions

Copy link
Member

@raulcd raulcd left a comment

Choose a reason for hiding this comment

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

This LGTM.
@pitrou are you ok with this change? you worked on the previous delvewheel fix :)

@raulcd raulcd requested a review from pitrou January 27, 2025 10:25
@pitrou
Copy link
Member

pitrou commented Jan 27, 2025

Someone should check that the wheel still loads on a system without msvcp140.dll installed. I can try to do that using Docker at some point.

@pitrou
Copy link
Member

pitrou commented Jan 27, 2025

@github-actions crossbow submit wheel-windows*

Copy link

Revision: 4cb4791

Submitted crossbow builds: ursacomputing/crossbow @ actions-708fedca9b

Task Status
wheel-windows-cp310-cp310-amd64 GitHub Actions
wheel-windows-cp311-cp311-amd64 GitHub Actions
wheel-windows-cp312-cp312-amd64 GitHub Actions
wheel-windows-cp313-cp313-amd64 GitHub Actions
wheel-windows-cp313-cp313t-amd64 GitHub Actions
wheel-windows-cp39-cp39-amd64 GitHub Actions

@pitrou
Copy link
Member

pitrou commented Jan 27, 2025

Ok, it seems to work with the Crossbow-generated wheel above:

Python 3.9.13 (tags/v3.9.13:6de2ca5, May 17 2022, 16:36:42) [MSC v.1929 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import ctypes
>>> ctypes.CDLL('msvcp140.dll')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Python\lib\ctypes\__init__.py", line 374, in __init__
    self._handle = _dlopen(self._name, mode)
FileNotFoundError: Could not find module 'msvcp140.dll' (or one of its dependencies). Try using the full path with constructor syntax.
>>> import pyarrow as pa
>>> pa.show_info()
pyarrow version info
--------------------
Package kind              : python-wheel-windows
Arrow C++ library version : 20.0.0-SNAPSHOT
Arrow C++ compiler        : MSVC 19.28.29336.0
Arrow C++ compiler flags  :  /DWIN32 /D_WINDOWS /GR /EHsc /D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING  /EHsc /wd5105 /bigobj /utf-8 /W3 /wd4800 /wd4996 /wd4065  /MP
Arrow C++ git revision    : 4cb4791fb41cd4d130949bad45f72f775301f24d
Arrow C++ git description :
Arrow C++ build type      : release
[...]

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jan 27, 2025
@raulcd raulcd merged commit 2eba03f into apache:main Jan 27, 2025
16 checks passed
@raulcd raulcd removed the awaiting merge Awaiting merge label Jan 27, 2025
@raulcd
Copy link
Member

raulcd commented Jan 27, 2025

Thanks @protokoul for your first contribution!

Copy link

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 2eba03f.

There were 8 benchmark results with an error:

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 17 possible false positives for unstable benchmarks that are known to sometimes produce them.

@protokoul
Copy link
Contributor Author

Someone should check that the wheel still loads on a system without msvcp140.dll installed. I can try to do that using Docker at some point.

@pitrou Does this mean that msvcp140.dll is not mandatory to create the wheel? I thought maybe some sort of if-else can be implemented in the .bat file to check if msvcp140.dll is already installed. If it isn't, then then it can be manually installed like this. Would this also work?

@REM Install a more recent msvcp140.dll in C:\Windows\System32
choco install -r -y --no-progress vcredist140
choco upgrade -r -y --no-progress vcredist140
dir C:\Windows\System32\msvcp140.dll

@pitrou
Copy link
Member

pitrou commented Jan 29, 2025

@pitrou Does this mean that msvcp140.dll is not mandatory to create the wheel?

I don't understand what you mean here, can you clarify the question?

@protokoul
Copy link
Contributor Author

protokoul commented Jan 29, 2025

As you mentioned that the someone should check if the wheel still loads on a system without msvcp140.dll installed, I interpreted it as an expectation that the wheel can be created on a machine which does not have msvcp140.dll installed. Did I understand this incorrectly?

@pitrou
Copy link
Member

pitrou commented Jan 29, 2025

"The wheel still loads on a system without msvcp140.dll installed" means exactly that. It has nothing to do with creating the wheel.

lriggs pushed a commit to lriggs/arrow that referenced this pull request Jan 30, 2025
…d and updated flags used with delvewheel repair (apache#45323)

### Rationale for this change

It is already explained in the issue.

### What changes are included in this PR?

This PR installs delvewheel by using its latest version instead of using a github branch. The new flag `--with-mangle` introduced in the latest version of delvewheel is used with `delvewheel repair` command. Removed comments that referred to the use of the github branch for delvewheel installation.

### Are these changes tested?

No, these changes are not tested because to run Windows containers I will need [Windows 11 Pro or Enterprise](https://docs.docker.com/desktop/setup/install/windows-install/#:~:text=To%20run%20Windows%20containers%2C%20you%20need%20Windows%2010%20or%20Windows%2011%20Professional%20or%20Enterprise%20edition.%20Windows%20Home%20or%20Education%20editions%20only%20allow%20you%20to%20run%20Linux%20containers.). I do not have a machine that satisfies this requirement. 

* GitHub Issue: apache#45278

Authored-by: anubhav <[email protected]>
Signed-off-by: Raúl Cumplido <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants