Open sebastianmika opened 3 months ago
Hi @sebastianmika, thanks for bringing this up. Previous to the addition of the auto_decompress
param in aiohttp
3.9, there's no way of supporting downloading gzipped files without decompressing them. I consider this to be a valid use case, as you may want to download a gzip file from GCS and serve it from your own server, or save it as-is to disk for later processing or whatever you may want to do with it. Automatically decompressing it just to compress it again makes no sense, so that's why we added the option to fetch it compressed in #714.
@TheKevJames thoughts on forcing aiohttp>=3.9
?
I am seeing your point and the advantage. My challenge (and I am likely not alone) is that I have plenty of aiohttp<3.9
dependencies (internal and external), why forcing 3.9
would come at quite some cost.
Looking at the recent PR I was wondering whether a very similar result couldn't have been achieved by making the user pass in a ClientSession(auto_decompress=False)
session into storage = Storage(session=session)
explicitly (i.e. the PR maybe wasn't needed?). The only scenario where that would be annoying is when I need to change this behaviour on a call by call basis. But I would not find that very likely to happen in reality.
Yes, that would be a valid option too. I was trying to achieve it on call-by-call basis as we usually have a single storage object for the whole application, and you could have different use cases within it. Still, I see that we do accept a session parameter on both Storage.download
and Blob.download
, so maybe there's a possible workaround that's not super painful. I also have to take into account how this will work with the requests.Session
object, as the codebase for gcloud-aio/rest
modules is shared.
I'll take a look at this next week, for the moment I suggest pinning the version to 9.2.0
Ah, good catch on this. I would rather avoid upgrading the aiohttp
pin too much, if it's not necessary -- it looks like 3.9
only added the auto_decompress
parameter to the request method, but it was supported in the client itself before that as early as 3.3.0 (original commit), so we can avoid needing to bump dependency ranges by doing some patching.
I plan to do the following:
auth
with aiohttp >= 3.9.0
, to make sure there's a working latest versionv5.3.0
v3.9.2
. That PR will return to the current aiohttp >= 3.3.0
lower bound.Proposed long-term fixes at #717 and #718.
Looking at the recent PR I was wondering whether a very similar result couldn't have been achieved by making the user pass in a ClientSession(auto_decompress=False) session into storage = Storage(session=session) explicitly (i.e. the PR maybe wasn't needed?).
That would have worked for aio (barring the call-by-call changes you called out), but unfortunately not for rest, since our internal usage of the stream
parameter also needs to change based on that setting. I don't think we can fully offload this while mataining feature parity between the async and sync builds of this library, though if anyone can think of a way to do so I'd love to be proven wrong!
With the latest release of
gcloud-aio-auth/storage
(5.3.0) the following breaks when usingaiohttp<3.9
, which is allowed since the minimal required version is currentlyaiohttp>=3.3.0
.Potential fixes:
aiohttp>=3.9
allow_redirects=allow_redirects
when setting up theself.ClientSession
object in line 556 ofgcloud/aio/storage/storage.py
and remove from the.get
calls (and probably other places)Code to reproduce with
aiohttp==3.8.6
: