search

Donders-Institute / bidscoin

BIDScoin converts your source-level neuroimaging data to BIDS
https://bidscoin.readthedocs.io
GNU General Public License v3.0
130 stars 36 forks source link

First working version to implement argparse-manpage #192

Closed marcelzwiers closed 1 year ago

marcelzwiers commented 1 year ago

I think I cannot invite @musicinmybrain here for review, but when you are up to it, can you have a look and see if this works for you? Any feedback is more than welcome :-)

marcelzwiers commented 1 year ago

I added a simple test (1ab2168466e1c1144d1398e5144d9f98860271cc) for the generated manpages, but I struggle to find the right path to the manpage files (I added a stub, hence the failed tests). Perhaps you know how to set this correctly? And if you'd like to add more upstream manpage tests of your own (checking whatever Fedora likes to have), this is the place to put them :-)

marcelzwiers commented 1 year ago

Ok, I patched it using manpage = Path(sys.executable).parents[1]/'share'/'man'/'man1'/f"{executable}.1", but I have no idea if that works robustly enough

marcelzwiers commented 1 year ago

Mhhh, my patch only works on Linux...

musicinmybrain commented 1 year ago

Ok, I patched it using manpage = Path(sys.executable).parents[1]/'share'/'man'/'man1'/f"{executable}.1", but I have no idea if that works robustly enough

The share/man/man1 part is hard-coded:

https://github.com/praiskup/argparse-manpage/blob/2856951732a850c8c7767146919b3d8bb1e3015c/build_manpages/build_manpages.py#L176

The prefix self.install_data seems to come from setuptools. I had a hard time finding documentation or relevant source code for how this is constructed. It looks like you’ve arrived at something that works in practice.

musicinmybrain commented 1 year ago

On Fedora Linux, I started testing by checking out the current master (https://github.com/Donders-Institute/bidscoin/commit/c73539b0bcf0b4199060b15c2486e45301f0cc0d) and running tox:

So I tried adding dcm2niix to the deps in tox.ini. This fixed the test_plugin failures, and test_bidscoiner now failed with:

E       assert 'ERROR' not in '2023-07-08 ...ruption(s)\n'
E         'ERROR' is contained here:
E           0:16:53 - ERROR | Failed to run:
E         ?           +++++
E           dcm2niix -b y -z y -i n -l n -f "sub-Archibald_ses-02CTHEADBRAINWOCONTRAST_acq-RoutineBrain_run-1_ct" -o "/tmp/pytest-of-ben/pytest-6/bids_dicomdir0/sub-Archibald/ses-02CTHEADBRAINWOCONTRAST/anat" "/tmp/pytest-of-ben/pytest-6/raw_dicomdir0/Doe^Archibald/02-CT, HEADBRAIN WO CONTRAST/002-Routine Brain"
E           Errorcode 0:
E           Chris Rorden's dcm2niiX version v1.0.20230411  (JP2:OpenJPEG) (JP-LS:CharLS) GCC8.4.0 x86-64 (64-bit Linux)
E           Found 4 DICOM file(s)...
E         
E         ...Full output truncated (22 lines hidden), use '-vv' to show

tests/test_bidscoiner.py:27: AssertionError

Switching to the build_manpages branch, I repeated the exercise. This time (with dcm2niix added to deps in tox.ini) tox passed in Python 3.8 through 3.10: tests_bidcoiner did not fail. (The dependency problem in Python 3.11 remains.) So this PR looks OK from that perspective.

When I do

python3.11 -m venv _e
. _e/bin/activate
pip install -e .

I get the following man pages in ./man/:

bidscoin.1
bidscoiner.1
bidseditor.1
bidsmapper.1
bidsparticipants.1
deface.1
dicomsort.1
echocombine.1
medeface.1
physio2tsv.1
plotphysio.1
rawmapper.1
skullstrip.1
slicereport.1

I spot-checked a couple with e.g. man ./man1/deface.1, and they looked legible.

I tried again, installing the package as a regular wheel instead of an editable one:

git clean -fxd
python3.11 -m venv _f
. _f/bin/activate
pip install .

This time the man pages were installed in _f/share/man/man1/, and commands like man bidscoin worked.

Overall, I would say this passes basic “smoke tests” on Fedora Linux. I haven’t done any of the following:

That’s all the time I expect to spend on this today, though. Hope it helped.

marcelzwiers commented 1 year ago

On Fedora Linux, I started testing by checking out the current master (c73539b) and running tox:

* On Python 3.11, I got `ERROR: No matching distribution found for pypet2bids>=1.0.12`.

That's not very important, because pypet2bids is part of the PET plugin, which is still a bit experimental / under development (it's probably something trivial that it hasn't been tested for v3.11)

tests/test_bidscoiner.py:27: AssertionError

That seems like a dcm2niix issue, not a BIDScoin issue...?

Switching to the build_manpages branch, I repeated the exercise. This time (with dcm2niix added to deps in tox.ini) tox passed in Python 3.8 through 3.10: tests_bidcoiner did not fail. (The dependency problem in Python 3.11 remains.) So this PR looks OK from that perspective. [..] This time the man pages were installed in _f/share/man/man1/, and commands like man bidscoin worked.

Looks like my refactoring/clean-up for this new branch worked out well :-). I'll go ahead and merge it to master, further development/PR's can be merged from there

Overall, I would say this passes basic “smoke tests” on Fedora Linux. I haven’t done any of the following:

* written a spec file to install this as an RPM package 
* tested on other platforms (I have access to MacOS, but didn’t want to mess with PyQt5)
* reviewed the source diff for this PR

Just let me know if you run into issues or have any news

That’s all the time I expect to spend on this today, though. Hope it helped.

Can't blame you, thanks and enjoy your weekend :-)

marcelzwiers commented 1 year ago

What's roughly speaking the time frame you have in mind for shipping BIDScoin? I think it is currently in pretty good shape, so I could brew a release for you when needed. (p.s. I am away from July 20 - August 10, so no release in that period)

musicinmybrain commented 1 year ago

What's roughly speaking the time frame you have in mind for shipping BIDScoin? I think it is currently in pretty good shape, so I could brew a release for you when needed. (p.s. I am away from July 20 - August 10, so no release in that period)

It’s hard to say. It depends on my free time for packaging it (including packaging some of the most desirable optional dependencies, and their dependencies), and on how much time other Fedora contributors have to do new-package reviews. I’ve taken it on as a low-priority project: I do intend to see it through, but I’m not in a big hurry. I would be perfectly happy to spend the next month lining up dependencies.

I also have the flexibility to ship (compatible) updates after the initial package, backport selected changes as patches if needed, package a snapshot if there’s a really good reason to do so, etc., so a new release before the initial package is nice to have, but not strictly required.