RelationalAI / rai-sdk-python

The RelationalAI Software Development Kit (SDK) for Python.
Apache License 2.0
17 stars 4 forks source link

[RAI-25151]: Fix polling duration in ```poll_with_specified_overhead``` function #151

Closed meruyert93 closed 4 months ago

meruyert93 commented 4 months ago

Fix polling duration calculation in poll_with_specified_overhead function.

It resolves RAI-25151

vilterp commented 4 months ago

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 commented 4 months ago

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 😄

vilterp commented 4 months ago

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 commented 4 months ago

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

vilterp commented 4 months ago

Hm, looks like tests failing with

Exception: max tries 1 exhausted
meruyert93 commented 4 months ago

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

Screenshot 2024-05-04 at 17 43 01
meruyert93 commented 4 months ago

@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 commented 4 months ago

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