Skip to content

Conversation

meruyert93
Copy link
Contributor

@meruyert93 meruyert93 commented May 2, 2024

Fix polling duration calculation in poll_with_specified_overhead function.

It resolves RAI-25151

@meruyert93 meruyert93 requested a review from vilterp May 2, 2024 12:45
@vilterp
Copy link
Contributor

vilterp commented May 2, 2024

Definitely looks more like the Julia one now…

This is making me wish Python had fake-able timers like Jest, so we could write a nice test for this! 😅

@meruyert93
Copy link
Contributor Author

This is making me wish Python had fake-able timers like Jest, so we could write a nice test for this! 😅

yeah agree that would be indeed nice 😄

@meruyert93 meruyert93 changed the title Fix polling duration calculation in poll_with_specified_overhead function [RAI-25151]: Fix polling duration in poll_with_specified_overhead function May 2, 2024
@vilterp
Copy link
Contributor

vilterp commented May 2, 2024

It looks like there is https://docs.python.org/3/library/unittest.mock.html

Maybe you could try using that to write a small unit test for poll_with_specified_overhead, like we did for the profiler poller? Since this logic is important for performance and has caused bugs in the past, it'd be nice to have it under test.

@meruyert93
Copy link
Contributor Author

@vilterp it actually had couple tests, now I have added more tests

@vilterp
Copy link
Contributor

vilterp commented May 3, 2024

Hm, looks like tests failing with

Exception: max tries 1 exhausted

@meruyert93
Copy link
Contributor Author

@vilterp all the tests are passed. Where do you see it?

Screenshot 2024-05-04 at 17 43 01

@meruyert93
Copy link
Contributor Author

@vilterp
it was failing with Exception: max tries 1 exhausted here: https://github.com/RelationalAI/rai-sdk-python/pull/151/files#diff-f4c8f93a330162e591e147d72a2dba80b81510662e5608f2d130bf63d5220ed7R33 However, I already fixed it:
I increased max_tries so that the function doesn't exit on the first iteration. Then sse a try except block to catch the exception but still check the behavior of time.sleep, because the goal of this test is to test initial delay.

@vilterp
Copy link
Contributor

vilterp commented May 4, 2024

Oh ok, I guess the github UI didn't refresh itself and I was looking at an out of date run. Will review again on Mon

Copy link
Contributor

@vilterp vilterp left a comment

Choose a reason for hiding this comment

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

Apologies for taking so long on this!

@meruyert93 meruyert93 merged commit dfc06d3 into main May 13, 2024
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