-
Notifications
You must be signed in to change notification settings - Fork 52
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
ENH: Add data loader class, install at root and data modules #816
Conversation
6786973
to
008277f
Compare
f81a873
to
58af61c
Compare
Tests passing. Anybody have time for a review? |
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.
This is cool! If I'm understanding it right, this can be used as a dataloader across the *preps ecosystem?
|
||
return "\n".join(doclines) | ||
|
||
def readable(self, *segments) -> Traversable: |
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.
Since it seems like this step is often, if not always, followed with a read_text()
call, wdyt about adding another method (read_text
?) to serve as some syntactic sugar?
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 like the explicitness of loader.how_to_load(what_to_load).do_something()
.
Yes. I would probably just vendor it in the local I think it's a handy util but not sure it's stable enough to really want to deal with it as a dependency. I did an initial pass in BIDS, and then while working on this refactor found I wanted to explicitly avoid caching. |
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.
looks great!
Just to confirm - "vendor it in" means copying the infrastructure across packages (for a few versions)? |
Yes |
Proposed in nipreps/niworkflows#816. Resolves: #377. Co-authored-by: Chris Markiewicz <[email protected]>
Fixes current test failures from using a deprecated
importlib.resources
API.Expanded on an
ExitStack
-based load function that @mgxd used in one of these projects into a class that provides flexible access to package data with readable, unzipped, and cached access methods.We have the following loaders:
niworkflows.load_resource
- Generic, can get anything from anywhereniworkflows.data.load
- Primary access point for package dataniworkflows.tests.data.load_test_data
- Convenience for testsniworkflows.interfaces.tests.data.load_test_data
- Convenience for interface testsI would like to figure out how to bundle the library in a zip to actually test that this works when installed that way, but for this PR the goal is to make it easy to think about and select whether we need the resource to be a
Path
or justTraversable
.This also finally purges
pkg_resources
, which we should avoid because we have no runtime dependency on setuptools, and the API is discouraged in third-party packages.