-
Notifications
You must be signed in to change notification settings - Fork 8
[testing] Tests mark themselves UNSUPPORTED if features aren't available #35
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
|
E.g. if you didn't install torch dependencies (e.g. did And if you then install Without this change the above UNSUPPORTED tests would show FAILED instead. |
tkarna
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.
LGTM. At the moment we are only testing the "everything is installed" case in CI, right?
examples/xegpu_matmul/matmul.py
Outdated
| @@ -1,4 +1,5 @@ | |||
| # RUN: %PYTHON %s --dump-kernel=xegpu-wg | FileCheck %s | |||
| # RUN: python %s --dump-kernel=xegpu-wg | FileCheck %s | |||
| # REQUIRES: torch | |||
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.
At the moment torch is indeed required because lighthouse/utils/runtime_args.py requires it. This test does not actually use torch though.
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.
Have now split out the torch dependent code to its own module. This file no longer unnecessarily REQUIRES torch.
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.
@adam-smnk could you have a look at the split in lighthouse.utils? I don't mind doing it differently, e.g. shallower namespacing.
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.
Split itself looks good 👍
Naming-wise - I don't think these functions fully fit runtime category. I'd stick with more shallow utils or tools.
Also, having our own torch.py module shadowing large library feels a bit wrong. How about torch_utils or another suffix like torch_ffi? Maybe ffi dir if you prefer more structure.
Just to help with differentiation and to avoid the need for extra aliasing during importing.
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.
Fixed it in e13be22.
Common way of using it should now look like lh_utils.torch.dtype_from_mlir_type and lh_utils.memref.to_packed_args etc. (Which took some time to get right ... though now we get to avoid a torch_utils module in lighthouse.utils/ lighthouse.tools 🥳 ).
|
why is the kernelbench still unsupported after installing torch? |
The example speaks for itself 😉 |
e5dd205 to
efff1b1
Compare
Indeed. If people want this UNSUPPORTED mechanism tested in CI as well, do let me know. |
b58358f to
84fd5fb
Compare
Also loosens dependency upon `uv` for running tests -- all that is required is that `lit` is running in a suitable environemnt (e.g. through `uv run lit .` or by first entering an venv and then `lit .`).
Means at least one more example can run without torch being installed.
1# with '#' will be ignored, and an empty message aborts the commit.
84fd5fb to
64bf0ab
Compare
64bf0ab to
e13be22
Compare
Also loosens dependency upon
uvfor running tests -- all that is required is thatlitis running in a suitable environment (e.g. throughuv run lit .or by first entering an venv and thenlit .).Also splits
lighthouse.utilsinto two so that the examples/tests that don't actually depend ontorchaccidently end up with that dependency (there was one such such instance already).