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

Unit Test: Add error handling for rate limit exceeded in model list #5715

Merged
merged 5 commits into from
Jul 11, 2024

Conversation

HeyangQin
Copy link
Contributor

This PR fixes the random failure in our unit test due to HTTP 429

@HeyangQin HeyangQin requested a review from tohtana July 1, 2024 23:05
@HeyangQin HeyangQin enabled auto-merge July 1, 2024 23:07
@loadams loadams disabled auto-merge July 2, 2024 17:02
@loadams loadams enabled auto-merge July 2, 2024 17:02
@adk9
Copy link
Contributor

adk9 commented Jul 2, 2024

Thanks for the fix. I think this will definitely make test_inference less flaky.

I've two high-level concerns though about what's happening here:

  1. What's the reason for the default model cache time to be 1 day? The models/tasks we're testing barely update so we could perhaps set a much longer cache time?
  2. We get rate-limited because we're making ~750k requests (getting model information for all models on HF) to test only 24 models. I think a better approach is to pass a filter to api.list_models() to get information of only the models we're interested in testing?

@loadams
Copy link
Contributor

loadams commented Jul 2, 2024

Thanks for the fix. I think this will definitely make test_inference less flaky.

I've two high-level concerns though about what's happening here:

  1. What's the reason for the default model cache time to be 1 day? The models/tasks we're testing barely update so we could perhaps set a much longer cache time?
  2. We get rate-limited because we're making ~750k requests (getting model information for all models on HF) to test only 24 models. I think a better approach is to pass a filter to api.list_models() to get information of only the models we're interested in testing?

It was re-set here, though we had it set to 1 day before this: https://github.com/microsoft/DeepSpeed/pull/5688/files

@loadams loadams disabled auto-merge July 2, 2024 17:13
@adk9 adk9 self-requested a review July 9, 2024 17:22
@loadams loadams merged commit 83aa184 into master Jul 11, 2024
8 of 11 checks passed
@adk9 adk9 deleted the HeyangQin/fix_unit_test_http branch July 11, 2024 16:49
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.

3 participants