[XPUPTI] Add missing stream field in a trace.
#1151
Conversation
|
Hi @tsocha! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
| const std::set<ActivityType>& activity_types, | ||
| const libkineto::Config& config) { | ||
| return std::make_unique<XpuptiActivityProfilerSession>( | ||
| const auto experimental_cuda_format_enabled = is_env_set("XPUPTI_USE_EXPERIMENTAL_CUDA_FORMAT"); |
There was a problem hiding this comment.
- Why call it CUDA format? It's defined by CUDA or HTA?
- What's the difference btw this format and original one? Looks like no more detail in this PR.
- Why have to introduce a env var? Generally, it's not a good idea to depend on a env var...
There was a problem hiding this comment.
Ad. 1. It is the first change in a series. The goal is to produce our traces in a way that it looks like CUDA trace.
Ad. 2. The first step is to produce a stream param. I plan to add other changes like renaming some operators in our trace to match CUDA's naming.
Ad. 3. I don't like the env var either, but I didn't get a better idea how to parametrize it. Any feedback how to do it better is welcome.
There was a problem hiding this comment.
The goal is to produce our traces in a way that it looks like CUDA trace.
I have no idea about the cuda trace. Could you show related links or examples? Thanks.
I don't like the env var either, but I didn't get a better idea how to parametrize it. Any feedback how to do it better is welcome.
Per this PR, I cannot understand your motivation, as there's no real change so far. I guess, finally, you will only keep one path, right? Could you show your design here?
There was a problem hiding this comment.
The goal is to produce our traces in a way that it looks like CUDA trace.
I have no idea about the cuda trace. Could you show related links or examples? Thanks.
Some examples of differences:
- CUDA produce a field named
streambut XPU producesycl queueandl0 queue.- This PR add
streamfield.
- This PR add
- Operators for queue start and end and named differently:
urEnqueueUSMMemcpyvscudaLaunchKernelExC- this is the next step to do.
- For distributed communication kernels it have
ncclprefix in CUDA.
I don't like the env var either, but I didn't get a better idea how to parametrize it. Any feedback how to do it better is welcome.
Per this PR, I cannot understand your motivation, as there's no real change so far. I guess, finally, you will only keep one path, right? Could you show your design here?
My motivation is to produce a trace which could be accepted in tools which does not have XPU support and expect traces produced by CUDA.
This PR introduce one simple change to add a missing field stream but I expect more changes.
Some of these changes can replace operators name or categories.
I don't want to break existing solution.
That's a reason why I have decided to create a separate class instead of modifying already existing one.
For some time there will be 2 paths but in the future we can remove/merge one of them.
There was a problem hiding this comment.
@tsocha These are not cuda format. These are API/interface names of cuda software stack, like stream, cudaLaunchKernelExC, nccl. I don't think we must show identical API names in the trace or UI. And we even cannot do that, as we cannot guarantee we are always able to find APIs in XPU software stack with same semantics.
If needed, we have to do some change in kineto or HTA to fit our requirement, instead of totally following CUDA for all, right?
My motivation is to produce a trace which could be accepted in tools which does not have XPU support and expect traces produced by CUDA.
Instead, we should improve the tool to support XPU. If you're talking about HTA, we should submit PR to HTA if needed. But I assume in most case HTA should be device-agnostic?
There was a problem hiding this comment.
@gujinghui
I see why you don't want to introduce this adapter into kineto, I will focus on changes in HTA.
So another question:
Do you agree to produce a stream param in our traces?
HTA requires it, but we produce l0_queue and sycl_queue.
We can just reuse sycl_queue as our stream.
This is a minimal change and will fix many issues here and in potential other tools.
I can see that stream field is used by CUPTI, ROCTRACER and AIUPTI.
PS in pytorch code stream is already used in xpu context.
example:
stream1 = torch.xpu.current_stream()
stream_tmp = torch.xpu.Stream(torch.xpu.current_device())
stream2 = torch.xpu.Stream(torch.xpu.current_device())
tsocha
changed the title
tsocha
changed the title
stream field in a trace.
3 participants
Currently tools like HTA accept traces which are strictly targeted for CUDA.
This PR is a MVP which allow traces gathered on XPU to be accepted by such tools.
Adapter introduced in this PR will be extended with more capabilities.