-
Notifications
You must be signed in to change notification settings - Fork 181
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-generated wrappers for executables #2741
Conversation
Change filesystem structure of python/. No need to preserve the lib/mrtrix3/ sub-directory structure in the repository; that can be constructed exclusively in the build directory. Executables now reside in sub-directory python/scripts/. For algorithm-based scripts, all code is placed in a sub-directory of python/scripts/, with the previous bin/ file contents now placed in __init__.py of that directory, thus co-locating all relevant source code for such commands. The algorithm module is simplified somewhat since the algorithm files are co-located with the interface source file. Delete file python/bin/mrtrix3.py, and script source files no longer load the mrtrix3 module and invoke the execute function at the end of the file; an alternative mechanism for loading the API and script entrypoint will be created subsequently to this commit.
Okay, extended this a little. Successfully have a configuration where the cmake build directory contains softlinks to the relevant source files, such that edits can be testing without touching cmake. When So overall this seems to tick a lot of boxes:
Potential downsides:
Other notes:
Would very much like feedback on this proposal. If we want to go for something else I need to know why & how. |
Here's what the structure looks like under this PR. ~/src/mrtrix3$ tree python/
~/src/mrtrix3$ tree build/lib
~/src/mrtrix3$ tree ~/bin/mrtrix3 # install location
|
This general idea might indeed end up being the cleanest solution we can come up with... A few quick questions:
That said, having looked at Python's own packaging page, there seem to be a million recommendations, all relying on different tooling and more or less suitable for different types of packages and environments. I think it's fair to say there's no good answer here... |
Probably. I just ripped #2735 to get the concept demonstration done.
I didn't completely follow the logic when you looked at this option in the meeting last week. I wanted to generate this proposal as it was on my mind but wasn't sure if it (or the differences to what you were thinking) were coming across. I need to figure out exactly what's being proposed there, and then do it and see if it's different to what you intended. From memory I did have some kind of concern with it at the time. I think it had to do with forcing creation of a Also would need clarification on exactly what you intend by "the site_packages approach". It could be that use of the shebang-to-softlink, or you could be proposing actually putting the python API inside the user's Python site-packages directory. |
On the I have to admit, I've not so far given a great deal of thought as to how we might end up dealing with external modules... Good to see you've got your eye on that! But like you said, I think this would work regardless of whether it's in As to how the
lib/mrtrix3/bin/dwi2response
lib/mrtrix3/lib/python3.11/site-packages/mrtrix3/dwi2response/init.pyContains everything that was in original
Having gone through that exercise, I'm not sure I like this approach after all, it feels like an even more convoluted hack than anything we've discussed so far... |
Having looked at various posts and suggestions online, it seems to me that the simplest solution really is just to add the path to the I had a go at mocking this up, it feels a lot more sane to me, and pretty close to what we have already. See what you think:
bin/dwi2responseThis could be trivially generated by
lib/mrtrix3/dwi2response/init.pyOriginal contents of
This one seems to work fully as expected out of the box... It doesn't solve the problem that the As to handling external modules, it's a trivial addition to add one more entry to |
Need to have a go at restarting & resolving this one. Reading latest comments from @jdtournier: I'm dissuaded by solutions that involve any content in the code repository residing in a directory called It is also in retrospect kind of strange to have such a wide separation between the location of eg. So I think what I'm leaning towards at the moment, at least as far as repository filesystem structure is concerned, is:
That would:
Potential downside is that file names in your editor won't intrinsically convey which particular command is currently being edited. I've noticed that at least VSCode is quite good in that it shows more of the file path if you have concurrently opened two files with the same base name. Please do chime in if you think I'm going down the wrong path. The topics of:
can I think be handled separately once the filesystem structure is decided. |
Could / should this be toggled with an environment variable at the |
Actually, revising the proposal above. Within the code repository, there is no need to have the Edit: Note that this was already the case with the current state of the code at time of writing (72ee94f). |
- 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.
Candidate further changes to this proposal are in #2850. I think my mind is converging on the solution in terms of:
What is still up in the air is the filesystem locations of command source code relative to API source code. This has to take into consideration the fact that currently, some commands are a standalone If we here assume a root of 1. Sub-directory to separate commands from API modules?
vs.
In the scenario where some commands appear as a single 2. Sub-directories for only algorithm-based commands, or all commands?Assuming just for this example that:
Note that you can't have a vs.
The former is a bit clumsy in 3. Split command code across multiple files?Assuming 1. A separate directory for command source code, and 2. sub-directories for all commands:
vs.
The latter:
It does however result in a lot of files called I note the possibility that for algorithm-based commands like If anyone has an opinion on any of these three separately, or has a preference for one specific combination, please do say so; otherwise I might need to just pull the trigger based on my own intuition / preference. For clarity, what I ended up generating in #2850 has: 1. No child directory separating commands from utilities, combined with 2. All commands get their own sub-directory, such that the distinction between utility modules and command source code is done based on files vs. directories. Then for 3. I did the separation of source code into
|
File missing from 02b0a7e.
Python cmake binwrappers: Split all commands into multiple files
Going to muse a little on the whole This echoes how things have been done in MRtrix3 C++ commands since before the Python API existed. Over and above being a logical modularisation of a command-line executable, in C++, separation of
In Python, something reasonably similar occurs:
It occurs to me that if supplanting Where this would however break is in those commands that provide multiple algorithms. Configuration of the subparser per algorithm would be best done using a function per algorithm that configures the command-line parser for just that algorithm, arguably within the module defined by that algorithm as is currently performed. |
Superseded by #2920. |
Experimental alternative to #2737 for addressing #2730.
Some comments in the initial commit, but I'll try to explain the wider concept here, especially given I don't have the capability with cmake to churn out a fully working proposal.
Here is what the source code looks like with this PR (reduced):
Note one change in particular:
5ttgen
anddwi2response
don't have.py
files inpython/scripts/
; instead, the corresponding code lies in eg.python/scripts/5ttgen/__init__.py
.Within the build directory, this will be arranged as follows:
(Note one minor change: need to use directory name "
_5ttgen
" as Python modules can't start with a number)Files within
bin/
corresponding to Python commands would be generated during the cmake build stage. Their contents would look something like (with relevant content substituted on a per-command basis):(Note: would need to deal with the fact that for different commands, the source code module might be at
src/mrtrix3/*.py
orsrc/mrtrix3/*/__init__.py
)Potential disadvantage of an approach like this is that it's no longer possible to have just a standalone Python file that can execute against the API as long as the API can be found; one would need to do whatever setup ends up being requisite for external projects.
Had planned to write up a pros & cons list, but have to hit the sack.
Open to thoughts.