Skip to content

Expose op builder to the python API using a macro class#4747

Merged
causten merged 7 commits into
developfrom
macro-op-builder
Apr 15, 2026
Merged

Expose op builder to the python API using a macro class#4747
causten merged 7 commits into
developfrom
macro-op-builder

Conversation

@pfultz2

@pfultz2 pfultz2 commented Apr 7, 2026

Copy link
Copy Markdown
Collaborator

Motivation

This exposes the op builder to the API, but uses slightly different API. Instead there is a m.add_instructions method that will take a macro that will then insert the ops from the op builder. I chose macro as it is short and descriptive of what it does.

Technical Details

Changelog Category

Add a CHANGELOG.md entry for any option other than Not Applicable

    • Added: New functionality.
    • Changed: Changes to existing functionality.
    • Removed: Functionality or support that has been removed. (Compared to a previous release)
    • Optimized: Component performance that has been optimized or improved.
    • Resolved Issues: Known issues from a previous version that have been resolved.
    • Not Applicable: This PR is not to be included in the changelog.

Copilot AI review requested due to automatic review settings April 7, 2026 00:55
@pfultz2
pfultz2 requested a review from causten as a code owner April 7, 2026 00:55

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 C++ op builder API to the Python bindings via a new migraphx.macro type, enabling users to insert builder-expanded instruction sequences into a module with add_instructions / insert_instructions.

Changes:

  • Add migraphx.macro(name, **options) Python type to carry an op-builder name plus options.
  • Add module.add_instructions(macro, args, mod_args=[]) and module.insert_instructions(ins, macro, args, mod_args=[]) Python APIs backed by op::builder::{add,insert}.
  • Add Python tests covering macro metadata and several builder expansions (add/mul/sub/broadcast/gemm, plus insertion).

Reviewed changes

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

File Description
src/py/migraphx_py.cpp Adds macro binding and module methods that call op::builder::add/insert.
test/py/test_macro.py New test suite validating macro/options and builder insertion behavior via the Python API.
test/py/CMakeLists.txt Registers the new test_macro.py in the Python test CMake targets.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov

codecov Bot commented Apr 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4747      +/-   ##
===========================================
+ Coverage    92.43%   92.49%   +0.06%     
===========================================
  Files          583      583              
  Lines        29244    29562     +318     
===========================================
+ Hits         27031    27343     +312     
- Misses        2213     2219       +6     

see 29 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread src/py/migraphx_py.cpp Outdated
@CharlieL7
CharlieL7 self-requested a review April 7, 2026 19:23
Comment thread test/py/test_macro.py

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

should we really be using op builders for simple ops like add, sub, mul, etc? Then along those lines, could we instead have more tests of the ops like batchnorm, convolution, einsum?

Also another minor nit is that we call the macro variable mac, but I would prefer it be named according to the op (like gemm_mac). So that it's a bit more clear

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

should we really be using op builders for simple ops like add, sub, mul, etc?

We dont actually have op builders for these operators they just fallback to migraphx operators.

The goal is to build a set of op builders that torch_migraphx can call to instead of directly adding ops. That way we can update the operators in non-backwards compatible way and it wont break. This actually blocks merging in things like #4606.

Then along those lines, could we instead have more tests of the ops like batchnorm, convolution, einsum?

I can try to update the test.

@pfultz2
pfultz2 requested a review from kahmed10 April 14, 2026 14:41
@causten
causten merged commit fc81408 into develop Apr 15, 2026
34 checks passed
@causten
causten deleted the macro-op-builder branch April 15, 2026 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants