Conversation
| if hasattr(f, 'seek'): | ||
| f.seek(0) |
There was a problem hiding this comment.
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: https://github.com/googleapis/google-resumable-media-python/issues/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.
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.
for reference, after meeting with hugo and some tests:
filesaccepts 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
dataaccepts 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.
nit: maybe only fix the bug in this PR?
There was a problem hiding this comment.
This prevented me to install it on my computer, it would be nice to include it in this PR
| 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.
nit: maybe only fix the bug in this PR?
There was a problem hiding this comment.
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.
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.
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>
thomas-riccardi
left a comment
thomas-riccardi
left a comment
There was a problem hiding this comment.
LGTM modulo last typos to fix
|
Kudos, SonarCloud Quality Gate passed!
|
2 participants
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