Conversation
| public IntPtr ModelCompilationOptions_SetGraphOptimizationLevel; | ||
| public IntPtr ModelCompilationOptions_SetOutputModelWriteFunc; | ||
| public IntPtr ModelCompilationOptions_SetOutputModelGetInitializerLocationFunc; | ||
| public IntPtr ModelCompilationOptions_SetInputModel; |
There was a problem hiding this comment.
Debating whether or not we need this, ultimately... I realized that there aren't managed versions of the model editor APIs. But since I am touching the API table, it seemed mandatory(?).
| // ORT_LOAD_CONFIG_FROM_MODEL is not supported for OrtModel input. | ||
| // Check early so we fail before session construction. | ||
| if (input_from_model) { | ||
| const Env& os_env = Env::Default(); | ||
| if (os_env.GetEnvironmentVar(inference_session_utils::kOrtLoadConfigFromModelEnvVar) == "1") { | ||
| return ORT_MAKE_STATUS(ONNXRUNTIME, INVALID_ARGUMENT, | ||
| "The environment variable ORT_LOAD_CONFIG_FROM_MODEL=1 is set, but loading " | ||
| "config from model is not supported for in-memory OrtModel input. " | ||
| "OrtModel is programmatically constructed and has no embedded ORT config. " | ||
| "Unset ORT_LOAD_CONFIG_FROM_MODEL or use file/buffer input instead."); | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: not sure this is worth the extra code here given (afaik) reading the config from the model is a very niche usage.
| const auto* output_path = ep_context_gen_options.TryGetOutputModelPath(); | ||
| if (!has_no_output_model_location && output_path != nullptr && output_path->empty()) { | ||
| has_no_output_model_location = true; | ||
| } |
There was a problem hiding this comment.
should we instead prevent an empty path being set?
| // Fast-fail: If OrtModel has no model_path and user hasn't specified output location or embed mode, | ||
| // EPs that need to write context binaries will fail later. Fail early with a clear error. | ||
| if (input_from_model && !model_has_path && has_no_output_model_location && | ||
| !ep_context_gen_options.embed_ep_context_in_model) { | ||
| return ORT_MAKE_STATUS(ONNXRUNTIME, INVALID_ARGUMENT, | ||
| "OrtModel has no model_path set and no output location was specified. " | ||
| "Please either: (1) set the model_path on the OrtGraph before adding to OrtModel, " | ||
| "(2) call SetOutputModelPath/SetOutputModelBuffer to specify an output location, or " | ||
| "(3) call SetEpContextEmbedMode(true) to embed EP context in the model."); | ||
| } |
There was a problem hiding this comment.
Is it worth duplicating code for this? Not sure we need a 'fast fail' path if the issue is a usage error as that should be caught and fixed during development.
| #if !defined(ORT_MINIMAL_BUILD) || defined(ORT_MINIMAL_BUILD_CUSTOM_OPS) | ||
| // Add custom domains | ||
| if (options && !options->custom_op_domains_.empty()) { | ||
| ORT_API_RETURN_IF_STATUS_NOT_OK(sess->AddCustomOpDomains(options->custom_op_domains_)); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Should this #if also cover the code to add custom domains from OrtEpDevice instances?
| compile_options.SetOutputModelBuffer(allocator.get(), &output_buffer, &output_size); | ||
|
|
||
| Ort::Status status = Ort::CompileModel(*ort_env, compile_options); | ||
| EXPECT_TRUE(status.IsOK()) << "First CompileModel failed: " << status.GetErrorMessage(); |
There was a problem hiding this comment.
FWIW there are helpers like EXPECT_ORTSTATUS_OK in onnxruntime\test\util\include\api_asserts.h that can potentially simplify some of the checks
| // Test: model can be reused after compilation | ||
| TEST(ModelEditorCompileAPITest, ModelCanBeReusedAfterCompilation) { |
There was a problem hiding this comment.
Is this something we care about? It's nice if it does it, but I'm wondering if this is a guarantee we need/want to provide or not.
| // Model should still be usable for creating a session | ||
| Ort::SessionOptions session_options; | ||
| Ort::Session session(*ort_env, model, session_options); | ||
| EXPECT_EQ(session.GetInputCount(), 1u); | ||
| EXPECT_EQ(session.GetOutputCount(), 1u); |
There was a problem hiding this comment.
Would be nice to execute the model to confirm it does all work as expected. e.g. say some memory has been prematurely freed. in theory we could create a session but might not notice an actual issue until we run the model as that might be the point we attempt to read from the memory.
2 participants
Description
This change adds a feature to the Compile API, allowing an in-memory OrtModel created via the Model Editor API to be compiled directly without first serializing to a file or buffer.
Motivation and Context
The Model Editor API and Compile API are both public C APIs in the ONNX Runtime, but until now there was no way to pass a programmatically constructed model directly to compilation. This change attempts to closes that gap (see #26750) and ensures the new code path behaves identically to the established file and buffer paths.