Skip to content
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

Python cmake binwrappers: Split all commands into multiple files #2850

Merged
merged 6 commits into from
Mar 24, 2024

Conversation

Lestropie
Copy link
Member

This is a proposed alternative structure for #2741. I wanted to generate it and then lay it side by side against the current proposal there.
(Browsing the code on this branch might communicate such better than the PR)

Admittedly there's some stuff in here that I have done along the way that is not totally specific to this proposal; that is, even if the core of this proposal is rejected, some of the content here would still be applicable to whatever alternative were to be chosen.

So the main points here:

  1. Both source files specific to an individual command, and the API modules, reside in python/mrtrix3.

    1. They do not need to reside in something called lib/ in the source tree, since they shouldn't be getting included from there directly.
    2. I do however keep a "mrtrix3" subdirectory since that will likely give IDEs a bit of assistance.
  2. All commands are stored in a sub-directory of python/mrtrix3, regardless of whether or not they possess multiple underlying algorithms.
    On Python: cmake-generated wrappers for executables #2741, some commands are standalone .py files, others contain their own subdirectories. This is a bit clumsy both when navigating, and in the cmake code, as it changes the content that needs to be written to the executable file.

  3. The sub-directories for the various commands reside within python/mrtrix3, alongside the utility module files.
    Python: cmake-generated wrappers for executables #2741 currently has these grouped in a sub-directory "scripts/"; perhaps if an unnecessary subdirectory can be avoided it might be preferable.

  4. Every command has had the code corresponding to the requisite usage() and execute() functions split between two separate files. Some commands also have additional code separated into their own files.

    1. Having so many occurrences of "usage.py" and "execute.py" is quite strange, particularly when dealing with maintenance on the whole code base; but if just working on one command at a time, it probably doesn't matter.
      If anything is going to result in this proposal getting rejected, this is probably it.
    2. This would both facilitate breaking up some currently unwieldy large files, and encourage better Pythonic modularisation of future additions.
    3. Some commands have separate files corresponding to tiny-to-empty functions for dealing with inputs and outputs. With Python CLI changes #2678 that handling changes, such that those functions (and consequently files) will disappear.

(PS. I'm aware that merging this against #2678 will be a nightmare... but I'll deal with it)

Lestropie and others added 4 commits March 6, 2024 19:04
- Do not split between "lib" and "src": All Python content goes into lib/mrtrix3/.
- Rather than having some executable scripts living as standalone .py files and others as sub-directories (to deal with separate algorithm files), place all files that are not part of the API into sub-directory trees. For now, all files have been renamed to __init__.py.
- For each script (or algorithm thereof), split code across at least usage.py and execute.py files.
- Remove Python scripts that are not based on the Python API.
- Remove mrtrix3.algorithm module. Its operation was incommensurate with the transition to cmake, where dependencies are expected to be known prior to execution, as it scanned the filesystem at execution time for the sake of discovering any newly added algorithms. Most of the prior functionality is replaced with overloading function app.Parser.add_subparsers().
- Greatly simplify the content of cmake-generated executables for Python commands.
@Lestropie Lestropie self-assigned this Mar 7, 2024
@Lestropie Lestropie changed the title Python cmake binwrappers splitall Python cmake binwrappers: Split all commands into multiple files Mar 8, 2024
@Lestropie Lestropie marked this pull request as ready for review March 24, 2024 00:10
@Lestropie Lestropie merged commit 53a4fe2 into cmake_python_binwrappers Mar 24, 2024
@Lestropie Lestropie deleted the python_cmake_binwrappers_splitall branch March 24, 2024 00:12
@Lestropie Lestropie restored the python_cmake_binwrappers_splitall branch April 24, 2024 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants