Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Template for packaging spinalcordtoolbox datasets in pip. #1
base: trunk
Are you sure you want to change the base?
Template for packaging spinalcordtoolbox datasets in pip. #1
Changes from 1 commit
ec0f257
cbbac73
dbcd1e3
7173954
24ba7f2
68fb7ff
305fb5e
2b0291a
f4d8f1e
b828e86
2521744
f4beed7
97683bc
cc38cc6
f45045b
8dde7fd
4410d0e
4fe54cc
7bbc02d
3b2c4ef
b9e31cb
0f1e9fa
c521774
19f2765
e74cf6a
4a09c4d
2afa7c3
eaea4c2
525ea88
fe246fc
65021c2
dfb1e5c
0053d78
73f9b33
9d6f082
7394563
804b510
7ed3fe3
3f67509
2fb6da0
12a97d2
ffeb997
07ca856
eb91ae0
a383f68
1322a88
6aae1ca
3746bea
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Which TODO is this referring to?
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.
There's another terrible python wart I ran into when setting this up that I don't know how or where to explain.
The basic issue is,
sys.path
behaves like$PATH
or likeoverlayfs
: it implies a virtual filesystem made of the union of all the folders listed in it. But python only pretends to support this and needs lots of workarounds for it. Recently, https://www.python.org/dev/peps/pep-0420/ dropped the necessity of__init__.py
to create a package, in order to make it easier for this virtual union filesystem thing to work out; otherwise, every package that did what I did here, share a folder (spinalcordtoolbox/data
), would be have to ship a__init__.py
and would therefore be fighting with each other. So, A+ to that; but for some reason in the process they never applied the same thinking to non-python files. There's even this comment: https://github.com/python/cpython/blob/30e507dff465a31901d87df791a2bac40dc88530/Lib/importlib/resources.py#L243-L247So what this means is this terrible confusing situation where:
pip install
everything, everything ends up in the same physical directories and things make sense__init__.py
everywhere like you used to before PEP420, and just be careful not to create any conflicts, and then everything would work finepip install -e
one or more of the packages, then you're in this virtual situation where the filesystem you see viaimport
andimportlib
is not the same as on disk but you can access python code but you can't access the others.__init__.py
s in this situation, you can't access files from the other appsspinalcordtoolbox/__init__.py
shadows the entirespinalcordtoolbox/data/
subdirectory, so none of these packages will work until we remove that, but only if you'vepip install -e spinalcordtoolbox
'd. It will work if youpip install spinalcordtoolbox
.BUT they literally just fixed this a month ago: python/cpython#24670. Praise the python maintainers. So maybe that link is what can go in the TODO.
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 tiny TODO took me like 4 hours to figure out. It definitely is where the bulk of my time working on this went and I don't know how to explain what I was doing because it's such a corner case.
And it has implications for the rest of sct. It means we need to delete all the init.pys from it. I checked and mostly they're empty anyway, but
aren't totally.
But (until python3.10 I guess, when we finally get python/cpython#24670) we'll have to keep these empty
__init__.py
s in the data folders but not in the code folders. Seems pretty fragile.Or it looks like https://github.com/python/importlib_resources/ is still being maintained as a backport; I thought it was just for python3.6 but maybe it even covers newer versions?
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 this true? PEP420 only applies to namespace packages.
Namespace packages are defined as packages that are split up into multiple distributions. I don't think this describes the subpackages inside
spinalcordtoolbox
, does it? (Since we distribute everything as a single package, we're more like the first directory tree from that link, rather than the second.)So, I think we should be able to leave the
__init__.py
files alone.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 almost want to throw out
importlib.resources
entirely and just go back to using__file__
. If we put:in every
__init__.py
we would have the same results for both. It would mean we wouldn't be able to use virtual namespaces, so we couldn't make a multi-part
spinalcordtoolbox-data-something
package like python3.10 apparently can, but why would we want to?...maybe we would want to do that to deal with splitting up data to fit online? Like a multi-part .rar or a replacement for
ghsplit
?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 does feel a lot more readable. I was worried about
importlib
not being very self-evident. I also like how this can be handled on the template level.Looking at the current usage, though (e.g.
sct_deepseg
with its use offolder
) I feel like exposing the base directory might line up more with how it parses for files.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.
Okay actually I think this provides a solution:
importlib_resources
, the backportimportlib.resources
So we can just:
and
and delete all
__init__.py
s and tell people to stop making new ones.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 still kind of like just using
__file__
. Butresources
is the Way Of The Future so maybe we should go with that.