Pin fsspec to use default expand_path
#1681
Conversation
|
The documentation is not available anymore as the PR was closed or merged. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1681 +/- ##
==========================================
+ Coverage 81.77% 81.95% +0.18%
==========================================
Files 62 62
Lines 7121 7123 +2
==========================================
+ Hits 5823 5838 +15
+ Misses 1298 1285 -13
☔ View full report in Codecov by Sentry. |
| if version.parse(fsspec.__version__) < version.parse("2023.5.0"): | ||
| # The default implementation in fsspec<2023.5.0 does not allow passing custom kwargs (e.g., we use these kwargs to propagate the `revision`) | ||
| def expand_path( | ||
| self, path: Union[str, List[str]], recursive: bool = False, maxdepth: Optional[int] = None, **kwargs | ||
| ) -> List[str]: |
There was a problem hiding this comment.
What happens then on recent versions (fsspec>=2023.5.0)? It defaults back to the expand_path implemented in fsspec directly?
There was a problem hiding this comment.
Yes, expand_path allows passing **kwargs in versions >=2023.5.0 (thanks to fsspec/filesystem_spec@834115f), so there is no need to override it.
There was a problem hiding this comment.
Ah I see, so the implementation is just a copy of the official fsspec one.
In your opinion, could we pin fsspec>=2023.5.0 in our dependencies to avoid having to support older versions?
Otherwise if you feel strongly against it, PR looks good to me. I'd just add a comment linking to https://github.com/fsspec/filesystem_spec/blob/4f0eb488f26299592df5cc6494a5ebd8cb8cca4f/fsspec/spec.py#L1139 to say that it's a duplicate (at first I understood that we has a custom implementation here, in addition of the kwargs).
There was a problem hiding this comment.
Pinning the minimal version sounds good to me :)
There was a problem hiding this comment.
For instance, Colab builds the env with version 2023.6.0, so this should have little impact on the user experience.
There was a problem hiding this comment.
Good to know! Thanks for making the change.
…e-default-expand_path
mariosasko
changed the title
expand_path in newer fsspec versionsfsspec to use default expand_path
There was a problem hiding this comment.
Thanks for iterating on the PR @mariosasko ! Removing code always feels good 😄
CI is failing on an unrelated test so we are good to merge 🎉
... to avoid having to update
HfFileSystem.expand_pathwhen new changes are made to the default implementation (e.g., the latestfsspecrelease (2023.9.0) added amaxdepthparam to the signature (PR))