Warn when exporting a @script() function under the placeholder domain this

[SIDDARTHA REDDY (SIDDARTHAREDDY8)]

Fixes #3044.

@script() with no explicit opset silently defaults to values.Opset("this", 1), so the exported function lands in .onnx files under the literal domain "this" — which reads like a real namespace and collides when two such models are composed (e.g. via onnx.compose), as seen in real Hugging Face uploads.

This change emits a UserWarning from OnnxFunction.to_model_proto() when the function being exported still carries the placeholder domain, pointing users at passing an explicit opset (e.g. @script(opset16)). Warning-only: no behavior change, and functions with an explicit domain are unaffected.

Testing: the edited module compiles cleanly (py_compile); the change is additive and guarded, so existing export paths are unchanged apart from the warning. I could not run the full test suite in this environment.

[Copilot]

Copilot review overview

🟡 Changes recommended

The warning checks the root function instead of the local functions actually serialized into the model, and regression tests are missing.

Review effort: Balanced
Findings: 2

Open (2)

• Check bundled called functions for default-domain collisions · New
• Add regression tests for default and explicit function domains · New

What changed in this PR

Adds a warning intended to prevent exporting local functions under the placeholder this domain.

Changes:

• Imports Python warning support.
• Emits a UserWarning during model export.

File	Description
onnxscript/​_internal/​values.py	Adds placeholder-domain warning logic.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[SIDDARTHA REDDY (SIDDARTHAREDDY8)]

Addressed the Copilot review feedback:

• The export warning now checks the local functions actually bundled into the model (get_called_functions()) instead of the wrapper function's domain. A plain @script() model with no local functions no longer warns, while a model bundling a default-domain @script() helper does.
• Added tests/this_domain_warning_test.py covering both sides of the warning contract: bundling a default-domain helper warns; bundling only explicitly-domained helpers, or no local functions at all, does not.

Verified locally: 3/3 new tests pass, and the existing external_tensor_test (to_model_proto path) still passes.

[Copilot]

Copilot review overview

🟡 Changes recommended

The warning incorrectly promises that the suggested standard opset produces a collision-free domain.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Add regression tests for default and explicit function domains
• Avoid claiming collision-free identifiers for opset16 · New

Resolved since last review (1)

• Check bundled called functions for default-domain collisions

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.73%. Comparing base (e33b96c) to head (f3c249b).
⚠️ Report is 15 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3046   +/-   ##
=======================================
  Coverage   72.73%   72.73%           
=======================================
  Files         265      265           
  Lines       32358    32361    +3     
  Branches     3069     3070    +1     
=======================================
+ Hits        23536    23539    +3     
  Misses       7779     7779           
  Partials     1043     1043           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[SIDDARTHA REDDY (SIDDARTHAREDDY8)]

Addressed Copilot review finding (discussion r4189880748): the warning no longer claims script (@script)(opset16) gives a collision-free domain - opset16 uses the empty standard domain, so the text now only promises a non-placeholder domain. Pushed f3c249b.

[Copilot]

🟢 Approval recommended

The focused warning is correctly scoped to bundled local functions and is adequately tested.

1 open finding

• Add regression tests for default and explicit function domains

1 resolved since last review

• Avoid claiming collision-free identifiers for opset16

🧠 Review effort: Balanced

[Mohammed Alkindi (MohammedAlkindi)]

The suggested fix produces an invalid model. Running _model_with_explicit_domain_helper.to_model_proto() from the new test gives a local function in domain "", and both onnx.checker.check_model(full_check=True) and onnxruntime reject it with "No Op registered for _explicit_domain_helper with domain_version of 17". The this variant passes both. A custom domain, values.Opset("my.domain", 1) with default_opset=op, checks and runs cleanly, so the warning text and the negative test could point there instead of @script(opset16).

[SIDDARTHA REDDY (SIDDARTHAREDDY8)]

Thanks for catching this. You are right: script (@script)(opset16) puts the helper in the empty domain, and that model fails the checker. Updated in 3e3ce9f: the warning now points to a custom domain (e.g. script (@script)(Opset('my.domain', 1), default_opset=opset17)) and explains the empty-domain problem, and the negative test now uses Opset("my.domain", 1) with default_opset=opset17 for the explicit-domain helper. Verified locally: the bundled helper lands in my.domain, onnx.checker.check_model(full_check=True) passes, the placeholder-domain case still warns, and all 3 tests in this_domain_warning_test.py pass.