cmake Python changes #2920
cmake Python changes #2920
Conversation
- Re-arrange Python source files on the filesystem. API files reside in python/mrtrix3/ (still in a sub-directory called "mrtrix3/" so that IDEs can reasonably identify them, but no need to be in a "lib/" sub-directory in source form). All command source code centralised into python/mrtrix3/commands. - cmake will generate short executable Python files in the build bin/ directory, one for each command. These load the version-matched API and command code from a relative path. This mechanism deprecates file python/bin/mrtrix3.py, and command source files no longer include the "import mrtrix3; mrtrix3.execute()" code at their tail. - Change to the interface of the app._execute() function. Rather than a module, it now accepts as input the usage() and execute() functions. This should allow greater flexibility in how a developer arranges their command code. - Following from point above, there are now three different ways for a developer to arrange their command code on the filesystem: 1. A solitary .py file residing in python/mrtrix3/commands/, which defines both usage() and execute() functions within. 2. A sub-directory residing in python/mrtrix3/commands/, which contains a .py file with the same name as the sub-directory, within which contains the usage() and execute() functions. This is useful for those commands using the "algorithm" concept, as each algorithm can be defined as its own .py file within that sub-directory; but all source code relating to that command is grouped within that directory. 3. A sub-directory residing in python/mrtrix3/commands/, which contains at least two files called usage.py and execute.py, which provide the usage() and execute() function implementations respectively. This will be useful for those scripts where the volume of code is too much for a single source code file. - mrtrix3.algorithm module deprecated. Some functionality is no longer required, and algorithm modules are now loaded directly using importlib instead. List of algorithms available for relevant commands is now explicitly coded in the corresponding __init__.py files. - No longer have to rename source code relating to the 5ttgen command to "_5ttgen"; all handling of module names should now be permissive of leading digits. - cmake can either copy or softlink Python source code files, toggled via envvar "MRTRIX_PYTHON_SOFTLINK". - Deprecate commands blend, convert_bruker, gen_scheme, notfound; commands not utilising the MRtrix3 Python API are not well managed by the new build system, and still do not appear in the online documentation, so will need to be either ported or provided externally. - Files python/mrtrix3/version.py.in and python/mrtrix3/commands/__init__.py.in generate build directory files lib/mrtrix3/version.py and lib/mrtrix3/commands/__init__.py, filled with relevant build-time information. However files python/mrtrix3/version.py and python/mrtrix3/commands/__init__.py are still defined in the source tree, with the relevant variables defined, only that they are void of information. This allows run_pylint to run without even configuring cmake, and should prevent IDEs from producing undefined symbol warnings.
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
CMAKE_SOURCE_DIR refers to the top level source tree. Hence, if mrtrix3 is built as a subproject of another project (e.g. using add_subdirectory), things won't work as expected.
|
Looks mostly fine to me at a first glance.
I don't quite the reasoning behind this. How does this help IDEs? |
|
clang-tidy review says "All clean, LGTM! 👍" |
Not actually tested, but I assume that if an IDE sees |
|
I see, that makes sense. Just tested this on VS Code and it behaves as you predicted. |
daljit46
force-pushed
the
cmake_python_changes
branch
2 times, most recently
from
cfe7d0c to
648468f
Compare
|
I think it would make sense for |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
I'm a little unsure on how to handle Furthermore, currently we create the testing tools in |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
RE
RE the build path for testing tools, personally I find it a little unusual to look inside of the |
This directs IDEs like Qt Creator to show the Python code as project files.
- Instead of specifying a BUILDDIR, the caller must specify an OUTPUT_DIR directory where the output file will be written. - The script now additionally accepts a list of directories that will be prepended to the system path in the generated script. This shouldn't be necessary for most commands.
- Executables for Python testing tools which are essentially MRtrix3 Python commands (e.g. they are defined by a usage and run function), are generated using the MakePythonExecutable script. - Additionally, some renaming of variables has been performed for consistency.
daljit46
force-pushed
the
cmake_python_changes
branch
from
fe31fc9 to
1f1fb1d
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
daljit46
force-pushed
the
cmake_python_changes
branch
from
4f55a08 to
922ddbf
Compare
|
I have made some changes to fix the running of unit tests. |
|
clang-tidy review says "All clean, LGTM! 👍" |
daljit46
force-pushed
the
cmake_python_changes
branch
from
922ddbf to
cee8d41
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Conflicts: CMakeLists.txt
|
Yep, that solution for Python command documentation generation is failing on Linux CI: Once that's resolved I'm happy for this to be merged. Edit: Was |
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
- Fix importlib call in cmake-generated Python executables for stand-alone Python files; this regressed in cleanup commit 291a747, which worked for the Python CLI test (as both executable and source file resided in the same directory) but broke execution of other Python commands. - Change handling of Python CLI test: executable will be placed in ${PROJECT_BINARY_DIR}/bin; source code will be placed in ${PROJECT_BUILD_DIR}/lib/mrtrix3/commands/. - Update docs/generate_user_docs.sh to reflect new handling of Python commands. - Auto-update of some Python command help pages to propagate prior changes. - Add per-test labels to unit tests.
Lestropie
force-pushed
the
cmake_python_changes
branch
from
8822ce8 to
3fce648
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Spoke too soon; one of the changes you made to the cmake-generated Python executables to facilitate Full details in 3fce648, but what I chose to do is put See if you're happy with that. Assuming there isn't anything outstanding still broken, I'm happy to proceed with merging. |
Closes #2730.
My third attempt at Python refactoring following
cmaketransition (#2689), after #2737 and #2741. Done a decent amount of thinking about it after it was last spoken about during a dev team meeting. I'm happy with this one and want to proceed. Commands all seem to execute as expected, from both build and install directories. For existing Python commands, requisite change is minimal; just deletion of the "import mrtrix3; mrtrix3.execute()" at the tail of the file.Lots of details in commit message of 93f3741.
@daljit46: Handballing to you. You have carte blanche to change as you see fit; whether to resolve my
cmakenaivety, or anything in this proposal that would be completely prohibitive if trying to expand the concept to external project support. But in the absence of any major criticisms from yourself or any other @MRtrix3/mrtrix3-devs, hoping this gets me out of your way to proceed with #2901.