-
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
Seek file before retry #94
Conversation
deepomatic/api/http_helper.py
Outdated
if hasattr(f, 'seek'): | ||
f.seek(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about non-seekable files? maybe we should fail instead of keeping the current bug of sending empty data?
Related issue in google storage python client: googleapis/google-resumable-media-python#165
they mandate seekable streams. (that being said I remember seeing in their code that they memory buffered everything that is in flight and not yet confirmed by API, via application-level multi part uploads, so it's strange... maybe it has changed?)
or maybe it doesn't make sense for the files
argument to have non seekable inputs; but the scenario should apply to other ways of passing input data: data
argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which non seekable files are you refering to ? Overall either those are bytes or stream that once read need to be reseeked.
I don't think the bug will still be there bug after this PR.
I don't want to break the current behavior now, though we can think about it for later (I would like also to remove the magic one day #33)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for reference, after meeting with hugo and some tests:
files
accepts non-streamable too, the real constraint is: if it's streamable and non seekable, then we cannot support retries: raise error on second try (maybe on first too?)- also need to keep supporting non streamable objects: strings, bytes, bytearrays, etc.
- also,
isinstance(file, io.IOBase)
is not enough: raw iterators work too, notably Django Files: https://docs.djangoproject.com/en/3.1/_modules/django/core/files/base/#File
data
accepts generator/iterator, or a dict of such thing: hard to check all that for now. maybe types constraints will help.
try: | ||
# for pip >= 10 | ||
from pip._internal.req import parse_requirements | ||
except ImportError: | ||
# for pip <= 9.0.3 | ||
from pip.req import parse_requirements | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe only fix the bug in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prevented me to install it on my computer, it would be nice to include it in this PR
@@ -359,7 +359,7 @@ def download_file(url): | |||
filename = os.path.join(tempfile.gettempdir(), | |||
hashlib.sha1(url.encode()).hexdigest() + ext) | |||
if os.path.exists(filename): # avoid redownloading | |||
logger.info("Skipping download of {}: file already exist in ".format(url, filename)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe only fix the bug in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flake got bumped automatically, I think either we freeze it, or if we let it flexible we can embed those kind of small fixes in PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requirements-dev.txt should be fully frozen, even for python libs: it's not used by the lib users, it's OK to freeze/pin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok it wasn't in requirement.dev so i moved it there and froze it
Co-authored-by: Thomas Riccardi <thomas@deepomatic.com>
Co-authored-by: Thomas Riccardi <thomas@deepomatic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo last typos to fix
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
When a retry is done due to an API error, the files where not seeked back to the beginning, thus the second time we were sending an empty file.
Also fixed pip install requirements