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

test collate_request w batch_timeout and batch_size #238

Merged
merged 7 commits into from
Aug 28, 2024

Conversation

aniketmaurya
Copy link
Collaborator

@aniketmaurya aniketmaurya commented Aug 28, 2024

Before submitting
  • Was this discussed/agreed via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

Fixes the collate_request function to handle batch_timeout=0, right now it is not able to return requests from the queue.

Before

Test fails for the case when batch_timeout=0, collate_request didn't return any item from the queue.

image

After

image

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in GitHub issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@aniketmaurya aniketmaurya changed the title add test for collate_request with batch_timeout and batch_size test collate_request w batch_timeout and batch_size Aug 28, 2024
@aniketmaurya
Copy link
Collaborator Author

aniketmaurya commented Aug 28, 2024

Our test fails in case of batch_timeout=0. Updating the code to handle this case.
image

@aniketmaurya
Copy link
Collaborator Author

added a fix to handle batch_timeout=0

Copy link

codecov bot commented Aug 28, 2024

Codecov Report

Attention: Patch coverage is 70.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 82%. Comparing base (a24b444) to head (318b24c).
Report is 2 commits behind head on main.

Additional details and impacted files
@@         Coverage Diff         @@
##           main   #238   +/-   ##
===================================
- Coverage    82%    82%   -0%     
===================================
  Files        13     13           
  Lines      1045   1055   +10     
===================================
+ Hits        854    861    +7     
- Misses      191    194    +3     

@aniketmaurya aniketmaurya self-assigned this Aug 28, 2024
@williamFalcon williamFalcon merged commit d597528 into main Aug 28, 2024
20 checks passed
@williamFalcon williamFalcon deleted the aniket/test-batchtimeout branch August 28, 2024 16:00
@williamFalcon
Copy link
Contributor

nice job @aniketmaurya. great format. let's formalize it in our template.

Copy link
Collaborator

@lantiga lantiga left a comment

Choose a reason for hiding this comment

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

Looks good, I agree with keeping the loops separate so it's more readable

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

Successfully merging this pull request may close these issues.

3 participants