Skip to content

docs(_framework_apis): say check_model is a deliberate no-op - #3012

Merged
Justin Chu (justinchuby) merged 2 commits into
microsoft:mainfrom
MohammedAlkindi:docs/2981-check-model-noop-docstring
Aug 25, 2026
Merged

docs(_framework_apis): say check_model is a deliberate no-op#3012
Justin Chu (justinchuby) merged 2 commits into
microsoft:mainfrom
MohammedAlkindi:docs/2981-check-model-noop-docstring

Conversation

@MohammedAlkindi

Copy link
Copy Markdown
Contributor

check_model reads """Check the model.""" with a body of del model # Unused yet. Together those say validation is pending implementation. Per #2981 it is not: the checker was removed deliberately because it can report false positives and adds overhead to export, and the function is kept for API compatibility.

This records that where someone reading the function will see it. No behaviour change; the body is still a no-op.

Wording is taken from Justin Chu (@justinchuby)'s explanation in #2981 rather than invented:

Initially it was intended that we ensure the model to be a "valid" model before returning. Later we realized (1) the checker may produce false positives and (2) it adds overhead for the export logic. So we decided to remove this step. But the api is maintained for compatibility.

Refs #2981.

Checks

ruff format --check reports the file already formatted. ruff check under the repo's pyproject.toml reports the same single pre-existing finding before and after this change (TID251 on import pathlib, line 18, untouched here), so it introduces nothing new. I diffed the two rule-code sets rather than eyeballing the counts.

I did not run the pytest suite: it needs a torch install this machine does not have. CI is the authority on that, and this change is a docstring and a comment.

Drafted with AI assistance. I read the function and its re-exports in torch_2_6.py through torch_2_11.py myself and confirmed torch_2_5.py holds the only definition, so this is the only site that needed changing.

The docstring read "Check the model." and the body read `del model  # Unused
yet`, which together suggest validation is pending implementation. It is not:
the ONNX checker was deliberately removed because it can report false positives
and adds export overhead, and the function is kept only for API compatibility.

Records that rationale where a reader of the function will see it.
The docstring read "Check the model." and the body read `del model  # Unused
yet`, which together suggest validation is pending implementation. It is not:
the ONNX checker was deliberately removed because it can report false positives
and adds export overhead, and the function is kept only for API compatibility.

Records that rationale where a reader of the function will see it.
Comment thread onnxscript/_framework_apis/torch_2_5.py Dismissed
@codecov

codecov Bot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 72.63%. Comparing base (e1fe520) to head (cbf59a4).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
onnxscript/_framework_apis/torch_2_5.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3012   +/-   ##
=======================================
  Coverage   72.63%   72.63%           
=======================================
  Files         265      265           
  Lines       32218    32218           
  Branches     3044     3044           
=======================================
  Hits        23403    23403           
  Misses       7781     7781           
  Partials     1034     1034           

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

@justinchuby
Justin Chu (justinchuby) merged commit 0e4df17 into microsoft:main Aug 25, 2026
28 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

3 participants