Transition to versioned bids functions

[pvandyken]

TODOS

• test bidspec mocking docstring_formatter as unavailable to ensure docstring still added
• exclude if TYPE_CHECKING: from codecov

Another large PR! The task ended up being more finicky than expected, but I'll try to justify some of my decisions below:

Version functions using configuration, rather than import

The versioned imports (e.g. import bids_v0_0_0 as bids) would have worked well in our snakemake context, where we typically directly import the bids function. However, it doesn't scale well to any other context. There's three particular problems to point out:

1. If bids() is only imported once, as in snakemake, all is well, but in a standard python project, where bids must be repeatedly imported, now you have to ensure you import the same version in every file
2. I'm hoping one day to have bids-like methods directly on BidsComponent that will derive new paths based on the old component. The versioned imports wouldn't help configure these methods.
3. The long-term goal is to move path building methods from snakebids into rsbids. rsbids will have a much more robust configuration system, and the versioned imports wouldn't fit well in it.

So I decided instead to make the bids spec globally configurable via snakebids.set_bids_spec. The function takes a spec object (importable via snakebids.paths.specs.vx_x_x()), or the string name of one of the official specs (e.g. "vx_x_x"). Obviously, this introduces non-pure, state-dependent behaviour for bids(), but I think this type of thing is very typical for configuration in python libraries (e.g. numpy, matplotlib, etc all use similar global config objects).

Dynamic generation of python code and interface files

In an effort to reduce the impressive levels of code duplication, especially docstrings, that would have been required for the versioned bids functions, I used a lot of dynamic importing in the path module. Basically, you can define __getattr__ directly in a module, and that will be used for dynamic imports. Then, to avoid reading a bunch of specs that would never get used, I transitioned the entire project to lazy imports (made possible by a custom fork of mkinit that is awaiting a PR). Finally, to keep type checking working, I set up a script to automatically generate *.pyi files for the dynamically generated versioned functions.

And then... I decided to switch over to the configurable system. So now the above paragraph only pertains to the spec reading functions (in snakebids.paths.specs), which are still dynamically generated and versioned. So I'm keeping around all the code generation stuff, but if it seems like overkill, that's why.

Documentation updates

I did some reorganization of the documentation, converting the very flat structure into a more explicit "User guide" and "API" sections. The main api page has been subdivided into different subpages based on category. Unfortunately, the API is complex enough that I could no longer sanely rely on autogen to make the pages, so most of the main functions are explicitly list in the documentation source (although the "Internal" API page is mostly automatic). So something to keep in mind for future maintenance

Blanket deprecation of include_subject_dir and include_session_dir

My original idea was to keep these arguments only for the v0_0_0 spec and not have them for all future specs. But the logic to maintain that exception was getting too complex, so I decided to still keep those arguments for every spec, but deprecate them, with plans for removal in 1.0. I don't know that many people use these arguments anyway (maybe in some of @akhanf's workflows?)

[akhanf]

Nice work! Haven't gone through what you've done yet, but looks really cool at a glance!

re: the deprecation of include_subject_dir and include_session_dir that sounds fine, I use it rarely enough that I am good to either keep those uses pinned to an early snakebids version, or adapt the code moving forward.. By the way, what is the preferred way to use e.g. the subject entity in a filename only and not in the folder?

[pvandyken]

At this point a custom spec:

bids_factory(specs.v0_0_0(sub_dir=False))(subject="001")

If you need such paths frequently, otherwise use Path(...).name.

Is that sufficient? We can consider a more privileged API if it's sufficiently useful, but I'm hoping the above methods are enough.

[pvandyken]

One other thing to discuss before this is ready, I still want the bids function to return a Path object rather than a string (if/when this gets ported over to rsbids, it would return a BidsPath). I know I tried this before and broke some workflows (probably because there's workflows doing + operations?). So one, do people think this is a worthwhile goal to begin with, and two, what would be an acceptable transition strategy? If we go with this, the two likely options would be 1) keep a permanent exception for the v0_0_0 spec, and 2) keep a str return until 1.0, and then hard switch everything. 1) is more permanent, 2) is much easier to implement

@kaitj, @akhanf

[kaitj]

Wow - a lot of great work done here! Will wait for the "ready for review" before going through it.

To your discussion / questions:

1. I personally agree with returning a Path object, based on my own use. That said, I think this might be a discussion that is worth calling a dev meeting for to get some more opinions of others who are working with Snakebids.
2. I think if we are make the switch, trying to keep a permanent exception might be a softer transition. The hard switch I think may cause workflows to break unexpectedly when the Snakebids upgrade ultimately done as you noted from your previous attempts.

[pvandyken]

I think if we are make the switch, trying to keep a permanent exception might be a softer transition. The hard switch I think may cause workflows to break unexpectedly when the Snakebids upgrade ultimately done as you noted from your previous attempts.

Yeah, so with a hard switch, it definitely wouldn't be until 1.0. Basically any workflows that break with the change would have to pin their snakebids version until they can be fixed.

One other advantage of a soft switch is that we wouldn't have to wait until 1.0 to get path returned. But it would make the typing difficult... I can't really type a function based on a configuration, so either we'd have to say it returns Path | str, which isn't super helpful (and not true anyway), or lie and say just Path. So that's not great either.

One other option: if/when this functionality gets ported to rsbids, I plan on renaming the function build. My intent would be to re-export it into snakebids as bids to preserve compatibility, but another option is to start introducing the name change now, so build would return a Path and bids would return a str, but otherwise be the same. To be honest I'm not a huge fan of this, as we have two nearly identical functions whose difference is not clearly indicated by the names, but it is an option.

[kaitj]

Hmm, yeah, I guess I wasn't thinking about the type hinting too much there. The more I think about it, I think the hard switch is fine as long as we give ample warning (i.e. have a warning message now or whenever we decide that this is going to change come 1.0).

I agree about the redundancy - I'd rather choose one path and go down that way.

[codecov-commenter]

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (19215f8) 90.26% compared to head (c5ae653) 90.73%.
Report is 7 commits behind head on main.

❗ Current head c5ae653 differs from pull request most recent head ea55e1a. Consider uploading reports for the commit ea55e1a to get more accurate results

Files	Patch %	Lines
snakebids/paths/_utils.py	91.42%	3 Missing ⚠️
snakebids/utils/containers.py	97.84%	2 Missing ⚠️
snakebids/paths/_config.py	96.15%	1 Missing ⚠️
snakebids/paths/_templates/spec_func.py	97.77%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #349      +/-   ##
==========================================
+ Coverage   90.26%   90.73%   +0.47%     
==========================================
  Files          32       38       +6     
  Lines        1489     1652     +163     
==========================================
+ Hits         1344     1499     +155     
- Misses        145      153       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kaitj]

This largely looks good to me, but given its a fairly large PR, I'll take a second look through to make sure I didn't miss anything before approving.

[pvandyken]

@kaitj, I expanded on the discussion of bids specs in the documentation. It's definitely not perfect: I don't discuss custom specs, and it's really too much prose now to be in the API section. I'd like to leave addressing of these imperfections for another time though, as mentioned above, the docs will likely be evolving quite a bit moving forward anyway.

[pvandyken]

Okay, there's two live bugs in the main branch, the fixes for which are split between this PR and #365. So I'm going to push one of them in with failing checks so that I don't have to make the fixes twice.