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

Exclude torch.compile time from metrics computation #31443

Merged

Conversation

zxd1997066
Copy link
Contributor

@zxd1997066 zxd1997066 commented Jun 17, 2024

When using torch.compile in accelerator.prepare_model, compilation time is being included while computing statistics. And this may result in incorrect performance metrics calculations. This PR measures time after the torch.compile called in accelerate to exclude the compilation time.

@zxd1997066 zxd1997066 force-pushed the xiangdong/exclude_compile_time branch from b30517e to fb37d01 Compare June 17, 2024 02:24
@zxd1997066 zxd1997066 changed the title Exclude compile time from metrics computation Exclude torch.compile time from metrics computation Jun 17, 2024
@zxd1997066 zxd1997066 marked this pull request as ready for review June 17, 2024 03:01
@amyeroberts
Copy link
Collaborator

cc @muellerzr @SunMarc

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Copy link
Member

@SunMarc SunMarc left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @zxd1997066 ! I think it would be better if we can do similar to what we did with f"{metric_key_prefix}_jit_compilation_time".

        if f"{metric_key_prefix}_jit_compilation_time" in output.metrics:
            start_time += output.metrics[f"{metric_key_prefix}_jit_compilation_time"]

@zxd1997066
Copy link
Contributor Author

Thanks for the PR @zxd1997066 ! I think it would be better if we can do similar to what we did with f"{metric_key_prefix}_jit_compilation_time".

        if f"{metric_key_prefix}_jit_compilation_time" in output.metrics:
            start_time += output.metrics[f"{metric_key_prefix}_jit_compilation_time"]

Hi @SunMarc , thanks for the review, I will take a look at it.

@zxd1997066 zxd1997066 force-pushed the xiangdong/exclude_compile_time branch 3 times, most recently from 0e09f5c to 5f105f4 Compare June 24, 2024 06:39
@zxd1997066 zxd1997066 force-pushed the xiangdong/exclude_compile_time branch from 5f105f4 to 28151ba Compare June 24, 2024 15:38
Copy link
Member

@SunMarc SunMarc left a comment

Choose a reason for hiding this comment

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

This is a lot better @zxd1997066 , thanks for iterating ! Could you fix the quality issue by typing make style ?

@zxd1997066
Copy link
Contributor Author

This is a lot better @zxd1997066 , thanks for iterating ! Could you fix the quality issue by typing make style ?

Fixed the quality issue, thanks!

@zxd1997066
Copy link
Contributor Author

Hi @SunMarc , could you please help merge this PR if all good? I do not have access. Thanks!

@SunMarc SunMarc requested a review from amyeroberts July 3, 2024 10:48
@SunMarc
Copy link
Member

SunMarc commented Jul 3, 2024

Hey @zxd1997066, I've pinged @amyeroberts for a final review !

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Thanks for your PR!

@LysandreJik LysandreJik merged commit d19b5a9 into huggingface:main Jul 5, 2024
21 checks passed
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.

5 participants