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

Bump fsspec #1805

Merged
merged 2 commits into from
Aug 25, 2022
Merged

Bump fsspec #1805

merged 2 commits into from
Aug 25, 2022

Conversation

antonymilne
Copy link
Contributor

@antonymilne antonymilne commented Aug 24, 2022

Description

Resolves #1804.

A reminder that we should at some point we should:

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes

@@ -4,7 +4,7 @@ cachetools~=4.1
click<9.0
cookiecutter>=2.1.1, <3.0
dynaconf>=3.1.2, <4.0
fsspec>=2021.4, <=2022.1
fsspec>=2021.4, <=2022.7.1
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why you pinned it to 2022.7.1 and not just 2022.7?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2022.7.1 is the latest release that fixes bugs in 2022.7. Pinning to <2022.8 wouldn't make sense here since they're following calver.

@noklam noklam mentioned this pull request Aug 24, 2022
5 tasks
@noklam
Copy link
Contributor

noklam commented Aug 24, 2022

reposting @datajoely comment here.

I appreciate we're taking a conservative approach here, but why not widen the scope to all of 2022 or beyond? We've not had any breaking issues since fsspec moved to CalVer and users only report this sort of issue when it's too late.

@antonymilne
Copy link
Contributor Author

@datajoely:

I appreciate we're taking a conservative approach here, but why not widen the scope to all of 2022 or beyond? We've not had any breaking issues since fsspec moved to CalVer and users only report this sort of issue when it's too late.

This is a very good point I think. Personally I think we are generally too conservative with upper bounding dependencies at the moment and should consider changing our policy on it, but I'm pretty sure @idanov would disagree strongly with this 😅

When fsspec first introduced CalVer I think we allowed a 6 month forward-looking window. I'd be happy to go to <2023 or even consider no upper bound. But again I think @idanov would hate this.

@MerelTheisenQB - thoughts? I'm happy to go with whatever you think is best here. @deepyaman I know thinks about these things and so would have an opinion here also 🙏 Maybe the least controversial solution would be to go along with our current conservative strategy for now but reconsider the policy in general and try to push forward with something like dependabot to automatically bump dependencies.

@antonymilne
Copy link
Contributor Author

antonymilne commented Aug 24, 2022

Also @jenikovsky - as a kedro user, any thoughts on this? I see you ❤️ ed @datajoely's comment so would probably be in favour of a more relaxed upper bound so we we're not always playing catch up and relying on users to report when things aren't compatible? It's tricky to know what to do with these calver libraries.

@merelcht
Copy link
Member

@MerelTheisenQB - thoughts? I'm happy to go with whatever you think is best here. @deepyaman I know thinks about these things and so would have an opinion here also 🙏 Maybe the least controversial solution would be to go along with our current conservative strategy for now but reconsider the policy in general and try to push forward with something like dependabot to automatically bump dependencies.

I think it's definitely worth revisiting our bumping strategy, but I'm not sure we can apply one strategy to all dependencies. fsspec is quite a fundamental dependency for our datasets so I'd be more careful in how loosely we pin it. We've also seen in the past that not all libraries stick to releasing non-breaking changes in major versions only, so not having any bounds will just lead to an even greater amount of issues related to version bumps in dependencies.

@datajoely
Copy link
Contributor

Could we look at dependabot auto-opening PRs for this library? Presumably if the test suite passes we're good to go.

Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

Thanks, I agree dependent bot will be a good fit here.

@Galileo-Galilei
Copy link
Contributor

Can't agree more with using dependabot and being confident in your test suite which is very solid! These conflicts between kedro dependencies and other packages may be very annoying and it would be very nice to update upper bounds much more frequently.

@jenikovsky
Copy link

Keeping up using dependabot makes a lot of sense.
Thank you guys for your super quick action on this request. When could this change be roughly released?

@antonymilne
Copy link
Contributor Author

Cool, thanks for the comments everyone. Let's do as I originally suggested then: keep this bound <=2022.7.1 and make a new ticket for us to try out dependabot.

@jenikovsky I think we should be doing a release pretty soon - within the next week or so would be my guess.

@antonymilne antonymilne merged commit 4843fad into main Aug 25, 2022
@antonymilne antonymilne deleted the bump-fsspec branch August 25, 2022 09:17
deepyaman pushed a commit to deepyaman/kedro that referenced this pull request Sep 12, 2022
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
@jenikovsky
Copy link

@jenikovsky I think we should be doing a release pretty soon - within the next week or so would be my guess.

Hi @AntonyMilneQB, would you have any updates on when the next release might be happening? Thank you

@antonymilne
Copy link
Contributor Author

Hi @jenikovsky, sorry for the delay on this - it's taking longer than expected indeed. We are still hoping to do one very shortly though 🤞

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.

Review fsspec upper boundary to allow use of >=2022.5.0
6 participants