Extract wheels directly from resource #9
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where embedded wheel files were being extracted using fake file paths based on resource names, causing "Error extracting zip file" messages during Python installation. The code now extracts wheels directly from resource streams into the site-packages folder, while pywin32 wheels continue to be installed via pip.
Key Changes:
- Replaced parallel wheel installation with direct extraction from embedded resource streams
- Added explicit site-packages directory creation and path construction
- Separated pywin32 wheels for pip installation from direct extraction logic
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| foreach (var resName in wheelsAssembly.GetManifestResourceNames()) | ||
| { | ||
| bool isWheel = x.Contains(".whl"); | ||
| if (isWheel && x.Contains("pywin32-")) | ||
| bool isWheel = resName.EndsWith(".whl"); | ||
| if (!isWheel) continue; | ||
|
|
||
| if (resName.Contains("pywin32-")) | ||
| { | ||
| pipWheelInstall.Add(x); | ||
| return false; | ||
| pipWheelInstall.Add(resName); | ||
| continue; | ||
| } | ||
|
|
||
| return isWheel; | ||
| }).Select(wheel => Python.Included.Installer.InstallWheel(wheelsAssembly, wheel))).ConfigureAwait(false); | ||
| using (var stream = wheelsAssembly.GetManifestResourceStream(resName)) | ||
| { | ||
| if (stream == null || stream.Length == 0) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| using (var zip = new System.IO.Compression.ZipArchive(stream, System.IO.Compression.ZipArchiveMode.Read, false)) | ||
| { | ||
| foreach (var entry in zip.Entries) | ||
| { | ||
| if (string.IsNullOrEmpty(entry.Name)) continue; | ||
|
|
||
| var destPath = Path.Combine(sitePkgsPath, entry.FullName.Replace('/', Path.DirectorySeparatorChar)); | ||
| var destDir = Path.GetDirectoryName(destPath); | ||
| if (!string.IsNullOrEmpty(destDir)) | ||
| { | ||
| Directory.CreateDirectory(destDir); | ||
| } | ||
|
|
||
| using (var inStream = entry.Open()) | ||
| using (var outStream = new FileStream(destPath, FileMode.Create, FileAccess.Write, FileShare.None)) | ||
| { | ||
| await inStream.CopyToAsync(outStream).ConfigureAwait(false); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The foreach loop contains an await statement (line 413), but the loop is not async-aware. This will execute the wheel extractions sequentially rather than in parallel. Consider collecting tasks and using Task.WhenAll() to maintain the parallel execution behavior from the original implementation, which could significantly impact installation performance when multiple wheels need to be extracted.
There was a problem hiding this comment.
this could be problematic?
There was a problem hiding this comment.
IMO this shouldn’t be a problem. Running the extractions sequentially should be safer and still fast enough since we only have around a dozen wheels.
The original version was parallel, but that can sometimes cause disk thrashing or file-exist race conditions. Given that the speed difference is minimal and our tests were already a bit noisy, keeping the extraction sequential felt like a more stable option.
Happy to revert if you think it’s better to restore the parallel behavior.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Can you point to a Jenkins run with the error output you were seeing? I'm wary of replacing Python.Included's built-in mechanism for this with our own extraction logic. But I guess if it works... |
|
So, how were the tests working with wheel files before?
There is no error, but we do see logs such as this when running tests: 
|
|
But I also want to understand why the install option did not work here |
|
Looks like it was hitting this exception. But it looks like Python.Included checks whether the files exist already, so not sure why that wasn't working. But I'm also not convinced that it's worth it to do ourselves just to get rid of a few log lines. |
|
@zeusongit , @twastvedt Now that are using the nuget , the wheels are embedded in |
|
Python.Included has always been a third-party dll, our wheels were never bundled into that dll, they have always been embedded in the DSPythonNet3Wheels resource file. This file, which bundles the whl files into DSPythonNet3Wheels.dll, has not been changed for a year. |
|
I was comparing to DSCPython 
|
…://github.com/ivaylo-matov/PythonNet3Engine into Extract-wheels-directly-from-resource_251103_
|
So a wheel like numpy for example is successfully installed when you work with Dynamo ? |
4 participants
This PR fixes the issue where
DuplicateCallsToImportShouldBeFineinDynamoPythontestswas showing standard output “Error extracting zip file … DSPythonNet3Wheels.Resources..whl”The problem was that the code tried to unzip embedded wheel files using fake file paths based on their resource names.
Now the code extracts the wheels directly from the resource stream into the site-packages folder instead of building those paths.
pywin32wheels are still installed using pip as before.This change should remove the noisy error messages and make the wheel installation more reliable.
@zeusongit
@dnenov