Skip to content
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

[iree.build] Add turbine_generate iree.build rule. #249

Merged
merged 5 commits into from
Nov 27, 2024

Conversation

stellaraccident
Copy link
Collaborator

@stellaraccident stellaraccident commented Nov 2, 2024

This supports performing parallelizable aot model export as part of the iree.build tool. By default, it runs actions out of process, meaning that multiple torch.export and torch-mlir imports can be running with true concurrency.

Signed-off-by: Stella Laurenzo <[email protected]>
Signed-off-by: Stella Laurenzo <[email protected]>
@stellaraccident stellaraccident marked this pull request as ready for review November 26, 2024 23:46
Copy link
Member

Choose a reason for hiding this comment

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

@marbre any ideas for how we could include iree-turbine docs as part of https://iree-python-api.readthedocs.io/ ?

I'd like to include this page under https://iree-python-api.readthedocs.io/en/latest/compiler/build.html as part of iree-org/iree#19019 , and we can also put the rest of the iree-turbine docs up too. We should limit the number of doc sites we have to maintain, even if this crosses repository boundaries a bit :P

Copy link
Member

Choose a reason for hiding this comment

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

Tracking issue: #77

Comment on lines +51 to +54
* `ExportOutput`: The result of calling `aot.export(...)` will result in
`save_mlir()` being called on it while still in the subprocess to write to
a file names `{name}.mlir` if there is one return or `{name}_{n}.mlir` if
multiple.
Copy link
Member

Choose a reason for hiding this comment

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

Can this output .mlirbc? What about .irpa files?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It needs to grow some options for mlirbc. Have some thoughts on how to plug parameter generation through in an ergonomic way. One of:

  • Support another return type and then the generator just returns something that causes the export.
  • Have an extra_outputs= dict that lets you set up additional output files and have that passed into the generator function to do with as you please.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(but wanted to land this before adding more)

Comment on lines +132 to +142
# Do an exhaustive subclass check.
for k, m in RETURN_MARSHALLERS_BY_TYPE.items():
if issubclass(t, k):
# Cache it.
RETURN_MARSHALLERS_BY_TYPE[t] = m
return m
raise ValueError(
f"In order to use a callable as a generator in turbine_generate, it must be "
f" annotated with specific return types. Found '{t}' but only "
f"{EXPLICIT_MARSHALLER_TYPES} are supported"
)
Copy link
Member

Choose a reason for hiding this comment

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

👀 Python magic

return export(fxb)


@entrypoint(description="Builds an awesome pipeline")
Copy link
Member

Choose a reason for hiding this comment

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

🚀

@@ -10,5 +10,5 @@
# Uncomment to skip versions from PyPI (so _only_ nightly versions).
# --no-index

iree-base-compiler==3.1.0rc20241122
iree-base-runtime==3.1.0rc20241122
iree-base-compiler==3.1.0rc20241126
Copy link
Member

Choose a reason for hiding this comment

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

One of the actions is failing on this
https://github.com/iree-org/iree-turbine/actions/runs/12040949027/job/33571834502?pr=249#step:4:99

Ah, because the macOS release build failed to upload with failed, reason: write ECONNRESET 🤦 : https://github.com/iree-org/iree/actions/runs/12024344562/job/33519713662

We should

  • Change that workflow to not need all packages to exist
  • Fix the release upload (this has happened a few times lately)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll update it with tomorrow's build before landing. Technically the one from today had a bug with this stuff on Python 3.10.

Copy link
Contributor

@monorimet monorimet left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

Copy link

@renxida renxida left a comment

Choose a reason for hiding this comment

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

LGTM with a minor terminology nit!

out_of_process: bool = True,
**kwargs,
):
"""Invokes a user-defined generator callable as an action, performing turbine
Copy link

Choose a reason for hiding this comment

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

consider renaming "generator callable" to "exporter callable" and/or explain that it is a callable that returns an exported torch module.

For me, "generator callable" resolves to the python language feature, "generator functions", like

def fib():
  prev = 0
  cur = 1
  while True:
    yield cur
    prev, cur = cur, cur+prev

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm running out of daylight before vacation to land changes, but sounds like a reasonable idea. Maybe you could send a patch next week and work with Ean/Scott on a better name? I'll land it like this but it has no use yet -- easy to rename.

Copy link

Choose a reason for hiding this comment

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

sounds good & will do!

Signed-off-by: Stella Laurenzo <[email protected]>
@stellaraccident stellaraccident merged commit d1dfb3c into main Nov 27, 2024
8 checks passed
@stellaraccident stellaraccident deleted the turbine_generate branch November 27, 2024 22:34
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