-
Notifications
You must be signed in to change notification settings - Fork 906
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
Unpin fsspec
#3481
Unpin fsspec
#3481
Conversation
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Why do we want to catch the error if it explicitly saying it's a permission error? |
My thinking was that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just a QQ:
pyproject.toml
Outdated
@@ -58,7 +58,7 @@ test = [ | |||
"blacken-docs==1.9.2", | |||
"black~=22.0", | |||
"coverage[toml]", | |||
"fsspec<2023.9", # Temporary, newer version causing "test_no_versions_with_cloud_protocol" to fail | |||
"fsspec>=2021.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the new error handling compatible with 2021.4<=version<2023.9
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the older versions the error will be caught later on in this check https://github.com/kedro-org/kedro/blob/main/kedro/io/core.py#L544
It's not possible to keep it fully backwards compatible, because the test-coverage would then fail and in different ways depending on which version of fsspec
is installed. It's just a test-requirement though, so backward compatibility isn't as important as for the regular dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we then pin the test requirement at >2023.9
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up just removing fsspec
from the test requirements, because it wasn't included before the pin was needed.
Signed-off-by: Merel Theisen <49397448+merelcht@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Description
Fixes #3102
Development notes
In the newer
fsspec
versions permissions are checked at a different stage so the following line already threw an error:The AWS Access Key Id you provided does not exist in our records.
https://github.com/kedro-org/kedro/blob/main/kedro/io/core.py#L539
I added a try except to handle this.
Developer Certificate of Origin
We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a
Signed-off-by
line in the commit message. See our wiki for guidance.If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.
Checklist
RELEASE.md
file