fix: engines default to no runtime cache; the module owns the implicit one - #4482
Conversation
c3c4cf7 to
03513c2
Compare
03513c2 to
5522dcd
Compare
3a8a16e to
f42577c
Compare
f42577c to
c31831c
Compare
A TRTEngine built without a module came up with the default RuntimeSettings, whose runtime_cache is a path string. Nothing owned the wrapper that string implies, so attaching it handed the live IRuntimeCache to the engine's IRuntimeConfig and then let it be collected -- a use-after-free at the next createExecutionContext. Engines now default to runtime_cache=None, matching the cpp side, where RuntimeSettings::runtime_cache is an intrusive_ptr defaulting to nullptr and no string form exists. The implicit cache belongs to the module, which resolves its path string to a RuntimeCache and pushes it down in setup_engine; that path is unchanged. TorchTensorRTModule resets its own RuntimeSettings on the post-load paths (set_extra_state, __setstate__) and those resets move to runtime_cache=None too, so the module and the engine it rebuilds agree. Leaving them at the string default would let a later runtime_config(...) block resolve the stale path and install an autosaving handle at the shared default location on exit -- switching caching on via a call that never mentioned it. An engine reached without a module -- built from packed engine info, or loaded as a graph constant from a saved ExportedProgram -- now runs with no cache instead of a dangling one, and a caller can attach a RuntimeCache explicitly. No configuration loses working behaviour: on the Python runtime this path raised, on the cpp runtime it already attached nothing, and on standard TensorRT the runtime config is never initialized.
_apply_settings had three arms, and the str one built a RuntimeCache it did not outlive. Its own docstring already claimed raw strings were not accepted here; the code twenty lines below accepted them. Engines now take only something that owns what it points at -- the Python equivalent of the cpp intrusive_ptr<RuntimeCacheHandle>. A str raises TypeError naming the module as the place path strings are resolved. The class's own default follows: a TRTRuntimeConfig built with no settings would otherwise start from the string form this commit exists to abolish. TorchTensorRTModule._resolve_runtime_cache normalizes an empty-string runtime_cache to None rather than passing it through, so no str can reach an engine from the module. Also corrects the RuntimeSettings.runtime_cache docstring, which promised the engine owned the implicit handle and saved it on __del__, and a reference to a method renamed some time ago.
TestEngineOwnsNoCache pins the contract at the engine: a module-less engine defaults to no cache, executes without one, accepts an explicitly attached RuntimeCache, and raises TypeError on a path string. TestModuleStillOwnsImplicitCache guards the other direction -- the compile path must keep building, attaching and persisting its implicit cache -- and covers the empty-string normalization. TestPostLoadOwnsNoCache pins the same contract on the reset paths, where module and engine could otherwise disagree: the config's own default, and what torch.load / load_state_dict leave behind. Its last test is the one that matters -- a cuda-graph-only context manager over a loaded module must not install a cache on enter or leave one installed on exit, because re-applying a path string through the setter creates a handle rather than restoring one.
c31831c to
7fffe7c
Compare
| logger.warning(f"Failed to warm-load runtime cache from {rc!r}: {e}") | ||
| cache = wrapped.ensure_cache(self._live) | ||
| self._live.set_runtime_cache(cache) | ||
| self._live.set_runtime_cache(rc.ensure_cache(self._live)) | ||
| else: | ||
| raise TypeError( |
There was a problem hiding this comment.
We probably want to destroy the self._live here because the initialization was not successful?
There was a problem hiding this comment.
Good catch, done in cb95af3 by resetting the live and propagating the exception up the call stack. Thanks Adrian!
ensure_initialized assigns self._live before _apply_settings runs, so an exception out of the apply left a half-configured IRuntimeConfig in place -- strategies set, runtime cache never attached. The early-return guard at the top then made the next call a no-op, so a caller who caught the error and retried proceeded silently against those partial settings; the error was only ever raised once. Reset self._live and re-raise instead, so a retry re-attempts initialization and fails the same way. Measured before the change: first execute raised TypeError, second returned normally with _live still populated.
509cfbd to
cb95af3
Compare
|
[by Claude Code] CI failures on this PR are pre-existing, not introduced here.
The base commit ( Verified locally on TensorRT-RTX/A100 — |
|
@tp5uiuc |
Description
What
An engine built without a
TorchTensorRTModulecame up with a path-stringruntime_cacheand attached a cache nothing owned, freeing it out from underthe engine. Engines now default to no cache; the module owns the implicit one.
Why
RuntimeSettings.runtime_cachedefaults to a path string, andTorchTensorRTModuleresolves it into aRuntimeCacheit owns, so the compilepath is fine. An engine reached without a module -- built from packed engine
info, or loaded as a graph constant from a saved
ExportedProgram-- neverpasses through that resolver, so
TRTRuntimeConfigwrapped the string in alocal, handed the live
IRuntimeCacheto the engine'sIRuntimeConfig, and letit be collected on return. The next
createExecutionContextread freed memory.The C++ runtime has no such path:
RuntimeSettings::runtime_cacheis anintrusive_ptrdefaulting tonullptr, with no string form to mis-own.The module's post-load resets have to move with the engine, or the two disagree:
a module holding a stale path string lets a later
runtime_config(...)block --a call that need not mention caching -- resolve it and leave an autosaving handle
installed at the shared default path on exit.
How
_TRTEngine.py: engines constructTRTRuntimeConfigwithruntime_cache=None, matching the C++ default.RuntimeSettings()'s owndefault is unchanged -- that one belongs to the module, which pushes the
resolved handle down in
setup_engine._TorchTensorRTModule.py: theset_extra_state/__setstate__resets dropto
runtime_cache=Noneso the module agrees with the engine it rebuilds; anempty-string
runtime_cachenormalizes toNoneso no string reaches anengine.
_runtime_config.py:_apply_settingsaccepts onlyNoneor aRuntimeCacheand raisesTypeErroron a string;TRTRuntimeConfig's ownno-settings default follows suit. The docstring already claimed raw strings
were not accepted here.
explicitly attached
RuntimeCache, and reject strings; the compile path stillbuilds and persists its implicit cache; the post-load resets leave no cache,
and a cuda-graph-only context manager over a loaded module installs none on
enter and leaves none on exit.
Testing
TensorRT-RTX on A100,
tests/py/dynamo/runtime/plusmodels/test_runtime_cache_models.py:The four failures before are the new tests themselves; each was confirmed
failing on the parent commit and passing here, on both builds. There are no
other failures on either build.
models/test_export_serde.py,test_cross_runtime_serde.pyandtest_fallback_data_dependent_ops.pywere run before and after on both buildsbecause the post-load resets sit on the
torch.load/load_state_dictpaths:identical results either side, with the only failures (
test_save_load_aoti,test_save_load_ts) pre-existing and unrelated.Save/load deployment was checked on both runtimes: the loaded engine runs with
no cache attached, and an explicitly attached
RuntimeCachepersists.Cost / Gotchas
An engine used without a module now gets no runtime cache instead of an implicit
one, and neither do modules restored by
torch.load/load_state_dict. Noconfiguration loses working behaviour: that path raised on the Python runtime,
already attached nothing on the C++ runtime, and never initializes a runtime
config on standard TensorRT. Callers wanting a cache there attach a
RuntimeCacheexplicitly.Supersedes #4541 -- the
TestPythonRuntimeAliasedIOfailures it skipped arefixed here rather than skipped.
Followups
runtime_config()/runtime_cache()locate engines vianamed_modules(), soneither reaches an engine loaded as a graph constant. Attaching settings to a
saved-and-loaded program currently requires the engine API directly.
Restoring implicit caching to
load_state_dict-- by re-applying the setterafter engine construction in
set_extra_state, assetup_enginedoes -- isdeliberately left out; it changes C++-runtime behaviour too and wants its own
testing.
Type of change
Checklist: