-
Notifications
You must be signed in to change notification settings - Fork 7
[transform] Basic example of applying a schedule to a payload #12
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
Conversation
Simple wrapper around upstream's transform_interpreter API. Invokable directly from Python and via the commandline on separate payload and schedule files.
python/examples/generate_payload_and_schedule_and_apply_latter_to_former.py
Outdated
Show resolved
Hide resolved
python/examples/generate_payload_and_schedule_and_apply_latter_to_former.py
Outdated
Show resolved
Hide resolved
|
|
||
| def example_schedule() -> ir.Module: | ||
| schedule = ir.Module.create() | ||
| schedule.operation.attributes["transform.with_named_sequence"] = ir.UnitAttr.get() |
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.
It would be better if the transform module provided this as a helper, so that users only need to add the transforms themselves.
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.
Indeed, we should probably be facilitating dealing with such details a bit more. The thing I would not like is to obscure that we are just constructing IR, i.e. to me this code still reflects what the .mlir will look like. We should find a middle ground somehow. So, the helpers should look like IR builders as well ... probably?
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.
On returning to the PR: I think this is only non-ergonomic in that one must intersperse operation between schedule and attributes (an upstream issue). Beyond that, this API usage properly reflects how IR is build, in as terse a manner as upstream supports.
Separately, we could of course have helpers to, e.g. to wrap named_sequences up in appropriate modules. As that's not entirely trivial, e.g. multiple sequences will live in the same module, so we cannot simply wrap a single named_sequence, I think we can punt this until we start observing the kind of replication we get across the repo.
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'd like to dissociate a bit the transform helpers from actual IR, because while the underlying IR shape may change, the API should not. If all our APIs follow the IR shape closely, then we'll have to change their usage every time the IR changes, and that doesn't scale.
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.
Sure, we can improve this. I don't think the right solution is obvious though (go ahead and play around with it -- I am sure @makslevental agrees) and don't think this is the PR to solve it. Lets just merge this and address the right kind of wrappers in a dedicated PR (such a more focused PR is also like to engage the ex-Brium folks more, I imagine).
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 agree with Renato's feedback here.
At a higher level, I am already struggling to parse the code-structure within Lighthouse. For example:
- Ingress here: https://github.com/llvm/lighthouse/tree/main/python/examples/ingress/torch
- Another ingress here: https://github.com/llvm/lighthouse/tree/main/ingress/mlir-gen/mlir_gen (also,
mlir-genandmlir_genin one path).
What's the intended difference between the two? I feel similarly about https://github.com/llvm/lighthouse/tree/main/python/examples and how that dir is evolving.
IMO, we should think carefully about the structure and these initial PRs are key. While I agree that it's hard to design future-proof APIs, there's scope to improve modularity here. And to identify clear boundaries between "components" within this PR. Specifically (*):
- Payload IR generation ("Ingress").
- Schedule IR generation ("Auto-tunning").
- Driver, i.e. generate payload -> apply schedule ("Execution + driver").
There's yet another step - testing - that is also included here. These are all fairly fundamental design elements.
I am not advocating for us identifying the perfect solution within this PR and I am happy for us to iterate in-tree. That said, I will leave some inline comments and do suggest that the newly added example is more modular.
(*) Using labels from https://github.com/llvm/lighthouse/wiki/Integrator#design-points in "".
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.
FYI, the discussion on general infrastructure is here:
#16
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.
The structuring of the repo is a distinct point from what this chain was about before: more convenience helpers.
I agree that mlir-gen (as a python module name: mlir_gen) should move inside python/lighthouse/ingress or else into a tools dir (and, as I've said before, IMO that leading python dir is redundant). I will do this clean-up in another PR👍
For the examples dir, I currently only see a single quirk: python/examples/mlir/compile_and_run.py. Otherwise the hierarchy mirrors that of the modules in python/lighthouse. Did you have something else in mind?
As for testing, I will now enable CI with this PR.
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.
and, as I've said before, IMO that leading
pythondir is redundant
+1
It also just hides examples which is better to have at the top-level. As a new comer to a repo, I want to find that first.
For the examples dir, I currently only see a single quirk: python/examples/mlir/compile_and_run.py. Otherwise the hierarchy mirrors that of the modules in python/lighthouse.
I don't think examples need to match overall modules structure. They largely do now as it makes sense thematically i.e., how to import a torch model into MLIR uses ingress modules (not saying current structure is perfect or immutable). But examples could be more abstract then individual modules lighthouse provides and/or span across multiple modules.
Closer structural mirroring would make sense for a test dir which we could use soon.
python/examples/generate_payload_and_schedule_and_apply_latter_to_former.py
Outdated
Show resolved
Hide resolved
adam-smnk
left a comment
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.
Really neat and easy to use 👍
python/examples/generate_payload_and_schedule_and_apply_latter_to_former.py
Outdated
Show resolved
Hide resolved
python/examples/generate_payload_and_schedule_and_apply_latter_to_former.py
Outdated
Show resolved
Hide resolved
Now requires eb9d56c (or later) of llvm-project
python/examples/generate_payload_and_schedule_and_apply_latter_to_former.py
Outdated
Show resolved
Hide resolved
banach-space
left a comment
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.
This is very exciting - thanks for driving this forward!
Before we land it, I think we should get CI set up and confirm that the current changes actually run end-to-end. Right now, I haven’t been able to run anything from Lighthouse. This might just be an issue with my local setup, but without CI we can’t easily tell whether it’s a configuration problem or a deeper design/assumption issue.
python/examples/generate_payload_and_schedule_and_apply_latter_to_former.py
Outdated
Show resolved
Hide resolved
python/examples/generate_payload_and_schedule_and_apply_latter_to_former.py
Outdated
Show resolved
Hide resolved
python/examples/generate_payload_and_schedule_and_apply_latter_to_former.py
Outdated
Show resolved
Hide resolved
banach-space
left a comment
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.
Thanks for the updates!
I am leaving some more comments inline. Also, now that we have CI, could you add this example there? Thanks!
python/examples/schedule/transform_a_payload_according_to_a_schedule.py
Outdated
Show resolved
Hide resolved
| from mlir.dialects.transform import structured | ||
|
|
||
|
|
||
| def example_payload() -> Module: |
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.
Lets avoid generic names like @example_payload and use something descriptive instead. For example, what name would we use for the next example? @example_payload_1? That doesn't scale 😅
How about generate_payload_two_matmuls_and_add? We could skip generate_payload if the filename was ... generate_payload.py or something similar ;-) Yes, I do think that having separate files would help.
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.
Just to preface, I see your point about setting a good example (pun intended) around naming.
Not sure if it's needed in this particular case. At least from the the perspective how I approach it here.
If we go with a more granular approach of multiple files with small examples working together (like you propose in another comment), then it might need different design approach, indeed.
I'd argue that specificity adds more information and implies that sth about the exact form/shape/implementation is important in a presented item. This addition can add to or distract from the core message.
I see this file as a self-contained example that focuses primarily on mechanism behind taking two MLIR modules: payload IR and a schedule, and executing them.
As such, I doubt there's need for scaling. Each standalone example script could have @example_payload as long as that specific payload doesn't matter for the overall idea we're communicating.
This particular IR could be an empty function and little would change (% lit checks and perhaps some user confusion due to "uselessness" of a schedule doing effectively nothing).
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.
Thank you, @adam-smnk ! That captures my perspective on what is happening here very well!
python/examples/schedule/transform_a_payload_according_to_a_schedule.py
Outdated
Show resolved
Hide resolved
| cmdline = [ | ||
| "python", | ||
| "-m", | ||
| "lighthouse.schedule", | ||
| schedule_file.name, | ||
| payload_file.name, | ||
| ] |
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'd rather we didn't generate python invocation lines from within Python. This means that there is no separation of concerns and it's quite hard to extract/learn what exactly needs to happen (i.e. what the steps are).
In particular, to me, this script is trying to achieve three things in one go:
- Generate Payload IR (most likely generating Linalg or Vector Ops)
- Generate Schedule IR (generates TD Ops).
- Generate cmdline and invoke it (orthogonal to MLIR generation).
These are 3 separate tasks, each of which comes with its own set of complexities and challenges. Also, ATM, both __main__.py and transform_a_payload_according_to_a_schedule.py are runnable. So, IIUC, there are two ways to run transform_a_payload_according_to_a_schedule.py? If "yes", why?
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'd rather we didn't generate python invocation lines from within Python.
It's removed now.
The three tasks are just because it's the minimal thing we need for a full example. For non-example code, the code for the distinct tasks will be more structured.
|
|
||
| def example_schedule() -> ir.Module: | ||
| schedule = ir.Module.create() | ||
| schedule.operation.attributes["transform.with_named_sequence"] = ir.UnitAttr.get() |
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 agree with Renato's feedback here.
At a higher level, I am already struggling to parse the code-structure within Lighthouse. For example:
- Ingress here: https://github.com/llvm/lighthouse/tree/main/python/examples/ingress/torch
- Another ingress here: https://github.com/llvm/lighthouse/tree/main/ingress/mlir-gen/mlir_gen (also,
mlir-genandmlir_genin one path).
What's the intended difference between the two? I feel similarly about https://github.com/llvm/lighthouse/tree/main/python/examples and how that dir is evolving.
IMO, we should think carefully about the structure and these initial PRs are key. While I agree that it's hard to design future-proof APIs, there's scope to improve modularity here. And to identify clear boundaries between "components" within this PR. Specifically (*):
- Payload IR generation ("Ingress").
- Schedule IR generation ("Auto-tunning").
- Driver, i.e. generate payload -> apply schedule ("Execution + driver").
There's yet another step - testing - that is also included here. These are all fairly fundamental design elements.
I am not advocating for us identifying the perfect solution within this PR and I am happy for us to iterate in-tree. That said, I will leave some inline comments and do suggest that the newly added example is more modular.
(*) Using labels from https://github.com/llvm/lighthouse/wiki/Integrator#design-points in "".
python/examples/schedule/transform_a_payload_according_to_a_schedule.py
Outdated
Show resolved
Hide resolved
|
Have simplied so it is hopefully easier to go in. The commandline functionality is now gone (can be re-introduced when we need it). Also runs in CI now (will add CHECK lines back upon enabling CI with lit -- the next PR to be opened). And to note: |
banach-space
left a comment
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.
Thanks!
Simple demonstration of applying a schedule to a payload.
Relies on the little wrapper method
NamedSequenceOp.apply, now existing upstream, to make things as simple as can be.