Add environment variable support for profiling configuration #26798
Conversation
- Add ORT_ENABLE_PROFILING environment variable to enable/disable profiling - Add ORT_PROFILE_FILE_PREFIX environment variable to set output file prefix - Environment variables override SessionOptions when set - Add comprehensive unit tests covering 9 critical scenarios - Add detailed documentation with complete 24-combination behavior table This allows users to enable profiling without modifying application code, useful for debugging and performance analysis
|
@lalithsharan-intel please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
|
Hi @yuslepukhin and @edgchen1, This approach seems reasonable to me. Do you see any issue with optionally enabling profiling via environment variables? |
|
There is another PR that enables profiling via RunOptions. I like that better. Typically, I am against env var due to additional control outside the binary. What this requires is a forethought on the part of the app developer. |
There was a problem hiding this comment.
Pull request overview
This PR adds support for configuring ONNX Runtime profiling behavior via environment variables (ORT_ENABLE_PROFILING and ORT_PROFILE_FILE_PREFIX). The intent is to allow users to enable profiling without modifying application code.
Key Changes:
- Add environment variable reading logic in InferenceSession constructor
- Add 9 unit tests covering various scenarios
- Add comprehensive documentation with behavior table
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| onnxruntime/core/session/inference_session.cc | Implements environment variable reading and override logic for profiling configuration |
| onnxruntime/test/framework/inference_session_test.cc | Adds 9 unit tests for profiling environment variable feature |
| docs/ProfilingEnvironmentVariables.md | Comprehensive documentation with usage examples and 24-combination behavior table |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!env_enable_profiling.empty()) { | ||
| if (env_enable_profiling == "1") { | ||
| // Environment variable enables profiling (does not disable if already enabled via SessionOptions) | ||
| session_options_.enable_profiling = true; | ||
| if (!env_profile_prefix.empty()) { | ||
| session_options_.profile_file_prefix = ToPathString(env_profile_prefix); | ||
| } else if (session_options_.profile_file_prefix.empty()) { | ||
| session_options_.profile_file_prefix = ORT_TSTR("onnxruntime_profile"); | ||
| } | ||
| LOGS(*session_logger_, INFO) << "Profiling enabled via ORT_ENABLE_PROFILING environment variable. " | ||
| << "Profile prefix: " << ToUTF8String(session_options_.profile_file_prefix); | ||
| } else if (env_enable_profiling != "0") { | ||
| LOGS(*session_logger_, WARNING) << "Invalid value for ORT_ENABLE_PROFILING environment variable: '" | ||
| << env_enable_profiling << "'. Expected '0' or '1'. Ignoring."; | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation does not match the documented behavior in several critical ways:
-
Missing prefix override when ORT_ENABLE_PROFILING is not "1": According to the documentation (rows 13-24 in the behavior table), when ORT_ENABLE_PROFILING="0" or is not set, the ORT_PROFILE_FILE_PREFIX should still override SessionOptions.profile_file_prefix. However, the current implementation only processes env_profile_prefix inside the
if (!env_enable_profiling.empty())block, meaning the prefix override is ignored when profiling is controlled by SessionOptions. -
Missing empty string override: The documentation states that setting ORT_PROFILE_FILE_PREFIX="" should explicitly override the session prefix to an empty string (rows 3-4, 9-10, 15-16, 21-22). However, the implementation checks
if (!env_profile_prefix.empty())which treats empty string the same as "not set".
The logic should be restructured to:
- Handle ORT_PROFILE_FILE_PREFIX independently of ORT_ENABLE_PROFILING
- Distinguish between env_profile_prefix being empty string vs not being set at all (use a method that returns an optional or a has_value indicator)
| // Verify the prefix is empty string (env var "" overrides session prefix) | ||
| const auto& session_options = session.GetSessionOptions(); | ||
| ASSERT_EQ(session_options.profile_file_prefix, ORT_TSTR("")); |
There was a problem hiding this comment.
This test expects that setting ORT_PROFILE_FILE_PREFIX="" will override the session prefix to an empty string. However, this cannot work with the current implementation.
When the environment variable is set to an empty string, GetEnvironmentVar returns "", and env_profile_prefix.empty() evaluates to true. The implementation then skips the override logic at line 524-525 and only sets a default prefix if session_options_.profile_file_prefix is also empty (line 526-527).
In this test case, session_options_.profile_file_prefix is set to "session_prefix" (line 3288), so it won't be overridden. The assertion at line 3299 expecting profile_file_prefix to be "" will fail.
This test appears to be written against a different (documented but not implemented) behavior.
| TEST(InferenceSessionTests, ProfilingPrefixIgnoredWhenDisabled) { | ||
| auto logging_manager = std::make_unique<logging::LoggingManager>( | ||
| std::unique_ptr<ISink>(new CLogSink()), logging::Severity::kVERBOSE, false, | ||
| LoggingManager::InstanceType::Temporal); | ||
|
|
||
| // Set prefix env var but don't enable profiling | ||
| Env::Default().SetEnvironmentVar("ORT_PROFILE_FILE_PREFIX", "should_be_ignored"); | ||
|
|
||
| std::unique_ptr<Environment> env; | ||
| auto st = Environment::Create(std::move(logging_manager), env); | ||
| ASSERT_TRUE(st.IsOK()); | ||
|
|
||
| SessionOptions so; | ||
| so.enable_profiling = false; | ||
|
|
||
| InferenceSession session{so, *env}; | ||
| ASSERT_STATUS_OK(session.Load(MODEL_URI)); | ||
| ASSERT_STATUS_OK(session.Initialize()); | ||
|
|
||
| // Verify profiling is disabled (prefix should be irrelevant) | ||
| ASSERT_FALSE(session.GetProfiling().IsEnabled()); |
There was a problem hiding this comment.
This test validates that the prefix is ignored when ORT_ENABLE_PROFILING is not set. However, this contradicts the documented behavior in the behavior table (rows 13-24 in ProfilingEnvironmentVariables.md), which shows that ORT_PROFILE_FILE_PREFIX should take effect even when ORT_ENABLE_PROFILING is "0" or not set.
For example, row 13 shows: ORT_ENABLE_PROFILING="0", SessionOptions.enable_profiling=false, ORT_PROFILE_FILE_PREFIX="custom" should result in profiling disabled but prefix="custom" (not "onnxruntime_profile" or the session default).
The current implementation only processes the prefix when ORT_ENABLE_PROFILING="1", which means the prefix is ignored in all other cases. This is inconsistent with the documentation.
|
|
||
| - Environment variables are read during `InferenceSession` construction | ||
| - Invalid values for `ORT_ENABLE_PROFILING` (anything other than "0" or "1") are logged as warnings and ignored | ||
| - The prefix from `ORT_PROFILE_FILE_PREFIX` is validated to ensure it doesn't contain path separators or other invalid characters |
There was a problem hiding this comment.
The documentation claims that "The prefix from ORT_PROFILE_FILE_PREFIX is validated to ensure it doesn't contain path separators or other invalid characters" but this validation is not present in the implementation. The implementation directly converts the environment variable value to a path string without any validation:
session_options_.profile_file_prefix = ToPathString(env_profile_prefix);
Either this validation should be added to the implementation, or this claim should be removed from the documentation.
| - The prefix from `ORT_PROFILE_FILE_PREFIX` is validated to ensure it doesn't contain path separators or other invalid characters | |
| - The prefix from `ORT_PROFILE_FILE_PREFIX` is used as provided; callers are responsible for ensuring it does not contain path separators or other invalid characters according to their platform's rules |
| // | ||
| // Testing Strategy Rationale: | ||
| // --------------------------- | ||
| // The profiling environment variable feature has 24 possible combinations (2+ù2+ù3+ù2): |
There was a problem hiding this comment.
The mathematical expression contains corrupted characters. The expression "(2+�2+�3+�2)" should likely be "(2×2×3×2)" or "(223*2)" to properly represent the calculation of 24 possible combinations.
| // The profiling environment variable feature has 24 possible combinations (2+�2+�3+�2): | |
| // The profiling environment variable feature has 24 possible combinations (2*2*3*2): |
3 participants
Summary
This PR adds support for configuring profiling behavior through environment variables, allowing users to enable profiling without modifying application code. This allows users to enable profiling without modifying application code, useful for debugging and performance analysis
Description
ORT_ENABLE_PROFILINGenvironment variable to enable/disable profilingORT_PROFILE_FILE_PREFIXenvironment variable to set output file prefixMotivation and Context
This feature is useful for:
Testing
Documentation
docs/ProfilingEnvironmentVariables.mdwith: