Skip to content

Conversation

@astralcai
Copy link
Contributor

@astralcai astralcai commented Sep 3, 2025

Context:
Right now, preprocess and transforms have two different _operator_decomposition_gen to do recursive long decision trees. This will eventually cause much pain and difficulty, especially when we are ready to have our new graph-based decomposition to do the device preprocess as well.
Since these two methods are doing more or less the same functionality only with slightly difference in accepted arguments, it will be much beneficial if we can unify them as soon as possible.

Description of the Change:

  • devices/preprocess.py::_operator_decomposition_gen has been removed.
  • Instead, a unify _operator_decomposition_gen living in transforms/decompose.py to have the same core internal implementation for _operator_decomposition_gen.
  • In comparison, the new _operator_decomposition_gen accepts two new args:
    • custom_decomposer : to support the decomposer feature of the preprocess.decompose, which was originally supported by the removed one.
    • strict : If True, an error will be raised when an operator does not provide a decomposition and does not meet the stopping criteria. Otherwise, a warning is raised.

Benefits:
Avoid code duplication, promotes feature parity.

Possible Drawbacks:
The _operator_decomposition_gen is getting bulky.

Related GitHub Issues:
[sc-98684]

@github-actions
Copy link
Contributor

github-actions bot commented Sep 3, 2025

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@codecov
Copy link

codecov bot commented Sep 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.69%. Comparing base (93867ba) to head (7e057e0).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8193      +/-   ##
==========================================
- Coverage   99.69%   99.69%   -0.01%     
==========================================
  Files         548      548              
  Lines       56851    56845       -6     
==========================================
- Hits        56678    56672       -6     
  Misses        173      173              

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

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

@astralcai astralcai changed the title [WIP] Unified internal implementation for decompositions Unified internal implementation for decompositions Sep 4, 2025
@astralcai astralcai requested review from JerryChen97 and removed request for JerryChen97 September 4, 2025 16:21
Base automatically changed from decomp-fallback to master September 4, 2025 18:06
@JerryChen97
Copy link
Contributor

@astralcai Hopefully I didn't resolve the merge conflicts with wrongly changing anything you wrote before

@JerryChen97
Copy link
Contributor

I would say actually this PR looks already ready for review. Would you like to suggest what is remained to implement here? @astralcai

@astralcai
Copy link
Contributor Author

I would say actually this PR looks already ready for review. Would you like to suggest what is remained to implement here? @astralcai

I don't think there's anything left to do here, but since nobody has capacity to take on more reviews this week, it'll likely have to wait till next week. I'll leave it up to you to address any review comments you receive.

Copy link
Contributor

@andrijapau andrijapau left a comment

Choose a reason for hiding this comment

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

That was pretty straight forward. Thanks! LGTM.

@JerryChen97 JerryChen97 added this pull request to the merge queue Sep 5, 2025
Merged via the queue into master with commit e83288f Sep 5, 2025
52 checks passed
@JerryChen97 JerryChen97 deleted the unified-decomp branch September 5, 2025 15:40
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.

4 participants