Skip to content

Expose the register as public register_constant_folder API (fix for Issue #2507) - #2967

Open
RohanMali2003 wants to merge 4 commits into
microsoft:mainfrom
RohanMali2003:constant-folding-registry-issue-2507-fix
Open

Expose the register as public register_constant_folder API (fix for Issue #2507)#2967
RohanMali2003 wants to merge 4 commits into
microsoft:mainfrom
RohanMali2003:constant-folding-registry-issue-2507-fix

Conversation

@RohanMali2003

Copy link
Copy Markdown

Summary

Exposes the internal constant-folding registration decorator as a public API: onnxscript.optimizer.register_constant_folder.

Previously, registering a custom constant-folding rule required importing the protected register function from onnxscript.optimizer._constant_folding directly, which linters flag as a private-member import. This re-exports it under a public, more descriptive name, following the same pattern used in #2773 for FOLDED_FROM_KEY.

Closes #2507

Changes

  • onnxscript/optimizer/__init__.py: re-export _constant_folding.register as register_constant_folder, added to __all__.
  • onnxscript/optimizer/_constant_folding.py: added a docstring to PartialEvaluatorRegistry.register documenting the callback signature, parameters, and a usage example.
  • onnxscript/optimizer/_constant_folding_test.py: added test_register_constant_folder, which registers a folder for a mock op under a dedicated test domain (test.custom), verifies fold_constants invokes it, and cleans up the registration in a finally block.

Scope notes

  • Only the register decorator is exposed; other PartialEvaluatorRegistry internals (e.g. lookup_evaluators) remain private.
  • The callback signature references OptimizerContext/OptimizerState, which are currently internal types. I've documented the expected signature informally in the docstring rather than exposing those types — happy to expose them too if preferred.

Testing

  • pytest onnxscript/optimizer/_constant_folding_test.py -v — all passing
  • pytest --doctest-modules onnxscript/optimizer — all passing
  • pytest tests/optimizer/test_models.py -v — all passing (broader sanity check)
  • lintrunner — clean

Sidenote:

I developed and documented this with AI-assisted tooling; all changes were reviewed, and verified by running the full test suite, doctests, and lintrunner locally before submission.

@RohanMali2003

RohanMali2003 commented Jul 16, 2026 via email

Copy link
Copy Markdown
Author

Copilot AI review requested due to automatic review settings July 30, 2026 20:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR exposes the internal constant-folding registration decorator as a public API, enabling users to register custom constant folders via onnxscript.optimizer.register_constant_folder instead of importing from a private module.

Changes:

  • Re-exported _constant_folding.register as the public register_constant_folder API and added it to onnxscript.optimizer.__all__.
  • Added documentation to PartialEvaluatorRegistry.register describing the expected callback signature and providing an example.
  • Added a unit test that registers a custom folder under a test-only domain and verifies it is invoked by fold_constants.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
onnxscript/optimizer/__init__.py Re-exports the registration decorator as register_constant_folder and adds it to the public API surface.
onnxscript/optimizer/_constant_folding.py Adds docstring documentation for the registration decorator/signature and a usage example.
onnxscript/optimizer/_constant_folding_test.py Adds a unit test covering public registration and invocation during constant folding.

Comment thread onnxscript/optimizer/_constant_folding_test.py Outdated
Comment thread onnxscript/optimizer/_constant_folding.py
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 30, 2026 20:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

onnxscript/optimizer/_constant_folding_test.py:884

  • This test is meant to demonstrate the public optimizer.register_constant_folder API, but the callback signature type-annotates state with the private _constant_folding.OptimizerState. That undermines the goal of avoiding private-member references in user-facing examples.
        def fold_custom_add(node: ir.Node, op, state: _constant_folding.OptimizerState):

onnxscript/optimizer/_constant_folding.py:284

  • The new public API’s docstring refers to internal types (OptimizerContext/OptimizerState/ReturnValue) and uses an example that isn’t self-contained (it assumes register_constant_folder is already in scope). Since this docstring is what users will see via onnxscript.optimizer.register_constant_folder, it should avoid requiring private imports and clarify how the op builder is intended to be used.
        """Register a custom constant folding rule for an operator.

        The decorated function must have the signature:
        `(node: ir.Node, op: OptimizerContext, state: OptimizerState) -> ReturnValue`

@RohanMali2003

Copy link
Copy Markdown
Author

Hi @justinchuby , gentle ping on this PR when you have a chance!

Also, I noticed the Copilot bot just flagged that the callback signature in the test and docstring still references the internal OptimizerContext and OptimizerState types. As I noted in the PR description, I held off on making those public to keep the scope tight, but I am more than happy to expose them as well if you'd prefer! Just let me know what direction you want to take. thanks!

@justinchuby

Copy link
Copy Markdown
Collaborator

Thanks! This is a welcomed change but I may need to take some time before I can review because my plate is full with other work.

@justinchuby justinchuby added this to the 0.7.3 milestone Jul 30, 2026
@RohanMali2003

Copy link
Copy Markdown
Author

No problem at all, and thanks for the heads-up, boss! Take your time, and I'll address any feedback whenever you get the chance to review. No rush! :)

@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.58%. Comparing base (9ef0aac) to head (2fa1f97).

Files with missing lines Patch % Lines
onnxscript/optimizer/_constant_folding_test.py 89.47% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2967      +/-   ##
==========================================
- Coverage   72.63%   72.58%   -0.05%     
==========================================
  Files         265      265              
  Lines       32204    32224      +20     
  Branches     3041     3043       +2     
==========================================
  Hits        23391    23391              
- Misses       7779     7794      +15     
- Partials     1034     1039       +5     

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

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.

Constant-Folding Registry

3 participants