-
Notifications
You must be signed in to change notification settings - Fork 374
feat: improve engine caching and fix bugs #3932
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@cehongwang please take a pass so we have multiple eyes on this PR |
a54907e to
ea81677
Compare
| logger.warning( | ||
| "require_full_compilation arg is not applicable for torch.compile with backend='torch_tensorrt" | ||
| ) | ||
| if settings.strip_engine_weights: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would a torch.compile use try to use strip weights?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added the warning back. Not sure why strip_engine_weights arg doesn't work for torch.compile()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just doenst make sense. Torch compile is not serializable. So why would you ever want a callable that doesnt have the weights in it
| logger.info(f"The engine already exists in cache for hash: {hash_val}") | ||
| return False | ||
|
|
||
| if not settings.strip_engine_weights: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like strip weights should only apply to the returned engine and not to the cache directly. So a returned cache engine with strip weights == True wont be refit. but you always only save stripped engine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current design is to save stripped engine for using less hard disk. A returned cache engine with strip weights == True wont be refit as well. Only strip weights == False will be refit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah but what i mean is every engine we save in the cache should be a stripped weights engine right? We arent doing weight matching in the hash function for pulling from the cache so we will need to refit anyway. So the serialization config should always have serialization_config.set_flag(trt.SerializationFlag.EXCLUDE_WEIGHTS) regardless if the user tells us settings.strip_engine_weights == True or settings.strip_engine_weights == False
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| User Setting | Engine Saved to Cache | Engine Returned to User |
|---|---|---|
| settings.strip_engine_weights == True | Weightless | Weightless |
| settings.strip_engine_weights == False | Weightless | Refit Weights |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is it that we dont need to explictly set the setting if the weight was built with stripped weights? Is there any harm in doing so? Then there is only one code path
| ), f"Attempted to refit a cached engine built for a different input size (input: {i}, cached size: {cached_engine_inputs[i]}, new size: {inputs[i]}" | ||
|
|
||
| logger.info( | ||
| "Found the cached engine that corresponds to this graph. It is directly loaded." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Print the hash
| and engine_cache is not None | ||
| and not settings.immutable_weights | ||
| ): | ||
| if settings.cache_built_engines or settings.reuse_cached_engines: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this code is unclear. Would recommend something like this
hash_val = engine_cache.get_hash(module, inputs, settings) if (settings.cache_built_engines or settings.reuse_cached_engines) else None
if settings.reuse_cached_engines:
serialized_interpreter_result = pull_cached_engine(
hash_val, module, engine_cache, settings, inputs
)
if serialized_interpreter_result is not None: # hit the cache
return serialized_interpreter_result
...
if (
ENABLED_FEATURES.refit
and not settings.immutable_weights
and settings.cache_built_engines
and engine_cache is not None
):
_ = insert_engine_to_cache(
hash_val, interpreter_result, engine_cache, settings, inputs
)
serialized_engine = interpreter_result.engine.serialize()
| if ( | ||
| ENABLED_FEATURES.refit | ||
| and not settings.immutable_weights | ||
| and settings.cache_built_engines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably throw a warning or something if engine_cache is None and settings.cache_built_engines == True
Description
As I requested, TensorRT 10.14 added an argument
trt.SerializationFlag.INCLUDE_REFITto allow refitted engines to keep refittable. That means engines can be refitted multiple times. Based on the capability, this PR enhances the existing engine caching and refitting features as follows:compilation_settings.strip_engine_weights. Then, when users pull out the cached engine, it will be automatically refitted and kept refittable.refit_module_weights(). e.g.:_conversion.py.Type of change
Checklist: