-
Notifications
You must be signed in to change notification settings - Fork 0
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
Reevaluate manual retry logic #20
Comments
(In the long-term, we want to implement the same polling policy in all the SDKs: https://github.com/RelationalAI/rai-sdk-issues/issues/58. But in the meantime, just removing this if statement would help.) |
Oh, @NRHelmi, we need to fix this before merging RelationalAI/rai-cli#17, otherwise all transactions longer than 10 seconds will appear to time out at the CLI! 😮 |
@NHDaly the count is there to abort if we exceed the 5 errors but I think it will poll forever if everything is running fine |
Hm, I guess the count only gets incremented if there's an error: Line 1240 in 2b1fdd6
This would be easier to see if the variable is called Should we just bail out at the first error? Is there any precedent for this "only bail out after 5 errors" policy in other SDKs? |
ooooooooooooh i see! Thank you for clarifying! So this is like a retry policy? I think that the Julia SDK handles retries automatically through the Julia SDK: Does the Golang HTTP library do any kind of retries for us automatically already? |
I guess this was added per this comment, here: I dunno what's standard here. I agree that bailing on the first error seems reasonable to me? |
Note: This issue has been migrated to https://relationalai.atlassian.net/browse/RAI-6269. This link is only accessible to employees of RelationalAI. |
I think we should just remove this if statement; it should be allowed to poll forever:
rai-sdk-go/rai/client.go
Line 1243 in 2b1fdd6
The Go-idiomatic thing might be to pass in a context for this function, and on each iteration check whether the context is cancelled. (though I'm not sure whether this should go cancel the transaction on the RAI side…)
cc @NRHelmi
The text was updated successfully, but these errors were encountered: