Skip to content

Commit 5410b1a

Browse files
Stop shipping unusable MKL search paths in the Linux wheel (pytorch#22541)
Fixes pytorch#21611 ## The problem, in plain terms Five shared libraries in the Linux 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 with a hardcoded list of link directories, and linking torch brings them in even though this project never asks for MKL: ``` ${MKL_ROOT}/lib ${MKL_ROOT}/lib/intel64 ${MKL_ROOT}/lib/intel64_win ${MKL_ROOT}/lib/win-x64 ``` `MKL_ROOT` resolves 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 `/lib` form 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 of `setup.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. ``` pytest .ci/scripts/tests/test_runtime_path_filter.py 27 passed ``` They cover both directions, because a filter that satisfies either one 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, so a library can still find its siblings - 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 I checked by mutation that each part of the fix is load bearing: | what I broke | tests that failed | | --- | ---: | | removed the packaging filter | 4 | | removed the release check's maths rejection | 3 | | removed its build directory branch | 2 | | removed its catch-all rejection | 1 | | severed the call that applies it to a shipped library | 1 | | reordered its guards so a build directory is accepted | 1 | | narrowed what it accepts, refusing a real install | 3 | | made the packaging filter reject an ordinary system directory | 1 | | added an arch to the shared constant, untaught to the check | 2 | 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: | wheel | libraries with a search path | unusable entries | | --- | ---: | ---: | | 1.4.1 release, CPython 3.12 | 6 | 0 | | current nightly, CPU, CPython 3.12 | 18 | 15 | | current nightly, CUDA, CPython 3.13 | 21 | 15 | | current nightly, CPU, aarch64 | 16 | 0 | 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. Co-authored-by: PyTorch Bot <pytorchbot@users.noreply.github.com>
1 parent 89e108c commit 5410b1a

3 files changed

Lines changed: 445 additions & 68 deletions

File tree

Lines changed: 331 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,331 @@
1+
# Copyright (c) Meta Platforms, Inc. and affiliates.
2+
# All rights reserved.
3+
#
4+
# This source code is licensed under the BSD-style license found in the
5+
# LICENSE file in the root directory of this source tree.
6+
7+
"""Unit tests for the runtime search path filter packaging applies to shipped libraries.
8+
9+
Here rather than in the wheel checks, because the decision under test is a pure function of one
10+
string. The wheel checks can only see it after a full wheel build, and only on the platform that
11+
built one, so a filter that dropped the wrong entry reached the published artifact before anything
12+
ran that would notice.
13+
14+
Two properties are covered, and they pull in opposite directions, which is why both are needed.
15+
The filter must drop the MKL arch directories torch's exported link interface leaves anchored at
16+
the filesystem root, and it must keep the absolute torch directory that is a library's only route
17+
to torch when no relative one was recorded. A filter that satisfies either alone is wrong: the
18+
first way ships unusable paths, the second way stops the extensions importing.
19+
20+
The functions are read out of setup.py rather than restated, so the test exercises what ships.
21+
setup.py calls setup() at module scope, so it is loaded by compiling the definitions this needs
22+
instead of importing it, which would exit during setuptools argument parsing.
23+
"""
24+
25+
import ast
26+
import importlib.util
27+
import re
28+
from pathlib import Path, PurePosixPath
29+
30+
import pytest
31+
32+
REPO_ROOT = Path(__file__).resolve().parents[3]
33+
34+
# Compiled from setup.py, so a change to the filter is exercised here rather than duplicated.
35+
_WANTED = (
36+
"_MKL_ARCH_DIRECTORIES",
37+
"_is_cuda_toolkit_directory",
38+
"_is_unresolved_math_library_directory",
39+
"_is_usable_runtime_path",
40+
)
41+
42+
43+
def _setup_source() -> str:
44+
"""setup.py's text, decoded as UTF-8.
45+
46+
The encoding is named because `read_text()` defaults to the locale's, and both files this test
47+
parses contain non-ASCII characters. On a Windows runner that resolves to a code page, which
48+
mangles them, and the mangled text is what gets parsed.
49+
"""
50+
return (REPO_ROOT / "setup.py").read_text(encoding="utf-8")
51+
52+
53+
def _setup_namespace() -> dict:
54+
"""The runtime path helpers from setup.py, compiled without running setup()."""
55+
tree = ast.parse(_setup_source())
56+
wanted = []
57+
for node in tree.body:
58+
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
59+
if node.name in _WANTED:
60+
wanted.append(node)
61+
elif isinstance(node, ast.Assign):
62+
names = [t.id for t in node.targets if isinstance(t, ast.Name)]
63+
if any(name in _WANTED for name in names):
64+
wanted.append(node)
65+
found = set()
66+
for node in wanted:
67+
found.add(
68+
node.name
69+
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef))
70+
else next(t.id for t in node.targets if isinstance(t, ast.Name))
71+
)
72+
missing = sorted(set(_WANTED) - found)
73+
assert not missing, (
74+
f"setup.py no longer defines {missing} at module scope, so this test would silently "
75+
"check nothing. Update the names here to match."
76+
)
77+
namespace = {"re": re, "PurePosixPath": PurePosixPath}
78+
exec(
79+
compile(ast.Module(body=wanted, type_ignores=[]), "<setup.py>", "exec"),
80+
namespace,
81+
)
82+
return namespace
83+
84+
85+
@pytest.fixture(scope="module")
86+
def setup_helpers() -> dict:
87+
return _setup_namespace()
88+
89+
90+
# setup.py's own arch names, read at import so the parametrized cases below are driven by the
91+
# shipped constant rather than a second copy of it. Adding an arch to setup.py then extends both
92+
# the reject and the accept cases, which is what keeps packaging and the release check in step.
93+
_MKL_ARCH_DIRECTORIES = _setup_namespace()["_MKL_ARCH_DIRECTORIES"]
94+
95+
# What the linker records when MKL's prefix resolves empty, leaving its arch subdirectory
96+
# concatenated onto nothing. Two of the three name a Windows layout, in a Linux wheel.
97+
UNRESOLVED_MATH_DIRECTORIES = tuple(f"/lib/{arch}" for arch in _MKL_ARCH_DIRECTORIES)
98+
99+
100+
@pytest.mark.parametrize("entry", UNRESOLVED_MATH_DIRECTORIES)
101+
def test_drops_math_directories_with_an_empty_prefix(entry, setup_helpers):
102+
assert setup_helpers["_is_unresolved_math_library_directory"](entry) is True
103+
# A trailing separator is the same directory, which the path type normalises on its own. Asserted
104+
# because patchelf prints entries as recorded, so that spelling can genuinely arrive.
105+
assert setup_helpers["_is_unresolved_math_library_directory"](entry + "/") is True
106+
107+
108+
@pytest.mark.parametrize(
109+
"entry",
110+
[
111+
# A real MKL installation spells the same arch directory below a prefix, and that
112+
# directory genuinely exists, so dropping it would break a library resolving through it.
113+
"/opt/intel/mkl/lib/intel64",
114+
"/opt/intel/oneapi/mkl/latest/lib/intel64",
115+
"/usr/lib/intel64",
116+
"/home/user/lib/intel64",
117+
# The arch name as a parent rather than as the entry itself.
118+
"/lib/intel64/extra",
119+
# Ordinary system directories, which differ from the bad entries only in the last part.
120+
"/lib",
121+
"/lib64",
122+
"/lib/x86_64-linux-gnu",
123+
# Relative entries are decided before this predicate is reached, but it must not claim
124+
# one, or a wheel's own hop into a directory named intel64 would be dropped.
125+
"$ORIGIN/../../lib",
126+
"$ORIGIN/lib/intel64",
127+
],
128+
)
129+
def test_keeps_directories_that_name_a_real_prefix(entry, setup_helpers):
130+
assert setup_helpers["_is_unresolved_math_library_directory"](entry) is False
131+
# Also through the production predicate, not just the narrow one. Asserting only the narrow
132+
# predicate let _is_usable_runtime_path start rejecting an ordinary system directory with the
133+
# whole suite green, since nothing checked that these entries actually survive the filter.
134+
assert setup_helpers["_is_usable_runtime_path"](entry, True, True) is True
135+
136+
137+
# Read off the pybindings extension in the published x86_64 CPU nightly, in the recorded order. The
138+
# MKL block sits after five relative hops and before one. On this wheel the entries are simply dead,
139+
# since nothing resolves through them, so dropping them costs a few wasted lookups at load time and
140+
# removes paths a user cannot have. On the CUDA wheel the same block precedes the hop into the CUDA
141+
# runtime, which is where the ordering matters.
142+
SHIPPED_RUNTIME_PATH = [
143+
"$ORIGIN/../../../torch/lib",
144+
"$ORIGIN/../../src/executorch/lib",
145+
"$ORIGIN/../../backends/qualcomm",
146+
"$ORIGIN/../../lib",
147+
"$ORIGIN/../../../lib64",
148+
"/lib/intel64",
149+
"/lib/intel64_win",
150+
"/lib/win-x64",
151+
"$ORIGIN/../../backends/cuda",
152+
]
153+
154+
155+
def _relative_torch_route_predicate():
156+
"""setup.py's own has_relative_torch_route expression, lifted out of its function.
157+
158+
Read rather than restated, because it decides the third argument to the filter under test.
159+
A copy here would let the test keep passing after that expression changed, which is the one
160+
failure a regression guard must not have.
161+
"""
162+
tree = ast.parse(_setup_source())
163+
for node in ast.walk(tree):
164+
if not (
165+
isinstance(node, ast.Assign)
166+
and any(
167+
isinstance(target, ast.Name) and target.id == "has_relative_torch_route"
168+
for target in node.targets
169+
)
170+
):
171+
continue
172+
# The expression iterates a name bound in setup.py's own scope, so it is rebound to this
173+
# function's argument by compiling it as the body of a one-argument lambda.
174+
source = ast.unparse(node.value)
175+
iterated = "for entry in found"
176+
assert iterated in source, (
177+
f"setup.py computes has_relative_torch_route as {source!r}, which no longer iterates "
178+
"the name this test rebinds. Update the rebinding rather than leaving it a no-op."
179+
)
180+
return eval(
181+
f"lambda entries: {source.replace(iterated, 'for entry in entries')}"
182+
)
183+
raise AssertionError(
184+
"setup.py no longer computes has_relative_torch_route, so this test would pass the "
185+
"filter an argument the shipped code never produces."
186+
)
187+
188+
189+
def _filtered(entries, setup_helpers):
190+
is_usable = setup_helpers["_is_usable_runtime_path"]
191+
has_relative_torch_route = _relative_torch_route_predicate()(entries)
192+
return [
193+
entry
194+
for entry in entries
195+
# True is safe_to_drop_toolkit_paths: this wheel ships no CUDA, so an absolute toolkit path
196+
# in it names only the build machine. Packaging derives the same value from the built tree.
197+
if is_usable(entry, True, has_relative_torch_route)
198+
]
199+
200+
201+
def test_shipped_library_keeps_no_absolute_entry(setup_helpers):
202+
kept = _filtered(SHIPPED_RUNTIME_PATH, setup_helpers)
203+
assert [entry for entry in kept if entry.startswith("/")] == []
204+
205+
206+
def test_shipped_library_keeps_every_relative_hop(setup_helpers):
207+
# Asserted separately from the absence of absolute entries, because a filter that dropped
208+
# everything would satisfy that one while leaving the library unable to find its siblings.
209+
kept = _filtered(SHIPPED_RUNTIME_PATH, setup_helpers)
210+
assert kept == [
211+
entry for entry in SHIPPED_RUNTIME_PATH if not entry.startswith("/")
212+
]
213+
214+
215+
def test_keeps_the_absolute_torch_directory_when_it_is_the_only_route(setup_helpers):
216+
# The case that stops this being a blanket "drop everything absolute": an extension links
217+
# torch and, with no relative route recorded, reaches it only through the directory the
218+
# linker found it in. Dropping that would stop the extension importing.
219+
entries = [
220+
"/opt/conda/envs/py_3.12/lib/python3.12/site-packages/torch/lib",
221+
"/lib/intel64",
222+
]
223+
assert _filtered(entries, setup_helpers) == [entries[0]]
224+
225+
226+
def test_drops_the_absolute_torch_directory_when_a_relative_route_exists(setup_helpers):
227+
# The other side of the same rule, and the only case that exercises the route expression at all.
228+
# Without a relative entry in the list the route is False whatever that expression says, so the
229+
# test above cannot tell a correct expression from an inverted one.
230+
entries = [
231+
"$ORIGIN/../../../torch/lib",
232+
"/opt/conda/envs/py_3.12/lib/python3.12/site-packages/torch/lib",
233+
"/lib/intel64",
234+
]
235+
assert _filtered(entries, setup_helpers) == [entries[0]]
236+
237+
238+
_MATH_DIRECTORY_REASON = "a maths library directory whose prefix resolved empty"
239+
_BUILD_DIRECTORY_REASON = "inside a build of this project"
240+
_UNREACHABLE_REASON = "an absolute directory the wheel has a relative route to"
241+
242+
243+
def _release_check_decision():
244+
"""The release check's own per-entry decision, imported rather than replayed.
245+
246+
Loaded as a module so the unit test exercises the function the wheel check calls, rather than
247+
that function's source text. Reading the text let the rejection be deleted outright.
248+
"""
249+
path = REPO_ROOT / ".ci" / "scripts" / "wheel" / "test_shared_libraries.py"
250+
spec = importlib.util.spec_from_file_location("_release_check_under_test", path)
251+
module = importlib.util.module_from_spec(spec)
252+
spec.loader.exec_module(module)
253+
return module._unusable_runtime_path_kind
254+
255+
256+
@pytest.mark.parametrize("arch", _MKL_ARCH_DIRECTORIES)
257+
def test_release_check_rejects_the_math_directories(arch):
258+
# The check and packaging are separate code, so a wheel built without patchelf keeps these
259+
# entries and only the check would catch it. Driven off setup.py's own constant, and asserting
260+
# the REASON rather than merely a rejection: the check rejects every absolute path it does not
261+
# recognise, so an arch it was never taught about would otherwise pass through the catch-all.
262+
decide = _release_check_decision()
263+
assert (
264+
decide(f"/lib/{arch}", "_C.cpython-312-x86_64-linux-gnu.so")
265+
== _MATH_DIRECTORY_REASON
266+
)
267+
268+
269+
def test_release_check_rejects_each_kind_for_its_own_reason():
270+
# One assertion per rejecting branch, by reason, so deleting any single branch fails here.
271+
# Asserting only "not None" let the build-directory branch and the catch-all each be removed
272+
# on their own with every test green.
273+
decide = _release_check_decision()
274+
assert decide("/home/u/pip-out/lib", "_C.so") == _BUILD_DIRECTORY_REASON
275+
assert decide("/opt/rocm/lib", "_C.so") == _UNREACHABLE_REASON
276+
assert decide("", "_C.so") is not None
277+
278+
279+
def test_release_check_rejects_a_build_directory_before_the_allowlist():
280+
# Order is load bearing and nothing else holds it. A torch directory inside a CI worker tree
281+
# must be rejected, which only happens because the build-directory branch runs before the
282+
# suffix allowlist gets to accept the /torch/lib ending.
283+
decide = _release_check_decision()
284+
entry = "/home/ec2-user/actions-runner/_work/executorch/pytorch/torch/lib"
285+
assert decide(entry, "_C.so") == _BUILD_DIRECTORY_REASON
286+
287+
288+
@pytest.mark.parametrize("arch", _MKL_ARCH_DIRECTORIES)
289+
def test_release_check_still_accepts_a_real_mkl_installation(arch, setup_helpers):
290+
# The two must agree on every arch in the shared constant. Packaging KEEPS a prefixed one,
291+
# because the environment provides it, so a check that rejected it would fail a wheel packaging
292+
# deliberately allowed and the builder could not satisfy both. Parametrized off the constant, so
293+
# adding an arch to setup.py without teaching the check about it fails here.
294+
entry = f"/opt/intel/mkl/lib/{arch}"
295+
assert setup_helpers["_is_usable_runtime_path"](entry, True, True) is True
296+
assert (
297+
_release_check_decision()(entry, "_C.cpython-312-x86_64-linux-gnu.so") is None
298+
)
299+
300+
301+
def test_the_wheel_scan_consults_the_classifier():
302+
# The unit tests above call the classifier directly, and the wheel scan is the only thing that
303+
# applies it to a real library. That scan needs an installed wheel, so it cannot run here;
304+
# what is checkable is the wiring, and severing it left all tests green. Asserted on the AST so
305+
# a rename or an accidental deletion fails rather than silently disabling the enforcement.
306+
path = REPO_ROOT / ".ci" / "scripts" / "wheel" / "test_shared_libraries.py"
307+
tree = ast.parse(path.read_text(encoding="utf-8"))
308+
scan = next(
309+
(
310+
node
311+
for node in ast.walk(tree)
312+
if isinstance(node, ast.FunctionDef)
313+
and node.name == "test_no_absolute_runtime_paths"
314+
),
315+
None,
316+
)
317+
assert scan is not None, "the wheel scan this check enforces no longer exists"
318+
called = {
319+
node.func.id
320+
for node in ast.walk(scan)
321+
if isinstance(node, ast.Call) and isinstance(node.func, ast.Name)
322+
}
323+
assert "_unusable_runtime_path_kind" in called, (
324+
"test_no_absolute_runtime_paths no longer calls _unusable_runtime_path_kind, so the wheel "
325+
"scan would report every shipped library clean while these unit tests still pass."
326+
)
327+
328+
329+
def test_release_check_accepts_a_relative_entry():
330+
# A relative hop is the normal case and must never be rejected, whatever the absolute rules do.
331+
assert _release_check_decision()("$ORIGIN/../../lib", "_C.so") is None

0 commit comments

Comments
 (0)