Stop shipping unusable MKL search paths in the Linux wheel - #22541
Conversation
shoumikhin
added
the
release notes: build
meta-cla
Bot
added
the
CLA Signed
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22541
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (1 Unrelated Failure)As of commit ce0cd12 with merge base c65b23a ( BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@claude review |
|
Claude finished @kirklandsign's task in 3m 20s —— View job Review: Stop shipping unusable MKL search paths in the Linux wheel
This is a clean, well-scoped fix. The diagnosis matches the code, the match is narrow enough to keep a real MKL install, and both halves of the fix are enforced. I traced the two code paths and the tests and have no blocking concerns — a few small notes below. What works well
Minor notes (non-blocking)
VerificationI could not run Overall: correct, narrowly targeted, and the test design (bidirectional coverage + exercising shipped code) is a notable improvement over the build-only check it supplements. LGTM with the minor notes above. |
11395d1 to
56f8493
Compare
56f8493 to
5a5a236
Compare
5a5a236 to
9786188
Compare
|
@claude review |
|
Claude finished @shoumikhin's task in 1m 37s —— View job Review: Stop shipping unusable MKL search paths in the Linux wheel
I traced both code paths and the test harness. This is a clean, well-scoped fix, and the two minor notes from the earlier review round have since been addressed. LGTM. What works well
Addressed since the last review
Minor notes (non-blocking)
VerificationI could not run Overall: correct, narrowly targeted, and the earlier feedback is folded in. LGTM. |
9786188 to
e569216
Compare
e569216 to
36d21da
Compare
Five shared libraries in the Linux x86_64 wheel search three directories that
exist on nobody's machine:
/lib/intel64
/lib/intel64_win
/lib/win-x64
Two of them name a Windows layout, in a Linux wheel.
They come from PyTorch. Its exported CMake package creates a caffe2::mkl imported
target, and linking torch brings it in even though this project never asks for
MKL. That target carries a hardcoded list of four link directories below
MKL_ROOT, which resolves to nothing here, so what the linker records is left
anchored at the filesystem root. The bare MKL_ROOT/lib does not survive, because
CMake filters its own implicit link directories out of the link line, which is
why three entries appear rather than four. Packaging copies the built libraries out of the build
tree rather than installing them, so whatever the linker recorded ships as is.
Nothing needs them. No shipped library names an MKL or OpenMP runtime among its
dependencies, so nothing resolves through those directories. They are not merely
untidy either: they sit ahead of the relative entries packaging appends, and the
loader searches in order, so a user who happens to have a matching directory
resolves a library from there instead of from the one the wheel installed. That
is the same shadowing the release check already rejects a CUDA toolkit prefix
for.
The fix drops them where the other unusable entries are already dropped, and
matches only the exact /lib/<arch> form that an empty prefix produces. A real
MKL installation spells the same arch directory below a prefix, as
/opt/intel/mkl/lib/intel64, and that one is a directory the environment
genuinely provides, so it is kept.
The release check that rejects absolute search paths listed these three as
allowed, which is why they shipped while a check whose whole purpose is
rejecting absolute paths reported the wheel clean. It now rejects the empty
prefix form specifically, while still accepting a real installation's prefixed
directory, so it agrees with what packaging does rather than contradicting it.
Test plan:
New unit tests in .ci/scripts/tests/test_runtime_path_filter.py, which read the
functions out of setup.py so they exercise what ships. They run on every pull
request through the existing unit test job, and need no wheel build, which is
what the previous check could not manage.
pytest .ci/scripts/tests/test_runtime_path_filter.py
23 passed
They cover both directions, since a filter that satisfies either alone is wrong.
The three unresolved entries are dropped, a real MKL installation and ordinary
system directories are kept, every relative entry on a shipped library survives,
and the absolute torch directory is kept when it is a library's only route to
torch, which is what stops this becoming a blanket rule that breaks importing.
Confirmed by mutation that each part of the fix is load bearing. Nine mutations,
each failing at least one test: removing the packaging filter, removing any one
of the release check's three rejecting branches, severing the call that applies
the check to a shipped library, reordering its guards so a build directory is
accepted, widening the packaging predicate, narrowing what the check accepts so a
real installation is refused, making the packaging filter reject an ordinary
system directory, and adding an arch to the shared constant without teaching the
check about it.
The release check's per-entry decision is a module level function, so the unit
test calls the same code the wheel check runs and compares the reason it returns.
Asserting only that a path was rejected was not enough: the check rejects every
absolute path it does not recognise, so an unknown arch passed for the wrong
reason and two of its three branches could each be deleted alone.
Also measured the published artifacts directly, reading the recorded search
paths out of every shared library in each wheel:
1.4.1 release, CPython 3.12 6 libraries with a search path, 0 unusable
current nightly, CPU 3.12 18 libraries with a search path, 15 unusable
current nightly, CUDA 3.13 21 libraries with a search path, 15 unusable
The 1.4.1 release predates the code changed here. Both nightly rows show the
same 15 entries across the same five libraries, which is what this removes.
Reading the same libraries' declared dependencies shows no MKL or OpenMP runtime
in any of them.
Not verified: no wheel was built for this, since the change is to the filter
those builds already run and the tests exercise it directly. The link line that
emits the paths is unchanged; they are dropped at packaging, the same way the
CUDA toolkit and torch directories already are.
36d21da to
ce0cd12
Compare
Fixes #21611
The problem, in plain terms
Five shared libraries in the Linux wheel search three directories that exist on nobody's machine:
Two of them name a Windows layout, in a Linux wheel.
They come from PyTorch. Its exported CMake package creates a
caffe2::mklimported target with a hardcoded list of link directories, and linking torch brings them in even though this project never asks for MKL:MKL_ROOTresolves to nothing here, so what the linker records is left anchored at the filesystem root. Packaging copies the built libraries out of the build tree rather than installing them, so whatever the linker recorded ships as is. The bare/libform does not survive, because CMake filters its own implicit link directories out of the link line, which is why three entries appear rather than four.This only affects Linux x86_64. The aarch64 nightly records no absolute entries at all, since MKL is not found there, and the macOS and Windows wheels record none either.
Why it is worth fixing
Nothing needs those directories. No shipped library names an MKL or OpenMP runtime among its dependencies, so nothing resolves through them.
They are not merely untidy either. They sit ahead of the relative entries packaging appends, and the loader searches in order, so a user who happens to have a matching directory resolves a library from there instead of from the one the wheel installed. That is the same shadowing the release check already rejects a CUDA toolkit prefix for.
What changed
Two small pieces.
Packaging now drops these entries, in the same place the other unusable ones are already dropped. The match is deliberately narrow: only the exact
/lib/<arch>form that an empty prefix produces. A real MKL installation spells the same arch directory below a prefix, as/opt/intel/mkl/lib/intel64, and that is a directory the environment genuinely provides, so it is kept.The release check that rejects absolute search paths listed these three as allowed. That is why they shipped while a check whose whole purpose is rejecting absolute paths reported the wheel clean. It now rejects the empty prefix form specifically, and still accepts a real installation's prefixed directory, so the two halves agree rather than contradict each other.
Test plan
New unit tests in
.ci/scripts/tests/test_runtime_path_filter.py. They read the functions out ofsetup.py, so they exercise the code that ships rather than a copy, and they run on every pull request through the existing unit test job. They need no wheel build, which is what the previous check could not manage: it could only see this after a full build, and only on the platform that built one.They cover both directions, because a filter that satisfies either one alone is wrong:
I checked by mutation that each part of the fix is load bearing:
The release check's per-entry decision is a small module level function, so the unit test calls the same code the wheel check runs and compares the reason it returns. Asserting only that a path was rejected was not enough: the check rejects every absolute path it does not recognise, so an unknown arch passed for the wrong reason and two of its three branches could each be deleted on their own.
I also measured the published artifacts directly, reading the recorded search paths out of every shared library in each wheel:
The 1.4.1 release predates the code changed here. Both nightly rows show the same 15 entries across the same five libraries, which is what this removes. Reading those same libraries' declared dependencies shows no MKL or OpenMP runtime in any of them.
Scope
This covers the first of the two things the issue suggests: not shipping a runtime search path a user cannot use. It does not do the second, which is to resolve MKL through something the wheel or its declared dependencies own. That one belongs upstream, since the directory list is set in PyTorch's own CMake package, and its own comment there marks it as a hack. Nothing in this wheel links MKL, so there is no dependency here left to redirect.
What I did not verify
No wheel was built for this. The change is to the filter those builds already run, and the tests exercise it directly. The link line that emits the paths is unchanged; the entries are dropped at packaging, the same way the CUDA toolkit and torch directories already are.