add torch-tensorrt-executorch-delegate wheel build#4398
Conversation
|
@lanluo-nvidia the new package should be in //py See: https://packaging.python.org/en/latest/guides/packaging-namespace-packages/ |
shoumikhin
left a comment
There was a problem hiding this comment.
Nice work on this. The core idea is right. You build _portable_lib from source and pull TensorRTBackend into the same module that holds the backend registry, so the two always share one copy of the ExecuTorch core. That is the correct way to fix the undefined symbol crash people hit today. I checked the linking part and it holds up: one registry, no duplicate core, and the whole-archive flag is actually needed.
| # latest commit in release/1.3 branch | ||
| commit = "6118688a095fd9697224f5cad72ce42db641c9cd", | ||
| # PyTorch 2.14-compatible ExecuTorch main commit | ||
| commit = "a6d812a082df57898b8608f56c867140cc9da32c", |
There was a problem hiding this comment.
Blocking: this change never takes effect in CI, so the build still uses the old ExecuTorch.
packaging/pre_build_script.sh line 84 rebuilds this whole file from a template:
cat toolchains/ci_workspaces/MODULE.bazel.tmpl | envsubst > MODULE.bazelThat template (toolchains/ci_workspaces/MODULE.bazel.tmpl) still points at the old commit 6118688. So when CI runs, it overwrites your edit and goes back to the old version, and that old version is what the delegate gets compiled against. It also means it works on your machine but not in CI.
Please put the new commit in toolchains/ci_workspaces/MODULE.bazel.tmpl (and the Windows template too if there is one). The root MODULE.bazel is generated, so it is cleaner to leave it alone and only edit the template.
| .github/scripts/verify-executorch-reference-runner.sh "${RUNNER_TEMP}/torchtrt-python.pte" | ||
|
|
||
| # Build the no-compile-for-users Python runtime/delegate wheel. | ||
| python -m pip wheel --no-build-isolation --no-deps --wheel-dir dist packaging/executorch_delegate |
There was a problem hiding this comment.
Blocking: this step builds the delegate wheel, but the wheel build needs a file that this job never creates, so it will fail here.
The delegate's setup.py calls torchtrt_version(), which reads py/torch_tensorrt/_version.py. That file is not checked in. It is written only by the main setup.py. This job runs bazel build //:libtorchtrt (C++ only) and never runs the main setup.py, so the file is missing and torchtrt_version() raises RuntimeError("Could not determine the Torch-TensorRT version"). Anyone following the README to build the wheel on its own hits the same error.
A simple fix is to fall back to version.txt and the env override when the file is not there:
def torchtrt_version() -> str:
if value := os.getenv("TORCH_TENSORRT_EXECUTORCH_DELEGATE_VERSION"):
return value
version_py = REPO_ROOT / "py/torch_tensorrt/_version.py"
if version_py.exists():
if m := re.search(r'__version__\s*=\s*["\']([^"\']+)', version_py.read_text()):
return m.group(1)
return (REPO_ROOT / "version.txt").read_text().strip()|
|
||
| import pytest | ||
|
|
||
| RUNTIME_PATH = Path(__file__).parents[3] / "py/torch_tensorrt/executorch/runtime.py" |
There was a problem hiding this comment.
Should fix: this test never runs in CI, so it will not catch regressions.
The ExecuTorch test group in tests/py/utils/ci_helpers.sh lines 157 to 162 does cd tests/py/dynamo and then runs pytest executorch/, so it only picks up tests in tests/py/dynamo/executorch/. This file is in tests/py/executorch/, a different folder that nothing runs.
On top of that, the test replaces the real runtime with a fake (FakeRuntime), so even if it did run it would not touch the tricky new code: activate(), the sys.modules swap, or the failure path.
Please move it under tests/py/dynamo/executorch/, and add a few plain Python tests (no GPU needed) that use fake modules to check: calling activate() twice is safe, it raises DelegateCompatibilityError when the stock runtime was already loaded, and it does not leave a leftover data_loader entry when the native import fails.
| return existing | ||
| try: | ||
| data_loader = importlib.import_module(__name__ + ".data_loader") | ||
| sys.modules[_DATA_LOADER_NAME] = data_loader |
There was a problem hiding this comment.
Should fix: this can leave Python half set up if the native import fails.
activate() puts data_loader into sys.modules first, then imports _portable_lib. If that second import fails (the same crash this wheel is meant to fix), the error is raised but the data_loader entry is left behind, and from then on anything importing the normal ExecuTorch data_loader gets your version instead.
Import both first, and only register them once both succeed:
try:
data_loader = importlib.import_module(__name__ + ".data_loader")
native = importlib.import_module(__name__ + "._portable_lib")
except ImportError as error:
sys.modules.pop(_DATA_LOADER_NAME, None) # do not leave a half state
raise DelegateCompatibilityError(...) from error
sys.modules[_DATA_LOADER_NAME] = data_loader
sys.modules[_NATIVE_NAME] = native
sys.modules.pop(_WRAPPER_NAME, None)Separate note, not a change request: the existing.__name__ check just above is actually correct and safe to call more than once. I built a small single-phase C extension to confirm its __name__ comes out as the full pkg._portable_lib, so repeat calls do not throw. Worth leaving that one as is.
| model_path = Path(path) | ||
| if not model_path.is_file(): | ||
| raise FileNotFoundError(f"ExecuTorch model not found: {model_path}") | ||
| return Program(_runtime().load_program(model_path.read_bytes())) |
There was a problem hiding this comment.
Should fix: small lifetime question. load_program(model_path.read_bytes()) reads the file into a bytes object and passes it straight in, without keeping a reference to it. If ExecuTorch points at that memory instead of copying it, the bytes get freed right after this line and you get a use after free.
Can you check whether load_program copies the data? If it does not, keep the bytes alive by storing them on the returned Program, for example self._data = data.
| ) | ||
|
|
||
| output = pathlib.Path(self.get_ext_fullpath(ext.name)).resolve() | ||
| if output.exists(): |
There was a problem hiding this comment.
Minor: if output.exists(): return skips the build when the .so is already there. On a clean CI run that is fine, but on a local rebuild it can ship an old .so from a previous build without telling you. Consider a build stamp, or just remove the early return.
| if existing is not None: | ||
| if existing.__name__ != __name__ + "._portable_lib": | ||
| raise DelegateCompatibilityError( | ||
| "ExecuTorch's stock runtime was imported first. Import " |
There was a problem hiding this comment.
Minor: the docstring implies importing torch_tensorrt.executorch.runtime reserves the slots, but activation is lazy and runs the first time you call load(). So the real rule is: call a delegate function before you import executorch.runtime. Please reword that, and maybe also have the check look at _WRAPPER_NAME.
| export EXECUTORCH_SOURCE_DIR="$(dirname "${executorch_cmake_location%%:*}")" | ||
| export TensorRT_ROOT=/path/to/TensorRT | ||
|
|
||
| python -m pip install executorch==1.3.1 |
There was a problem hiding this comment.
Minor: the README says pip install executorch==1.3.1, but MODULE.bazel now targets main for the 2.14 ABI. Please make the doc match the version you actually build against.
| requires = [ | ||
| "setuptools>=68", | ||
| "wheel>=0.40", | ||
| "torch", |
There was a problem hiding this comment.
Minor: torch is listed here without a version. It does not matter in CI because CI uses --no-build-isolation, but a normal build would pull the newest torch into a clean build environment and mismatch the ABI. Please pin it, or say clearly that --no-build-isolation is required.
| /external/ | ||
| build-executorch/ | ||
| packaging/executorch_delegate/build/ | ||
| packaging/executorch_delegate/dist/ |
There was a problem hiding this comment.
Minor: this adds a trailing blank line at the end of the file.
|
One more note, on a file this PR does not change so I cannot leave it inline. In |
Description
export EXECUTORCH_SOURCE_DIR=/home/lanl/git/executorch
export EXECUTORCH_ROOT=/home/lanl/git/executorch
build the torch-tensorrt-executorch-delegate.whl
python -m pip wheel
--no-build-isolation
--no-deps
--wheel-dir dist
packaging/executorch_delegate
install the wheel
python -m pip install --no-deps --force-reinstall
dist/torch_tensorrt_executorch_delegate-*.whl
export the pte
python /home/lanl/git/py312/TensorRT/examples/torchtrt_executorch_example/export_static_shape.py
load the pte
python examples/executorch_reference_runner/load_model.py
--model_path=/path/to/model.pte
Fixes # (issue)
Type of change
Please delete options that are not relevant and/or add your own.
Checklist: