-
Notifications
You must be signed in to change notification settings - Fork 511
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_PYTORCH_EXTENSIONS #3654
base: main
Are you sure you want to change the base?
Conversation
d85c46b
to
3cdae9f
Compare
Get an error
@stellaraccident Any suggestions, should we just disable the TORCH_MLIR_ENABLE_JIT_IR_IMPORTER? |
Yeah, you can try that. I think we could also just move the e2e test tools out of that and into the main project. Would need to look at it some more. |
@stellaraccident The tosa/stablehlo/linalg default e2e test will use the |
abd6705
to
ddf555b
Compare
Yes, go ahead and delete. We can't support the old thing anymore and have replacements. |
Need to disable the onnx/onnx_tosa/torchdynamo e2etest as well, but this does not have replacement. |
3cd1ed6
to
a796f37
Compare
Need to change the CI test_posix.sh |
135ae96
to
8dc4e4f
Compare
Error in torch-nightly, need to delete the torch_mlir.jit_ir_importer in .sh file. |
@stellaraccident need a review. |
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 think we need to come up with a replacement for update_torch_ods.sh and possibly update_abstract_interp_lib.sh before landing this. I believe that it is mostly historical that both of them rely on that one method in the JitIR extension to get the op registry, and I believe there are more direct ways to go about that these days. Been on my list for a very long time to research this... If I recall the method they rely on is just using a C++ API to get all of the schemas and then putting them together into a JSON struct for the code generators to use. There may be a comparative API on the Python side these days, or worst case, we could just parse the op definition yaml files like PyTorch itself does. Probably not a lot of work but may take some digging.
build_tools/update_torch_ods.sh
Outdated
@@ -42,8 +42,3 @@ if [ ! -z ${TORCH_MLIR_EXT_MODULES} ]; then | |||
fi | |||
|
|||
set +u |
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.
As tempting as it is to just delete these lines and call it done, I'm not sure we can do this: people still use this tool like this.
@@ -40,8 +40,3 @@ TORCH_MLIR_EXT_MODULES="${TORCH_MLIR_EXT_MODULES:-""}" | |||
if [ ! -z ${TORCH_MLIR_EXT_MODULES} ]; then | |||
ext_module="${TORCH_MLIR_EXT_MODULES} " | |||
fi | |||
|
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.
Ditto: We can't just make this a no-op.
There's another weird thing about We should possibly remove this as part of this PR as well? Lines 238 to 242 in 9a6fe58
@stellaraccident any opinion on that? |
Yeah. Let's get the last two code generation things here separated and then do a full excision. |
Just disable/remove it. I wasn't aware a new dep like this was added, and we've been quite clear we're moving away from this. Was probably just an oversight and using the wrong thing -- the folks doing that will need to upgrade. |
a8205c8
to
b6a6958
Compare
In #3668 the symbols required by onnx e2etest have been extracted to a common interface. Now it should no longer depend on jit_ir_importer. |
Than you for catching/fixing that. I had missed it. |
b6a6958
to
2cfafa2
Compare
To fix the no module named 'torch_mlir._mlir_libs._jit_ir_importer' bug.
- To fix the bug No module named 'torch_mlir_e2e_test' when test torch-stable
- Delete torchscript related test
- To fix error when e2etest from torch_mlir import torchscript then output bug: No module named 'torch_mlir.jit_ir_importer'
… torchscript - No module named 'torch_mlir.jit_ir_importer'
- also delete the onnx and onnx_tosa flag in config
…ABLE_JIT_IR_IMPORTER
2cfafa2
to
eb9fc70
Compare
@stellaraccident
I guess the Another issue you didn't mentioned is in the call path: |
pure python ods implementation will be in PR #3780 |
Related discussion: https://discourse.llvm.org/t/drastically-reducing-documented-scope-of-project/80484
To fix the no module named 'torch_mlir._mlir_libs._jit_ir_importer' bug when test pytorch related models like
pytorch/models/mit-b0
If I use :
pip list:
Error in model-run.log
The error disappear if I
pip uninstall torch-mlir
and use my local build 0820 with this patchexport PYTHONPATH=${PYTHONPATH}:/proj/gdba/shark/chi/src/torch-mlir/build/tools/torch-mlir/python_packages/torch_mlir