Turn it into a proper Python package #611

hoechenberger · 2022-09-23T09:51:23Z

Fixes #580

Very much WIP at this stage, just trying to get the ball rolling.

cc @larsoner

Before merging …

Changelog has been updated (docs/source/changes.md)

Fixes mne-tools#580

hoechenberger · 2022-09-23T17:18:24Z

@larsoner Now it actually installs and creates an entry-point:

❯ mne-bids-pipeline
ERROR: Missing required flags: {'config'}
Usage: mne-bids-pipeline <flags>
  optional flags:        --config
  required flags:        --steps | --root_dir | --subject | --session |
                         --task | --run | --interactive | --n_jobs | --debug |
                         --cache
  additional flags are accepted

For detailed information on this command, run:
  mne-bids-pipeline -- --help

larsoner · 2022-09-23T18:11:50Z

Nice! Since all the mne commands are underscored like mne browse_raw here it probably makes sense for consistency to use mne_bids_pipeline ...

hoechenberger · 2022-09-23T18:40:22Z

Nice! Since all the mne commands are underscored like mne browse_raw here it probably makes sense for consistency to use mne_bids_pipeline ...

mne_bids uses underscores too, so I suppose you're right. Although I have to admit I do not like the inconsistency between package name and entry point name! It's confusing … and the underscores are not very UNIX-y 🤔

hoechenberger · 2022-09-24T11:33:37Z

@larsoner Using MNE's run_subprocess introduces an additional newline / empty line after each line printed by the logger here. Do you know how to fix this?

larsoner · 2022-09-24T11:35:01Z

Why do you need run_subprocess?

hoechenberger · 2022-09-26T17:06:23Z

Why do you need run_subprocess?

I only use it in run_tests.py to ensure all tests are run through the CLI!

.gitignore

hoechenberger · 2022-10-01T17:15:26Z

Re logging, I've opened a bug upstream: mne-tools/mne-python#11218

hoechenberger · 2022-10-03T17:56:14Z

@larsoner @agramfort
This should be good to merge once CI comes back green

The changes here allow installation of the pipeline via pip install . (editable mode supported as well), and python run.py ... can simply be replaced with mne_bids_pipeline ... but also remains available!

In followup PRs, we can add features like "please create a template config file" etc.

Once we have done that, I believe it's time for a first release!

agramfort

@hoechenberger thanks

can you see if the doc needs any update?

agramfort · 2022-10-03T19:38:47Z

run.py

@@ -316,5 +316,9 @@ def process(
        logger.info('Done running 👆', extra=extra)


-if __name__ == '__main__':
+def main():
    fire.Fire(process)


run.py is still useful?

Yep, it was one of my goals here to get everything working with as few changes as possible!

But see #611 (comment)

larsoner

A remarkably small diff actually!

I would prefer that we change the folder structure to reflect the standard scientific Python folder layout, i.e., nest all Python under a mne_bids_pipeline/* rather than at the repo root. The root would then just have docs, mne_bids_pipeline, and pyproject.toml.

larsoner · 2022-10-03T19:37:36Z

.github/workflows/run-tests.yml

@@ -30,7 +30,7 @@ jobs:
    - name: Lint config.py with flake8
      run: make flake-config
    - name: Install codespell
-      run: pip install codespell
+      run: pip install "codespell @ https://github.com/Freed-Wu/codespell/archive/refs/heads/iss2055.zip" tomli  # fork that supports pyproject.toml


-1 on this, let's push for codespell-project/codespell#2055 instead

... I see why you did this, so you could get rid of setup.cfg entirely. Let's keep it then and I'll fix this once the upstream PR is merged

larsoner · 2022-10-03T19:40:05Z

BUILDING.md

+* Create `sdist` distribution:
+
+  ```shell
+  python -m build --sdist


you should always also create a wheel so that --only-binary ":all:" works

I usually never build wheels, thanks for the pointer

hoechenberger · 2022-10-03T20:06:10Z

I would prefer that we change the folder structure to reflect the standard scientific Python folder layout

I actually actively wanted to avoid this until now, to cause as little disruption as possible to long-term users who expect to use run.py! But we can make the change if you think it'd make sense

hoechenberger · 2022-10-03T20:09:18Z

@larsoner I will probably be busy for the next few days, please feel free to take over and coordinate with @agramfort if you want

Otherwise you'll just to have to wait for me :)

larsoner · 2022-10-03T20:13:06Z

I think a lot of the internal mechanics might be simplified if we can make everything private by moving run.py to mne_bids_pipeline/_run.py etc. I've seen quite a bit of redundant code and gymnastics in all the automation routines that could be simplified by this. Just a few examples:

No need for sys.path insertion because we can use proper package-relative imports like from ._scripts import ... rather than absolute from scripts import ... which failed before (for me at least)
Code dedup between config.py and run.py, e.g., related to logging and imports
Moving all helper functions out of config.py to a new _utils.py to make the config.py "pure" in the sense that it can contain just all the default config values -- so we can both use it ourselves, and copy-paste it to give people a default config (probably while commenting out all lines like in standard Unix config files)
Simplification of doc building gymnastics due to interactions between variables (e.g., MAINT: Autogenerate step table #619 would be easier I think)

If we support python run.py ... for people rather than making them transition to mne_bids_pipeline ... we have to wait on this stuff :(

larsoner · 2022-10-03T20:28:41Z

Okay I'll leave the refactor to a follow-up PR since it's going to be a lot of git mv that would be nice to keep separate from this package-related stuff. I'll just push a quick fix for my comments above and then mark for merge!

hoechenberger · 2022-10-03T20:37:13Z

I think a lot of the internal mechanics might be simplified if we can make everything private by moving run.py to mne_bids_pipeline/_run.py etc.

Yep! That code will become much easier to maintain

hoechenberger · 2022-10-03T20:37:36Z

Okay I'll leave the refactor to a follow-up PR since it's going to be a lot of git mv that would be nice to keep separate from this package-related stuff. I'll just push a quick fix for my comments above and then mark for merge!

Sounds good to me!

larsoner · 2022-10-03T20:38:30Z

Thanks @hoechenberger !

* upstream/main: Turn it into a proper Python package (mne-tools#611)

agramfort · 2022-10-11T07:08:47Z

isn't it just asking people to use mne_bids_pipeline instead of run.py ?

…

Message ID: ***@***.***>

hoechenberger · 2022-10-11T07:44:05Z

@agramfort I don't know the context of your response :) do you mean how we would need to update the docs? We already did do that!

agramfort · 2022-10-11T08:02:03Z

Sorry this email was sent with a delay from my iPad …

hoechenberger added 6 commits September 23, 2022 11:48

Turn it into a proper Python package

4bc750b

Fixes mne-tools#580

Use codespell fork with pyproject.toml support

89b73e4

Specify test requirements in pyproject.toml too

0c7670c

Amend .gitignore

e509d8c

Add entry-poiny

dd1dc54

Fix install

d240e4e

hoechenberger added 3 commits September 23, 2022 20:43

Use underscores as separators in entry point script name

e0092c4

Use dev version of MNE, MNE-BIDS, autoreject

82b32cf

Add version number

74463b0

hoechenberger force-pushed the hoechenberger/issue580 branch from 95bee84 to 74463b0 Compare September 23, 2022 19:14

hoechenberger added 2 commits September 23, 2022 22:13

typing_extensions only needed for Python < 3.8

b460a0f

Install tomli too (for codespell fork)

a78d5e3

hoechenberger force-pushed the hoechenberger/issue580 branch from cb6d16a to a78d5e3 Compare September 23, 2022 20:21

Include FS BIDS app version file

4a32380

hoechenberger force-pushed the hoechenberger/issue580 branch from 51f996e to 4a32380 Compare September 23, 2022 20:43

This comment was marked as resolved.

Sign in to view

Run tests through CLI

bdfbc2d

hoechenberger added 2 commits September 29, 2022 16:05

Merge branch 'main' into hoechenberger/issue580

3dfa9d1

Merge branch 'main' into hoechenberger/issue580

2b2e66d

hoechenberger commented Oct 1, 2022

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

hoechenberger added 3 commits October 1, 2022 08:47

Update .gitignore

e6a3c1d

Merge branch 'main' into hoechenberger/issue580

97c9be7

Fix Config doc

25804b2

hoechenberger marked this pull request as ready for review October 3, 2022 17:54

hoechenberger closed this Oct 3, 2022

hoechenberger reopened this Oct 3, 2022

agramfort reviewed Oct 3, 2022

View reviewed changes

larsoner reviewed Oct 3, 2022

View reviewed changes

larsoner mentioned this pull request Oct 3, 2022

MAINT: Refactor project code to mne_bids_pipeline/*.py #620

Closed

DOC: Document [ci skip]

eeda0cb

larsoner merged commit e241d48 into mne-tools:main Oct 3, 2022

larsoner added a commit to larsoner/mne-bids-pipeline that referenced this pull request Oct 3, 2022

Merge remote-tracking branch 'upstream/main' into reorg

77a7d0d

* upstream/main: Turn it into a proper Python package (mne-tools#611)

hoechenberger deleted the hoechenberger/issue580 branch October 4, 2022 05:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn it into a proper Python package #611

Turn it into a proper Python package #611

hoechenberger commented Sep 23, 2022 •

edited

Loading

hoechenberger commented Sep 23, 2022

larsoner commented Sep 23, 2022

hoechenberger commented Sep 23, 2022 •

edited

Loading

This comment was marked as resolved.

hoechenberger commented Sep 24, 2022 •

edited

Loading

larsoner commented Sep 24, 2022

hoechenberger commented Sep 26, 2022

hoechenberger commented Oct 1, 2022

hoechenberger commented Oct 3, 2022 •

edited

Loading

agramfort left a comment

agramfort Oct 3, 2022

hoechenberger Oct 3, 2022 •

edited

Loading

larsoner left a comment

larsoner Oct 3, 2022

larsoner Oct 3, 2022

larsoner Oct 3, 2022

hoechenberger Oct 3, 2022

hoechenberger commented Oct 3, 2022

hoechenberger commented Oct 3, 2022 •

edited

Loading

larsoner commented Oct 3, 2022

larsoner commented Oct 3, 2022

hoechenberger commented Oct 3, 2022

hoechenberger commented Oct 3, 2022

larsoner commented Oct 3, 2022

agramfort commented Oct 11, 2022 via email

hoechenberger commented Oct 11, 2022

agramfort commented Oct 11, 2022

Turn it into a proper Python package #611

Turn it into a proper Python package #611

Conversation

hoechenberger commented Sep 23, 2022 • edited Loading

Before merging …

hoechenberger commented Sep 23, 2022

larsoner commented Sep 23, 2022

hoechenberger commented Sep 23, 2022 • edited Loading

This comment was marked as resolved.

hoechenberger commented Sep 24, 2022 • edited Loading

larsoner commented Sep 24, 2022

hoechenberger commented Sep 26, 2022

hoechenberger commented Oct 1, 2022

hoechenberger commented Oct 3, 2022 • edited Loading

agramfort left a comment

Choose a reason for hiding this comment

agramfort Oct 3, 2022

Choose a reason for hiding this comment

hoechenberger Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

larsoner left a comment

Choose a reason for hiding this comment

larsoner Oct 3, 2022

Choose a reason for hiding this comment

larsoner Oct 3, 2022

Choose a reason for hiding this comment

larsoner Oct 3, 2022

Choose a reason for hiding this comment

hoechenberger Oct 3, 2022

Choose a reason for hiding this comment

hoechenberger commented Oct 3, 2022

hoechenberger commented Oct 3, 2022 • edited Loading

larsoner commented Oct 3, 2022

larsoner commented Oct 3, 2022

hoechenberger commented Oct 3, 2022

hoechenberger commented Oct 3, 2022

larsoner commented Oct 3, 2022

agramfort commented Oct 11, 2022 via email

hoechenberger commented Oct 11, 2022

agramfort commented Oct 11, 2022

hoechenberger commented Sep 23, 2022 •

edited

Loading

hoechenberger commented Sep 23, 2022 •

edited

Loading

hoechenberger commented Sep 24, 2022 •

edited

Loading

hoechenberger commented Oct 3, 2022 •

edited

Loading

hoechenberger Oct 3, 2022 •

edited

Loading

hoechenberger commented Oct 3, 2022 •

edited

Loading