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

Disable TORCH_MLIR_ENABLE_JIT_IR_IMPORTER by default #3693

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

AmosLewis
Copy link
Collaborator

@AmosLewis AmosLewis commented Sep 9, 2024

Temporary fix for Disable TORCH_MLIR_ENABLE_PYTORCH_EXTENSIONS #3654

Disable TORCH_MLIR_ENABLE_JIT_IR_IMPORTER by default

  • Only enable it in CI for update_abstract_interp_lib.sh and update_torch_ods.sh usage.

TODO:

@AmosLewis AmosLewis marked this pull request as ready for review September 9, 2024 18:11
Copy link
Collaborator

@stellaraccident stellaraccident left a comment

Choose a reason for hiding this comment

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

We also need to revise some docs:

https://github.com/llvm/torch-mlir?tab=readme-ov-file#all-the-roads-from-pytorch-to-torch-mlir-dialect : The graphic can either be updated or removed. We should reference FX and ONNX as the entry points. MHLO -> StableHLO

Remove the Demos section (https://github.com/llvm/torch-mlir?tab=readme-ov-file#demos) as it refers only to demos that are now disabled by default. In its place we can do something like:

## Using torch-mlir

Torch-MLIR is primarily a project that is integrated into compilers to bridge them to PyTorch and ONNX. If contemplating a new integration, it may be helpful to refer to existing downstreams:

* IREE (link)
* Blade (link)
* Reference CPU runner (local docs)

While most of the project is exercised via testing paths, there are some ways that an end user can directly use the APIs without further integration:

* (see example of FX import)
* (see example of ONNX import)
* etc

While editing this, we should also drop the Meetings section as I don't think those are running anymore.

There are some deeper doc changes that would be useful to scrub but we can do those once fixing the front door README.

Fine to make the doc changes in a separate PR, but let's not submit this patch without at least a link to the PR with pending doc changes.

CMakeLists.txt Outdated Show resolved Hide resolved
docs/add_ops.md Show resolved Hide resolved
- Only enable it in CI for update_abstract_interp_lib.sh and update_torch_ods.sh usage.
Copy link
Collaborator

@stellaraccident stellaraccident left a comment

Choose a reason for hiding this comment

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

Updates lgtm

@AmosLewis AmosLewis merged commit fbb0db1 into llvm:main Sep 10, 2024
3 checks passed
@AmosLewis AmosLewis deleted the disable_jit_ir_importer branch September 10, 2024 05:58
@qingyunqu
Copy link
Collaborator

I have two points of concern on this PR:

  1. It seems that ci doesn't test fx_importer with tosa or stablehlo backend.
  2. GeneratedTorchOps.td was generated depend on jit_ir_importer now. What's the future plan?
    @AmosLewis @stellaraccident

@qingyunqu
Copy link
Collaborator

Sorry, I see that #3698 add test on stablehlo backend.

@stellaraccident
Copy link
Collaborator

The stablehlo test diffs were an omission, which I think now is resolved. I don't have anyone working with me who uses the TOSA side and need contributors there to configure things properly.

Regarding the ODS issues, Chi is working on a pure python solution for this. The original idea was to do that first but the situation with the c++ extension failing for users (and breaking the whole project) was becoming critical: we had a comparative spike of issue reports over the last week and my team was not able to identify any fixes. In addition, these code paths were implicated in blocking a recent llvm upgrade, consuming multiple engineers on a tangent for a week (llvm/llvm-project#105656 (comment)).

We just don't have any contributors who can maintain this code anymore (and it is the single flakiest thing in the project), and we've been saying that it is deprecated since the beginning of the year/the day will come we can't keep it running. My team is at that point, and we can't spend any more time keeping investing in it. We will continue with a fix to make the ODS generation pure python.

I see you've posted on the discourse thread. Let's discuss options there.

@sjarus
Copy link
Collaborator

sjarus commented Sep 24, 2024

Sorry for the late comment @stellaraccident - I didnt see this as I wasn't tagged. Arm can step in around TOSA - we are actively working on it and test all targets locally, with our priority being the fx_importer_tosa one that has a significantly better pass rate and rising.

PyTorch 1.0 JIT import isn't really priority, and ideally we'd focus on fx_importer_tosa and onnx_tosa . @mgehre-amd was a user of the PT1 TOSA import path - has that changed ?

@mgehre-amd
Copy link
Contributor

We are currently still using make_fx_tosa and are looking to migrate to fx_importer_tosa.

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.

5 participants