search

Azure / azure-openai-benchmark

Azure OpenAI benchmarking tool
MIT License
129 stars 54 forks source link

Value for MAX_RETRY_SECONDS should not be hardcoded #36

Open sushaanttb opened 8 months ago

sushaanttb commented 8 months ago

Currently, the value of MAX_RETRY_SECONDS is hardcoded in code,

image image

It would be a good idea to make this value configurable.

Relates to - #33

michaeltremeer commented 8 months ago

I am copy-pasting my response from your other issue for anyone else who reads this issue:

Hey @sushaanttb,

Have a look at https://github.com/Azure/azure-openai-benchmark/pull/21 and the discussion within. You will see some more info there about what is going on with different retry settings, and how MAX_RETRY_SECONDS effects the outputs. That PR has not been merged and it doesn't look like it will, so if you want the behaviour you suggest, I would suggest you use my fork which fixes this issue and uses a more suitable value of 60 seconds for MAX_RETRY_SECONDS.

sushaanttb commented 8 months ago

Hey @michaeltremeer

Thanks for the comment!

I did had a look on the existing issues, including Issue#20 but as it was marked closed and then as your PR was also "still" not merged in the main branch, I was a bit confused on why it was still not merged in the main repo and as it was also becoming a bit confusing. I accordingly re-raised some of these issues again.

Btw, your PR got merged yesterday after I raised those issues, so congratulations and great work!.

Lastly, I like how the documentation is now more clear and also saw your results with different MAX_RETRY_SECONDS,, in which 60s was found to be giving better results.

But the point is it's still hardcoded, & thus should we not still give the option to make it configurable? We can keep the default to 60s.If not, we should def. add those benchmark stats results for reference in the README for anyone trying to understand why 60s was chosen.