Skip to content
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

json: Add an offset to sorting job #196

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

Conversation

MatthewKhouzam
Copy link
Contributor

The json trace files can have events before the "traceevents": key. This patch allows the parser to start sorting on the correct entry.

This allows traces generated by pytorch to be read by the incubator as they add out-of-spec metadata information.

Example trace generated:
stable-diffusion.json

Example of how the trace can help accelerate pytorch:
lllyasviel/stable-diffusion-webui-forge#716

Devs can see the impact of the arguments they are passing to the API.

This patch needs an incubator counterpart.

The json trace files can have events before the "traceevents": key.
This patch allows the parser to start sorting on the correct entry.

Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Copy link
Contributor

@arfio arfio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the processMetadata method should be used instead of just skipping the bytes ? I do not think that adding an additional constructor is warranted here whereas there is already a mechanism in place that should work if the metadata is before or after the events in the json format.

@MatthewKhouzam
Copy link
Contributor Author

We still need to skip forward, would having processmetadata return a start offset be OK in your opinion? I am trying, I like your approach but I think the caffeine is failing me.

@arfio
Copy link
Contributor

arfio commented Dec 10, 2024

We still need to skip forward, would having processmetadata return a start offset be OK in your opinion?

process metadata is called at the end of events, but I am not sure what the reasoning was when this was written. I think it should be called before reading and sorting the events, if the metadata is at the end of the file, then it is the responsibility of the implementation to find and read the metadata information.
Or it could be a setting where the implementation can return if the metadata is at the beginning or at the end.
In any case the implementation has access to the BufferedInputStream and can skip the bytes itself.

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

Successfully merging this pull request may close these issues.

2 participants