Bug Bash - removing ep_policy option DISABLE#635
Conversation
Remove the DISABLE option from ep_policy in both C# and C++ ArgumentParser. Unknown policies now default to DEFAULT instead of DISABLE. Updated READMEs accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
yeelam-gordon
left a comment
There was a problem hiding this comment.
Review summary
Small, focused cleanup. Removes the DISABLE value from --ep_policy in both the C++ and C# ArgumentParser, updates the corresponding usage strings, and updates the two READMEs. The motivation in the description is sound: the old DISABLE path set ep_policy = nullopt / null, which then collided with the !ep_policy && ep_name.empty() validator and surfaced as a misleading "Missing EP selection" error.
Verdict: LGTM with one nit. Safe to merge after addressing (or explicitly accepting) the description/code mismatch below.
What looks good
- Both parsers (
Shared/cpp/ArgumentParser.cppandShared/cs/ArgumentParser.cs) are updated symmetrically. - Help text in both parsers and both READMEs is consistent:
(NPU, CPU, GPU, DEFAULT). - Unknown values now produce a valid
EpPolicy(DEFAULT) instead ofnull/nullopt, so the downstream validator no longer throws the misleading "Missing EP selection". This is the actual bug fix and it's correct. - Grep across
Samples/WindowsMLconfirms no otherDISABLEreferences remain.
Nit — description vs. behavior mismatch
The PR description says:
Unknown values now throw immediately with the list of valid options (NPU, CPU, GPU, DEFAULT), so typos and invalid inputs are caught at parse time…
But the implemented behavior is lenient, not strict:
std::wcout << L"Unknown EP policy: " << policy_str << L", using default (DEFAULT)\n";
options.ep_policy = OrtExecutionProviderDevicePolicy_DEFAULT;Console.WriteLine($"Unknown EP policy: {policyStr}, using default (DEFAULT)");
options.EpPolicy = ExecutionProviderDevicePolicy.DEFAULT;So --ep_policy NPUU will print a warning and silently run as DEFAULT — it does not throw or list valid options. Two reasonable resolutions:
- Match the description — throw on unknown values (parallel to the existing
Mutually exclusive EP options/Missing EP selectionexceptions), e.g.throw new Exception($"Unknown EP policy: {policyStr}. Valid: NPU, CPU, GPU, DEFAULT");and the C++ equivalent. This is closer to "fail fast on typo," which is what the description argues for. - Keep the lenient fallback and tweak the PR description to say "falls back to DEFAULT with a warning" instead of "throws immediately."
Either is fine; just keep the description and behavior aligned.
Minor
Default: DEFAULTin the READMEs reads a bit redundantly, but it is accurate and matches the enum name, so not worth changing.
Remove the DISABLE option from ep_policy in both C# and C++ ArgumentParser. Unknown policies now default to DEFAULT instead of DISABLE. Updated READMEs accordingly.
Description
The samples require either --ep_policy or --ep_name to be set. The following scenarios fail due to DISABLE:
Scenario 1 : User explicitly passes DISABLE: The user runs --ep_policy DISABLE. The parser sets ep_policy = null and ep_name remains empty. Validation sees neither is set and throws: "Missing EP selection" - even though the user did specify a policy.
Scenario 2 : User passes an invalid value (e.g., a typo): The user runs --ep_policy NPUU. The parser prints "Unknown EP policy: NPUU, using default (DISABLE)" and silently sets ep_policy = null. Validation again throws "Missing EP selection" : masking the real error (an invalid value)
Thus I would propose this resolution
Removed DISABLE as a valid --ep_policy option. If the user's intention is to default to CPU behavior, they can use DEFAULT instead, making DISABLE redundant.
Unknown values now fall back to DEFAULT with a warning so typos and invalid inputs are caught at parse time rather than falling through to a incorrect validation error