Open falexwolf opened 4 months ago
This might be related:
So while the API behaves as documented, unfortunately the API model from which the SDK is generated from is lacking the desired output shape.
I upgraded botocore from 1.31.17
to 1.34.84
and now it works:
Given the issues that Tim ran into and given this (https://github.com/laminlabs/lamindb/issues/1590), we should start to lower-bound and upper-bound rigorously everything that relates to aws.
The issue is that the version that works is pretty recent: 1.34.84
It'd be a rather strict requirement to depend on it.
When I look through botocore, even today, it seems like they test this response element
Ok, so, I think it's safer to use the old way of calling it.
Hence, I refactored and added these tests:
Sorry, i overlooked this, but it is not about pinning or lower bounds, using x-amz-bucket-region
is the correct way (that is how s3fs
gets regions and so on) and BucketRegion
is just not. I haven't checked the PR properly at that time. But it should be tested, i agree, thank you for fixing.
and BucketRegion is just not
Judging from the issue on aws-sdk
, I think BucketRegion
is the new correct way of doing it.
The tests don't catch it because they run on the latest versions of botocore.
Also about pinning, what is your version of botocore
that gives BucketRegion
, @falexwolf?
The problem is that we actually pin botocore through aiobotocore - botocore>=1.34.41,<1.34.70
in lamindb
, so we should not use or test with the latest version. And 1.34.84
is actually incompatible with our pins. And for our pin x-amz-bucket-region
is the correct way.
Also about pinning, what is your version of botocore that gives BucketRegion, @falexwolf?
The 1.34.84 - I asked Sunny to try that.
The problem is that we actually pin botocore through aiobotocore - botocore>=1.34.41,<1.34.70 in lamindb,
Great! Then we're all good on this front!
Hence, the only thing left is to lower and upper bound s3fs rather than pinning it.
And: we should be able to get rid of one of these two lines, shouldn't we?
Yes, i will work on this soon.
Also note that botocore 1.34.84
is incompatible with aiobotocore
, so you might have different problems due to that, better to downgrade.
Now running into this issue here - https://github.com/laminlabs/lamindb-setup/actions/runs/8835925517/job/24261282965?pr=732
Because we don't define the aws
extra on the lamindb-setup level, we might have inconsistent installation across lamindb-setup CI and lamindb CI.
To keep things in sync, we should likely define the aws extra dependencies on the lamindb-setup level and then port them to lamindb.
Making the first step in this direction here:
This is harder to manage if we have the pins in both lamindb-setup
and lamindb
.
And we also have gcp
extras, then we have to move it here.
This is really harder to manage and confusing if we scatter this extras across different packages. Can we keep it in lamindb
?
Can we keep it in lamindb?
I don't see how. If I don't have them here, the tests are failing. And it makes sense because a lot of storage functionality is already here.
if we have the pins in both lamindb-setup and lamindb.
We only have pins here. lamindb delegates to lamindb-setup.
Ok, could you move gcp
also here then?
I still think it is a bit confusing to scatter extras, but i agree that it is more convenient for testing.
Ok, could you move gcp also here then?
Will do!
I still think it is a bit confusing to scatter extras, but i agree that it is more convenient for testing.
Right now I think it's a hard requirement for testing. If we find a different solution or eventually can have a mono-repo, then things will be different.
Ok, then i will try upgrading fsspec and replacing pins with lower and upper bounds on the weekend.
Sounds great!
I just ran this with the following output:
When @sunnyosun runs it she gets the below, hence, she doesn't get any
BucketRegion
return value: