Skip to content

Conversation

YichengDWu
Copy link
Contributor

@YichengDWu YichengDWu commented Aug 31, 2025

This PR refactors the Value.from_mlir class method in max/graph/value.py. The previous implementation relied on a growing if/isinstance chain, which has been replaced with a robust, self-registering factory pattern.

Changes Made

  • The Value base class now leverages __init_subclass__ to act as a self-registering factory.
  • The factory automatically registers new Value subclasses by introspecting the generic alias in their base class definition (e.g., Value[mo.TensorType]) to infer the associated MLIR type.
  • This change completely eliminates the conditional block for creating Value instances from MLIR types.

Benefits of this Change

This refactoring improves the software design in several key areas:

  • Decoupling: The Value base class is now fully decoupled from its concrete subclasses. It no longer needs explicit knowledge of them.
  • Open/Closed Principle: The system is now open for extension but closed for modification. New Value types can be added without any changes to the base class, significantly improving maintainability and extensibility.

@YichengDWu YichengDWu requested a review from a team as a code owner August 31, 2025 08:05
Copy link

github-actions bot commented Aug 31, 2025

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@YichengDWu
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

modular-cla-bot bot added a commit to modular/cla that referenced this pull request Aug 31, 2025
@YichengDWu YichengDWu force-pushed the refactor/value-factory branch from b41b242 to 6d50b64 Compare August 31, 2025 09:02
Signed-off-by: YichengDWu <yichengdwu@outlook.com>
@YichengDWu YichengDWu force-pushed the refactor/value-factory branch from 6d50b64 to 0274b3c Compare August 31, 2025 09:05
@JoeLoser JoeLoser requested a review from bethebunny September 4, 2025 21:41
@JoeLoser
Copy link
Collaborator

JoeLoser commented Sep 4, 2025

@bethebunny do you mind taking a look at this, please? Thanks!

@bethebunny
Copy link
Contributor

Hi @YichengDWu, thank you so much for the contribution! Impressive use of Python, I like this pattern in general :) That said

Open/Closed Principle: The system is now open for extension but closed for modification. New Value types can be added without any changes to the base class, significantly improving maintainability and extensibility.

We have no plans to extend the system here, by contrast we want to simplify the type system and make opaque types and tensor values use the same mechanism.

Your pattern is a great application of the principle of "Don't Repeat Yourself", but in this case I would rather defer to "Keep it Simple" / "Yain't Gonna Need It": we won't add more if/else clauses in the future, and with so few cases I prefer the explicitness if the if/else checks.

@bethebunny bethebunny closed this Sep 6, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Sep 6, 2025
@bethebunny bethebunny reopened this Sep 6, 2025
@bethebunny
Copy link
Contributor

I'm going to close this PR, but feel free to continue discussion or re-open if you think there's a better explicit pattern.

@bethebunny bethebunny closed this Sep 6, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants