Skip to content
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

CI: Force newer S3FS #48291

Closed
wants to merge 2 commits into from
Closed

CI: Force newer S3FS #48291

wants to merge 2 commits into from

Conversation

lithomas1
Copy link
Member

@lithomas1 lithomas1 commented Aug 28, 2022

Copy link
Member

@phofl phofl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove aiobotocore if not pinned, it's not a dependency of us

@lithomas1 lithomas1 marked this pull request as ready for review August 29, 2022 00:54
@lithomas1 lithomas1 requested a review from phofl August 29, 2022 00:54
@lithomas1 lithomas1 added this to the 1.5 milestone Aug 29, 2022
@mroeschke
Copy link
Member

Could we enforce botocore>=1.23.0 instead? Looks like the six changes occured there: boto/botocore@5d4ecca

Not totally ideal IMO that this s3fs pinning in these files is higher than our minimum version.

@lithomas1
Copy link
Member Author

Is there a reason to pin down s3fs?

mamba is pulling really old versions of s3fs by chance(I think it's happening because old s3fs are not pinning down aiobotocore, allowing for mamba to pull down the newest aiobotocore versions which are not actually compatible with s3fs). Really old s3fs working with the newest aiobotocore is more luck than anything else.

This PR is just forcing mamba not to do this by setting a lower bound to a s3fs that pins down aiobotocore to a version it can support. The minimum version is still getting tested in the min version build so I don't think we're losing coverage.

Could we enforce botocore>=1.23.0 instead? Looks like the six changes occured there: boto/botocore@5d4ecca

I haven't tested this, but I'm pretty sure aiobotocore pins/controls the botocore version(IIUC it messes around with botocore internals so they have to pin down to a specific botocore version), so doing that would probably just result in an unsolvable environment.

(As for why we care about aiobotocore, its a dependency of s3fs)

I agree with you on the point of bumping s3fs though(I think we should probably do it ASAP for 2.0).

@phofl
Copy link
Member

phofl commented Aug 29, 2022

Yeah the actual issue is that our minimum version of s3fs is not pinning aiobotocore, causing failures, hence we have to pin aiobotocore too causing the python 3.10 issues…

1 similar comment
@phofl
Copy link
Member

phofl commented Aug 29, 2022

Yeah the actual issue is that our minimum version of s3fs is not pinning aiobotocore, causing failures, hence we have to pin aiobotocore too causing the python 3.10 issues…

@mroeschke
Copy link
Member

I'll check the environment solving by lower pinning botocore quickly in #48297

Is there a reason to pin down s3fs?

Not really. I think it was May when I updated the min version of several packages using a 1-year-release-prior rule of thumb. IMO I think it would be okay to bump s3fs for 1.5 to 2021.08.0 since we advertise our min versions of optional dependencies as the ones we test with, and lower versions may very well still work.

@phofl
Copy link
Member

phofl commented Aug 29, 2022

Yeah then let’s bump this for the rc, if the solve fails

@phofl
Copy link
Member

phofl commented Aug 29, 2022

#48299, if this works out then we can close here

@lithomas1 lithomas1 closed this Aug 29, 2022
@lithomas1 lithomas1 deleted the fix-ci branch August 29, 2022 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: newer version of s3fs is pulled causing builds to fail
3 participants