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

Add warmup-ms argument to benchmark_requests #78

Open
wants to merge 1 commit into
base: abokovoi/upstream
Choose a base branch
from

Conversation

avbokovoy
Copy link

This PR aims to stabilize nbit_device test for small L and D sizes. Recommended safe warmup-ms value is 100

@avbokovoy avbokovoy self-assigned this Dec 17, 2024
@avbokovoy avbokovoy requested a review from kudomcho December 17, 2024 15:16
@liligwu
Copy link
Collaborator

liligwu commented Dec 17, 2024

This is my solution:
just disabled the iteration numbers. warmup and profiling runs are time-based.
diff_inf_warmup_run_time.txt

@avbokovoy
Copy link
Author

This is my solution: just disabled the iteration numbers. warmup and profiling runs are time-based. diff_inf_warmup_run_time.txt

My goal was to give user an option to choose iteration based and/or time based benchmarking. And I'm not sure that omitting this functionality is needed. Also, I'm not sure that after a proper warmup we need a time based profiling.

Anyway, I don't insist on my approach. @liligwu wdyt?

@liligwu
Copy link
Collaborator

liligwu commented Dec 17, 2024

This is my solution: just disabled the iteration numbers. warmup and profiling runs are time-based. diff_inf_warmup_run_time.txt

My goal was to give user an option to choose iteration based and/or time based benchmarking. And I'm not sure that omitting this functionality is needed. Also, I'm not sure that after a proper warmup we need a time based profiling.

Anyway, I don't insist on my approach. @liligwu wdyt?

Yeah, I understand. I'm also wondering how we should use these changes. Will we finally upstream it?

@avbokovoy
Copy link
Author

This is my solution: just disabled the iteration numbers. warmup and profiling runs are time-based. diff_inf_warmup_run_time.txt

My goal was to give user an option to choose iteration based and/or time based benchmarking. And I'm not sure that omitting this functionality is needed. Also, I'm not sure that after a proper warmup we need a time based profiling.
Anyway, I don't insist on my approach. @liligwu wdyt?

Yeah, I understand. I'm also wondering how we should use these changes. Will we finally upstream it?

From my point of view we need to do the following:

  1. Test it internally
  2. Describe the problem to the customer and suggest this PR as a solution (with default value of warmup-ms=100)
  3. Upstream it
  4. Make according changes in our internal benchmark repository

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