Fix unguarded import torch in HIPBackend#9441
Conversation
|
cc @njriasan |
Arech8
force-pushed
the
arech_fix_bomb
branch
from
e924a90 to
52b23b2
Compare
| import torch | ||
| if HIPBackend._TORCH_AVAILABLE == 0: | ||
| try: | ||
| import torch |
There was a problem hiding this comment.
Maybe it's just better to do a guarded import torch on a file level, at the top. This would at least make that dependency apparent, but it could remain optional with a similar guard flag as here.
Please tell me your preference, I can change it right here.
Arech8
changed the title
import torch bomb in HIPBackendimport torch in HIPBackend
Arech8
changed the title
import torch in HIPBackendimport torch in HIPBackend
| # availability state: 0 - not tested, 1 - torch is present. Anything else - | ||
| # no torch available. First call to is_within_2gb() checks torch availability | ||
| # and caches it. | ||
| _TORCH_AVAILABLE: int = 0 |
There was a problem hiding this comment.
Can we name it as _torch_available and move to L110 to be close to other controls?
There was a problem hiding this comment.
This is not a control/knob. This is a private state tracking variable useful only as an optimization to prevent running try: import torch on each invocation of the method. If torch isn't available on the first call, there's no sensible reason to expect it'll be available on the subsequent calls, hence no sense in wasting cycles in trying to find it and handle ImportError. (we're reasonably assuming that a user won't change python package discovery paths between the first and the subsequent calls).
Hence, I don't think it's beneficial to mix this var with the control vars in L108-109, b/c var semantic is different and this var isn't supposed to be touched by anyone except the method itself. I'll rename it though.
Does this address your question?
2 participants
This PR fixes unguarded
import torchstatement introduced intothird_party/amd/backend/compiler.pyby this commit 37ff43c#diff-33c9a103282c05c9d9d213b94450ae7481b6db8c3c6d810f54f175b4735a3c72Triton isn't necessary used with torch only and the commit breaks, for example,
jax-tritonimplementation requiring a dedicated patch https://github.com/jax-ml/jax-triton/pull/371/changes#diff-2f4d625858aeb282314bd7151b9f4973006bbe4b925b1fede28ba5330dff79a6R170New contributor declaration
I am not making a trivial change, such as fixing a typo in a comment.
I have written a PR description following these
rules.
I have run
pre-commit run --from-ref origin/main --to-ref HEAD.An unexpected error has occurred: URLError: <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: self-signed certificate in certificate chain (_ssl.c:1000)>. This might be due to some corporate junk installed on the working machine. Quick googling didn't help to find a solution for this particular case. I tried to make changes in line with used style, so hopefully this is ok.Select one of the following.
/testforlittests/unittestfor C++ tests/python/testfor end-to-end teststhe change is obvious and doesn't require dedicated test. To be twice safe I tested that very code on a locally installed package.Select one of the following.
littests.littests I have added follow these best practices,including the "tests should be minimal" section. (Usually running Python code
and using the instructions it generates is not minimal.)