Save TensorRT engine in weights file with onnx2leela and automate during runtime - #2428
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements support for packaging TensorRT EPContext (embedded engine) ONNX models into lc0 weights files, enabling more stable TRT performance by reusing the embedded engine model instead of rebuilding engines at runtime.
Changes:
- Adds
is_ep_contextto theOnnxModelprotobuf to mark embedded EPContext models. - Extends
onnx2leelato detect EPContext models and emit weights with the EPContext flag and model payload. - Updates the ONNX TRT runtime to dump/build an EPContext model and automatically write an
-embedded.pb.gzweights variant.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/tools/onnx2leela.cc | Detects EPContext models and sets is_ep_context while packaging ONNX into weights. |
| src/neural/wrapper.cc | Makes backend network options inherit from parent options (affects how weights path is discovered). |
| src/neural/backends/onnx/network_onnx.cc | Enables TRT EPContext dumping/embedding and writes -embedded.pb.gz during runtime; adjusts TRT options. |
| proto/net.proto | Adds the is_ep_context field to the ONNX model proto. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I'll take a look later, but just want to say that the suggestions for the Copilot are often too "defensive", and often extra checks that it suggests are not worth it (use your judgement). |
Sure, thanks a lot! Yup seems like copilot suggestions are a bit defensive. Either ways, I have replied to all |
Menkib64
left a comment
There was a problem hiding this comment.
Do we have to use file to store the context temporary everytime?
I haven't figured out how the loading code passes these to TensorRT but it would be useful if it could be loaded as string like we do for other models. If string cannot be passed, then I think it is better to use a temporary file/directory to pass the model and make sure it is deleted. The loading should also work if there is multiple threads running at the same time loading different engines.
We have a need to store many engines together. We use engines specifically optimized for a small range of batch sizes. For example 5090 can use optimized backend options with -o batch=7,steps=12. This requires loading 12 slightly different engines created from the same network. This would be an important feature to include when embedding the engine to a network file. There would be reason to add a couple of more engines to optimize a few key batch sizes even more. It could make sense to add special engines which handle only batch size 1 and exact minibatch size. These should offer a little faster evaluation for these special case. There might be need to add more small batch size kernels because TRT is bad at optimizing kernels for multiple batch sizes.
| } | ||
| } | ||
|
|
||
| if (provider_ == OnnxProvider::TRT && !is_ep_context_) { |
There was a problem hiding this comment.
I don't think it is good idea to load a different file to what user specified. We should let user decide which network is used for a specific run.
There was a problem hiding this comment.
This block of code only saves the embedded engine in a -embedded.pb.gz weights file, where the network would come from the --weights input if provided by the user or from the DiscoverWeightsFile if no input is given. The net_path is never passed on to the Ort::Session
The engine is loaded with only the net file the user provides.
Please do correct me if I am wrong somewhere
There was a problem hiding this comment.
Right. I missed the point that it was about saving instead of load. There is options which are passed to the engine. We would need to define an option to give the desired embedded context file path. Backend should only generate the file when user requests it.
There was a problem hiding this comment.
Right, I can do this, I was not sure if I can create an option for this and instead hardcode it, but I'll create that which would save it only when requested
There was a problem hiding this comment.
Done i added the option dump_embedded_weights. When not used, the engine does not store the _ctx model in a new package weight file. If mentioned, it stores it at the path mentioned
also the _ctx model is now deleted at the end, so it does not get saved in trt_cache after run. I found no way to have this not save on runtime however (like model.onnx). Do you think it is possible?
| if (is_ctx) { | ||
| for (const auto& out : model.graph().output()) { | ||
| const auto& name = out.name(); | ||
| if (onnx->has_output_policy() && name.find("policy") != std::string::npos) { |
There was a problem hiding this comment.
It is possible to get onnx models where output names don't follow naming convention. The names are provided as command line arguments to let user choose name maps to which output or input.
There was a problem hiding this comment.
ah good point, ill use the cli arguments instead of the hardcoded strings
There was a problem hiding this comment.
I removed this part altogether. Why set it again, when it was set before. I missed this previously
| onnx->set_input_planes(in); | ||
| data_type = GetDataType(model, in); | ||
| if(is_ctx){ | ||
| data_type = GetDataType(model, ""); |
There was a problem hiding this comment.
Why does empty string override the input data_type?
There was a problem hiding this comment.
The reason for the empty-string fallback is that ctx models can have their input node renamed by TRT compilation
I didnt intend the overriding; it should have been an if else chunk
There was a problem hiding this comment.
Shouldn't we save the renamed node names for inputs and outputs and pass the new name here?
There was a problem hiding this comment.
Yes youre right, added that in
Firstly, thanks a lot for having a look at this! Yes this is a valid point, the context file does take up quite some space (171 MB on my system currently). I'll have to take a look if we can bypass the step of having to save this in the trt_cache folder. It should be possible, but in the case that this isnt found, it can always be manually deleted once the weights file is generated EDIT: I have now added code that removes the _ctx.onnx when the run finished. However it still remains in trt_cache folder until the embedded weights are packaged in the file given in the dump_embedded_weights option |
| trt_options["trt_engine_cache_enable"] = "1"; | ||
| trt_options["trt_dump_ep_context_model"] = "1"; | ||
| trt_options["trt_ep_context_file_path"] = cache_dir; | ||
| trt_options["trt_ep_context_embed_mode"] = "1"; |
There was a problem hiding this comment.
trt_ep_context_file_path needs similar prefix as cache file uses. This makes sure that concurrent exports work if building different engines for different batch sizes on multiple GPUs.
There was a problem hiding this comment.
Added the prefix based naming
| file.onnx_model().model().size(), | ||
| std::filesystem::path ctx_path = std::filesystem::path(cache_dir_) / "_ctx.onnx"; | ||
| for (int step = 1; step <= steps_; step++) { | ||
| if (provider_ == OnnxProvider::TRT && is_ep_context_) { |
There was a problem hiding this comment.
This branch should be removed. We should be able to provide a context model just like an ordinary model. There is no need for special handling here.
It is open question if we have to disable cache uses when loading a context model.
| for (const auto& output : model.graph().output()) { | ||
| const auto& name = output.name(); | ||
| if (md_out->has_output_policy() && | ||
| name.find("policy") != std::string::npos) { |
There was a problem hiding this comment.
Should this use the name from original model?
Original model could have outputs named in any possible way. There is no requirement to include any specific string in the name.
|
|
||
| if (provider_ == OnnxProvider::TRT && !is_ep_context_ && | ||
| opts.Exists<std::string>(SharedBackendParams::kDumpEmbeddedWeightsId)) { | ||
| std::string net_path = opts.Get<std::string>(SharedBackendParams::kDumpEmbeddedWeightsId); |
There was a problem hiding this comment.
Backend specific options don't have help messages currently. Backends just implement them using direct key string lookup. You can see how "gpu", "batch", "steps" are handled currently.
SharedBackendParams makes it look like option should be passed directly to Leela. But the option is parsed for --backend-options dump-embedded-weights=file_path.
| } | ||
|
|
||
| md_out->set_model(ctx); | ||
| md_out->set_is_ep_context(true); |
There was a problem hiding this comment.
We should store trt_profile_min_shapes, trt_profile_max_shapes and trt_profile_opt_shapes with the model. These parameters restrict when the engine is valid. For example 5090 could be configured using --backend-options batch=7,steps=12 which generates 12 different engines. Each engine is valid for only 7 batch sizes. Loading has to verify that the context file matches requested optimization state.
This also puts another requirement to context files. We want to embed many engines together to provide optimized engines for full batch range.
There was a problem hiding this comment.
Yup, have added these. Also tested with various settings with backendbench
| for (int step = 1; step <= steps_; step++) | ||
| session_.emplace_back(onnx_env_, file.onnx_model().model().data(), | ||
| file.onnx_model().model().size(), | ||
| std::filesystem::path ctx_path = std::filesystem::path(cache_dir_) / "_ctx.onnx"; |
There was a problem hiding this comment.
This path would have had race condition when initializing many paths. All temporary files must use unique filename like tmpfile created file.
I had a quick look into the TensorRT provider code. It looks to me that we can just use the existing loading code as is. Context file is a drop in replacement for onnx models. No need for temporary files.
| trt_options["trt_layer_norm_fp32_fallback"] = "1"; | ||
| trt_options["trt_force_sequential_engine_build"] = "1"; | ||
| trt_options["trt_context_memory_sharing_enable"] = "1"; | ||
| trt_options["trt_context_memory_sharing_enable"] = is_ep_context_ ? "0" : "1"; |
There was a problem hiding this comment.
Why is context sharing disabled?
Did you see a bug when loading them?
Did you test a multi-engine configuration like batch=7,steps=4?
There was a problem hiding this comment.
Yes, i recall that the build failed to load the model when this was set, so i had it disabled for ep context models. But after reading your other reviews, i'll make some changes to the model loading to see if that solves it. If it does, I shall remove this diff
There was a problem hiding this comment.
This is the exact error i got while running the embedde engine weights file, with this setting set to true:
2026-08-02 12:01:10.7885568 [E:onnxruntime:lc0, tensorrt_execution_provider.h:90 onnxruntime::TensorrtLogger::log] [2026-08-02 06:31:10 ERROR] IExecutionContext::setDeviceMemory: Error Code 3: API Usage Error (Parameter check failed, condition: memory != nullptr || mEngine.getDeviceMemorySizeInternal(mOptimizationProfile, false) == 0. setDeviceMemory: Cannot set memory to nullptr.)
2026-08-02 12:01:10.7978102 [E:onnxruntime:lc0, tensorrt_execution_provider.h:90 onnxruntime::TensorrtLogger::log] [2026-08-02 06:31:10 ERROR] IExecutionContext::enqueueV3: Error Code 3: API Usage Error (Parameter check failed, condition: noDeviceMemory. The engine requires 6727168 device memory. The IExecutionContext is created with ExecutionContextAllocationStrategy::kUSER_MANAGED or ICudaEngine::createExecutionContextWithoutDeviceMemory. IExecutionContext::setDeviceMemoryV2 should be called before enqueue/execute.)
2026-08-02 12:01:10.8144406 [E:onnxruntime:, sequential_executor.cc:615 onnxruntime::ExecuteKernel] Non-zero status code returned while running TRTKernel_graph_TRTKernel_graph_org.lczero/converted_12010072553396873023_0_10041065298831557996_0 node. Name:'TensorrtExecutionProvider_TRTKernel_graph_TRTKernel_graph_org.lczero/converted_12010072553396873023_0_10041065298831557996_0_0' Status Message: TensorRT EP execution context enqueue failed.
Unhandled exception in worker thread: Non-zero status code returned while running TRTKernel_graph_TRTKernel_graph_org.lczero/converted_12010072553396873023_0_10041065298831557996_0 node. Name:'TensorrtExecutionProvider_TRTKernel_graph_TRTKernel_graph_org.lczero/converted_12010072553396873023_0_10041065298831557996_0_0' Status Message: TensorRT EP execution context enqueue failed.
I have a feeling this is related to the setting itself. What do you think?
There was a problem hiding this comment.
The error message looks like it might be a bug in onnxruntime side. It fails to allocated context memory if loading embedded model.
| int min_batch_size_; | ||
| int gpu_; | ||
| // trt cache directory and flag for trt EPcontext model | ||
| std::string cache_dir_; |
There was a problem hiding this comment.
We don't have to store the cache directory because it is only used in constructor. The code can use helper function to return the string when it is needed in different parts of code.
| int gpu_; | ||
| // trt cache directory and flag for trt EPcontext model | ||
| std::string cache_dir_; | ||
| bool is_ep_context_; |
There was a problem hiding this comment.
Runtime doesn't need to know about context file. We should keep it as a stack variable in constructor.
| onnx->set_input_planes(in); | ||
| data_type = GetDataType(model, in); | ||
| if (is_ctx) { | ||
| bool found = false; |
There was a problem hiding this comment.
There shouldn't be any need for special case here. Caller should provide input and output names to converter.
|
Thanks a lot @Menkib64 for having a look, ill have a relook on the code. Apologies if it was a bit bothersome to review this. |
|
Made a couple of changes. Here's a rundown:-
Here is some testing I did with the generated embedded package weights. Since each embedded weight has the engine baked into it, a particular shape can only be run for an embedded weight file that has the same shape baked into it. I have added an exception for this case. Default:-Batch=8, Steps=2Batch=14, Steps=1 |
| trt_options["trt_layer_norm_fp32_fallback"] = "1"; | ||
| trt_options["trt_force_sequential_engine_build"] = "1"; | ||
| trt_options["trt_context_memory_sharing_enable"] = "1"; | ||
| trt_options["trt_context_memory_sharing_enable"] = is_ep_context_ ? "0" : "1"; |
There was a problem hiding this comment.
The error message looks like it might be a bug in onnxruntime side. It fails to allocated context memory if loading embedded model.
| trt_options["trt_engine_cache_prefix"] = | ||
| "Lc0_ONNX_TRT_ORT_" + Ort::GetVersionString() + "_batch_" + | ||
| std::string cache_prefix = | ||
| "Lc0_ONNX_TRT_ORT_" + Ort::GetVersionString() + "_gpu_" + |
There was a problem hiding this comment.
Different gpu are expected to use the same cache file. Current initialization is done in the main thread only. I have a branch where backends can be initialized concurrently. This branch has extra initialization locking for onnx-trt so each GPU loads different batch size range. All GPUs should share the same cache file for the same batch size range.
I'm thinking that the same locking rule would apply to saving context files. It would have minimal help to use many GPUs to build context file concurrently because onnxruntime has some unnecessary locking in tensorrt execution provider.
There was a problem hiding this comment.
cool, ill use the existing prefix then
| std::string cache_dir = | ||
| (std::filesystem::path(CommandLine::BinaryDirectory()) / "trt_cache") | ||
| .string(); | ||
| is_ep_context_ = md.is_ep_context(); |
There was a problem hiding this comment.
is_ep_context_ could be a stack variable.
| session_.emplace_back( | ||
| onnx_env_, model.data(), model.size(), | ||
| GetOptions(threads, batch_size_ * step, hash, optimize, | ||
| dump_weights && !multi_step_embedded ? &ctx_paths[step - 1] : nullptr)); |
There was a problem hiding this comment.
Should dump_weights be always false when the input has an embedded context?
Now I think that an error check together with size checks would make sense. It would throw an error if trying to dump from an embedded context.
There was a problem hiding this comment.
Yeah, fair.
Yes dump weights should be false when the input itself has an embedded context, since the engine is basically just one node right, the EPContext stub. Theres nothing to dump.
I added some error checks around this for all possible cases. Theres an additional case if trying to dump but the provider itself isnt onnx-trt but any of the other ones. Another case is if the weight file is ep context but provider is not trt.
I think this covers all cases?
| trt_options["trt_max_partition_iterations"] = "1000"; | ||
| trt_options["trt_min_subgraph_size"] = "1"; | ||
| trt_options["trt_engine_cache_enable"] = "1"; | ||
| trt_options["trt_dump_ep_context_model"] = "1"; |
There was a problem hiding this comment.
We should only enable dump context conditionally when constructor is asking for the context path.
There was a problem hiding this comment.
thanks yes youre right
With respect to the following issue: #2185
This PR adds the following:-