Skip to content

Conversation

NRHelmi
Copy link
Contributor

@NRHelmi NRHelmi commented Oct 26, 2022

EDITED: This PR adds support to polling with overhead the the python sdk.
Polling delay is x% overhead of the time the transaction has been running so far.
By default the overhead is 10% no default value is set
for transactions async the overhead is 20% similar to what we have in golang sdk

railib/api.py Outdated
overhead_rate: float = 0.1,
start_time: datetime = datetime.now(timezone.utc),
timeout: int = None,
max_retries: int = None
Copy link

Choose a reason for hiding this comment

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

max_retries is never used by any tests or examples. Speaking of which, why do we need max_retries when we have timeout?

Copy link
Contributor Author

@NRHelmi NRHelmi Oct 26, 2022

Choose a reason for hiding this comment

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

this was inspired from the julia sdk https://github.com/RelationalAI/rai-sdk-julia/blob/main/src/api.jl#L126-L157
timeout and max_retries could/should co-exist a polling library for python do the same
https://github.com/justiniso/polling/blob/master/polling.py#L44

Copy link

Choose a reason for hiding this comment

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

Then add tests. Also, 'retry' is usually something you do when you get an error. max_tries from the polling library seems more correct.

railib/api.py Outdated

def poll_with_specified_overhead(
f,
overhead_rate: float = 0.1,
Copy link

Choose a reason for hiding this comment

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

Is 10% the standard for transactions? Seems a bit high.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

railib/api.py Outdated
break

retries += 1
duration = (datetime.now(timezone.utc) - start_time).total_seconds() * overhead_rate
Copy link

Choose a reason for hiding this comment

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

Why do we use time.time() above and datetime.now() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we use datetime to convert transaction created_on timestamp to utc datetime (the comparison is based on utc timezone) which will require extra processing if using time.time()
we simply use time.time() to check timeout, we can also use datetime

@NRHelmi NRHelmi merged commit c81fd92 into main Nov 1, 2022
@NRHelmi NRHelmi deleted the hnr-polling-feature branch November 1, 2022 12:23
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