-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add retry for curl requests #223
Conversation
for more information, see https://pre-commit.ci
I think this looks good but perhaps we need an importable function that can help users do this |
I was toying around with some ways to do that; it was proving a tad tricky since the user (or ourselves) might wish to call any executable code on the open with retry_s3_call(max_retries=10):
# try to do something on an open io where the thing you try to do could be as simple as |
Though I suppose that a context is simply not possible for that kind of thing... https://stackoverflow.com/a/64770597 Perhaps a simple function like def try_until_success(command, attempts=1):
for _ in range(attempts):
try:
return command()
except Exception as exc:
err = exc
raise err might work (from https://stackoverflow.com/a/64770679) |
yeah that would be cool but I don't know if that's possible. Usually contexts pass in an object that is used within the context, e.g. with open("fpath", "r") as f:
# use f here I think the best approach might be to do two things:
robust_read(nwbfile.acquisition["ElectricalSeries"].data, slice(1, 10), max_retries=5) what do you think? |
One tricky thing is that the HDF5 Datasets themselves don't have information about whether they are s3 or local. I can't think of a way to automatically switch to retrying if ros3 is being used. I suppose we could just always retry, but even that is tricky because we are interfacing with the h5py.Dataset objects directly. There are lots of ways we could potentially approach this. Let's discuss in our meeting today. |
That's what I've done in latest commit using a very similar function to what you outlined in Could also put in switch at the corresponding deployment levels to determine whether to do it at the classic way or S3 way. Will discuss this later, then. |
Summary: this patch is fine for now, it hits most of the major error points where this can occur - long term solution is to support Otherwise, a more sophisticated approach would be a decorator that wraps functions in the registry (and |
fix #197
Attempting to resolve curl requests errors to S3 paths, up to
max_retries
using the suggested exponential back-off