Skip to content

Fix default cupti time converter #1064

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DeadSonger
Copy link

I use Kineto as a tracer connected to applications via injection. By default, a time converter is used that does not correspond to the real time on the execution machine, and can also have a multiple accelerated time flow (for example, the time indicator in cupti can grow twice as fast as the time indicator on the executable machine). To solve this problem, this patch has been prepared, which corrects the previously indicated inaccuracies. As a result, the corrected version allows you to get after conversion the time matched with the real one with a small but constant offset (the offset is the same for all events)

@sraikund16
Copy link
Contributor

sraikund16 commented Mar 26, 2025

Seems reasonable to me. Typically we do this conversion in PyTorch profiler and then we set the kineto converter there. See: https://github.com/pytorch/pytorch/blob/3a8171efadd54c79453451156cf982f45d36ed66/c10/util/ApproximateClock.cpp#L41

If you are only using Kineto you will need this change but I am wondering if it would be more accurate to use a sampling approach like the one I linked above.

@facebook-github-bot
Copy link
Contributor

@sraikund16 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@sraikund16
Copy link
Contributor

Hi, so it seems like this change is causing several tests to fail. We can fix them but I was wondering if instead of changing the default within kineto you can change it using the get_time_converter call like we do in pytorch?

https://github.com/pytorch/pytorch/blob/dfcd98e684123b0cb0a143d8718b0672c58ec268/torch/csrc/autograd/profiler_kineto.cpp#L418

@DeadSonger
Copy link
Author

DeadSonger commented Apr 2, 2025

Hi, we compile KINETO as shared library and start it using CUDA_INJECTION64_PATH, there is no way to change converter in runtime in this case
May be we can select time converter using environment variable? for example KINETO_CUPTI_TIME_CONVERTER=[SIMPLE, SCALE]
And by default use current realisation of time converter

@sraikund16
Copy link
Contributor

That works for me! Feel free to update with env var instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants