Conversation
|
This is a draft PR for now because I'm making these changes based on pytorch documentation and recommendations for multi device. I need to get access to multi device machines in order to test properly. The lack of AMD machine availability for testing is very annoying. I don't have any access through work (currently). And AMD didn't respond to my application for developer grants for this project. |
|
@vbharadwaj-bk I tested this on a multi device host and verified the problems the branch is trying to address exist in main and verified that using the device guard works as intended. There are a few aspects to this change.
I have not tested this on a AMD machine. I cannot get access. I need help testing this aspect of the change. |
vbharadwaj-bk
left a comment
There was a problem hiding this comment.
This rolls in a few different changes. Some comments:
-
Prefer the stream_ordering_test.py bio e in a separate PR (and would like a few more details about the failure mode you are trying to catch here).
-
The multi device changes are good.
-
[No action needed] Regarding the BLAS handle - there's a tricky problem with the existing codebase. cuBLAS is not ABI-stable across major versions, so if the user has a CUDAToolkit that's not the same as PyTorch's version (which could well be the case, as PyTorch installs just the NVIDIA libraries it needs from a Python distribution channel), then we have a mismatch and may not be able to find certain symbols at link time. So far, it has worked because we load torch first and the symbols it loads are consistent with the ones we have compiled against.
Switching to PyTorch's BLAS handle is a step in the right direction; there's no good way to solve this problem without distributing multiple package versions.
Can run some of the testing in a bit.
|
Thanks for the review.
Whatever the solution, it would be nice to have a strategy that we can port for other accelerators. |
|
I went back and thought about this more. The core problem is that we get the cublas handle but don't know which version of cublas it's from. I think a cleaner solution is there is a stable torch c shim that allows you to do batched matmul "inplace" aka with out specified. Which is all we were doing with the BLAS calls. This would let us make torch do the proper handling of the different torch versions for us. I think this is great. This is the only way we can really get away from needing to do the oeq-cuda-128 , oeq-cuda-130. Which would have been a real drag. So on a separate branch, I'm going to try to move us off of the direct BLAS calls onto a torch stable abi c shim. That will separately resolve the most sketchy part of this PR. then this PR can just focus on device guards, using the right streams, ect. |
|
Cool; or if there is a solution using python-callable group_gemm or any higher level Python API (https://pytorch.org/blog/accelerating-moes-with-a-triton-persistent-cache-aware-grouped-gemm-kernel/), that would be preferable. The only reason this is C++ at all is to express something that Python could not, at the time. |
|
The aten grouped gemm op is incomplete / not really real. We recreate the grouped gemm behavior by calling batched matmul many times. We could move this into the python level with a for loop and batched matmul calls. Or with a custom triton op. I wanted to make a smaller change so it's easier to review, so the PR I'm making keeps the structure the same, a loop inside the op in c++: instead of calling blas, we call torch's out of place batched matmul op. Long term I know how and want to get rid of the grouped gemm calls by making a specialized kernel for symmetric. If we want something sooner, because we want to bring this to Jax as well, then yeah, I think we'd have to move the batched matmul calls up to the framework level. I think freeing us from having to link BLAS is a good step on its own. Let me polish up the PR and then we can discuss it directly. |
|
see #220 |
This is a PR to harden multi device behavior.
device, which device to use.