-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[MLIR] Update Maintainers.md: add Python bindings under "Core" #153410
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-mlir Author: Mehdi Amini (joker-eph) ChangesSee https://discourse.llvm.org/t/mlir-project-maintainers/87189 Full diff: https://github.com/llvm/llvm-project/pull/153410.diff 1 Files Affected:
diff --git a/mlir/Maintainers.md b/mlir/Maintainers.md
index c31b5c635fba3..77d415976f17c 100644
--- a/mlir/Maintainers.md
+++ b/mlir/Maintainers.md
@@ -27,6 +27,17 @@ dialects, build system and language bindings.
[@joker-eph](https://github.com/joker-eph) (GitHub),
mehdi_amini (Discourse)
+### Python Bindings
+
+ - Maksim Levental \
+ maksim.levental@gmail.com (email),
+ [@makslevental](https://github.com/makslevental) (GitHub),
+ makslevental (Discourse)
+ - Rolf Morel \
+ rolf.morel@intel.com (email)
+ [@rolfmorel](https://github.com/rolfmorel) (GitHub),
+ rolfmorel (Discourse)
+
## Egress
MLIR components pertaining to egress flows from MLIR, in particular to LLVM IR.
|
Although the original RFC states that the bindings are under Can you tell me in which way you see these as "core" to upstream? |
I see the same distinction as C++, what is "non-core" in MLIR are specific dialects (TOSA, linalg, etc.) or components well isolated for a specific use-case (like a GPU runtime or something similar). In the bindings, I would differentiate the infra (which is "core") from the specific of each dialects. For example:
|
This ontology is intra-component not inter-component i.e., it distinguishes between "core" subcomponents of any component (core and non-core dialects) but does not as far as I can see inductively translate into a single component being "core". If we take your definition then, for example, most of the utils suddenly become "core" since they're generically applicable. So I believe the appropriate term here is generic rather than core (and indeed you've used it). I agree the bindings are generic but I'm still not convinced they're core to MLIR. To wit: you could remove them and absolutely nothing would fail in-tree except their own tests (and @grypp's integration tests) - this aligns with your bucketing too (if the GPU runtimes were removed, nothing would fail either other then their own tests and integration tests). |
They are.
Seems like a tautology: I can remove almost anything in tree and the only things that will fail are the things that depend on the feature... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what this PR accomplishes. The currently open PR to add core sub-categories (#152136) already adds Python Bindings to Core. Why adding another competing PR?
Occam razor: I wasn't aware of your PR... The way you're questioning it comes across a bit weird to me. |
That's certainly not true; if you remove DialectConversion then the unit tests for DialectConversion will and all the actual dialect conversions will fail; if you remove PDL/PDLL then transform dialect uses will fail; if you remove DRR then all those uses will fail. |
You approved that PR... |
What are "actual dialect conversions"? |
I'm not sure we'll get anywhere: you're saying the python bindings are different because the project being written in C++ then the other non-core component in tree can't use it directly to implement transformations? |
As @makslevental mentioned, the ontology is flawed. We should not read too much into what "core" / "egress" / "tensor" means, as they were the initial step, not the end result. For lack of a better place, we put python bindings under "core" because it's not egress/tensor related, but we can very well create another "unrelated" or "standalone" categories (possibly without "top" maintainers), so that we can bundle the loose parts of MLIR. But it doesn't matter at this point, it's all nomenclature. We shall converge to a better state at some point, but for now, I'd like to get the original PRs merged and the initial list in tree. After that, we can start creating RFCs and PRs to change the current state. |
See https://discourse.llvm.org/t/mlir-project-maintainers/87189