Explicit MRtrix3 dependencies of Python commands - #3394
Open
Lestropie wants to merge 4 commits into
Open
Conversation
Previously, building any MRtrix3 Python command forced compilation of every C++ binary, because CMake had no knowledge of which commands a Python script might invoke. Each Python command now declares an MRTRIX_DEPENDENCIES constant listing the MRtrix3 commands it may call, and new CMake helpers parse and recursively resolve these declarations at configure time to attach a minimal set of build targets to each command, falling back to the full command set when a declaration is absent. A linter run within the secondary-checks CI action verifies that every command declares its dependencies and that statically-determinable run.command() invocations and image header accesses are covered. Session prompts: 1. > Currently, cmake is instructed that if at least one MRtrix3 Python command is to be built, then all MRtrix3 C++ binary commands must also be built, as cmake is not provided with information about which MRtrix3 commands an MRtrix3 Python script may invoke. The purpose of this session is to provide this information. > 1. Within each MRtrix3 Python command (python/mrtrix3/commands/*.py if single source file, python/mrtrix3/commands/*/__init__.py if source directory), a constant list-of-strings MRTRIX_DEPENDENCIES is to be defined, which lists the names of all MRtrix3 commands that may be invoked. For each existing command, perform a search through the source code for reference to MRtrix3 commands and write that constant. Anything in a comment can be ignored, but do not restrict search to invocations of run.command() as a command name may be embedded within a variable. > 2. Write a cmake function, to exist in a standalone file in the cmake/ directory, that takes as input a Python command name and yields a list of MRtrix3 commands that must also be built if that Python command is to be built. This is to be based on direct parsing of the relevant source code file, without invoking the Python interpreter. > 3. Write a cmake function, to exist in a standalone file in the cmake/ directory, that recursively builds the set of command compilation dependencies. A Python command may itself invoke another MRtrix3 Python command, in which circumstance it is necessary for all dependencies of that invoked Python command to also be included in the compilation target list. > 4. Modify the cmake identification of compile targets. No longer establish "all C++ binaries" as a compile target dependency for all Python commands (though leave that target defined). Instead for each Python command establish at configure time the set of corresponding build targets utilising the functions described in steps 2 and 3. > 5. If a specific Python command fails to define the MRTRIX_DEPENDENCIES constant, then this must be appropriately reported by the function defined in step 2, and the result of step 3 being "determine the set of compile target dependencies for this Python command" (whether that specific function is invoked or skipped) must be the comprehensive set of all MRtrix3 commands (not just C++ binaries), since without knowledge of those dependencies compilation of all targets is the only safe option > 6. Add a script that acts as a linter for explicit Python command dependencies and is invoked by CI Action "secondary-checks". The script must first generate a list of all MRtrix3 commands present in the worktree. Then, it must loop over all Python commands. If constant MRTRIX_DEPENDENCIES is not defined, an error is reported. If an invocation of `run.command()` is found, where the invoked command is trivially parsed statistically (ie. no variable substitution) (noting that run.command() can take as first input argument a string, f-string, or list-of-strings), but that MRtrix3 command is not defined in MRTRIX_DEPENDENCIES, an error is reported. If function image.mrinfo() is called, or image.Header class is constructed, but command "mrinfo" is not present in MRTRIX_DEPENDENCIES, an error is reported. Generated-by: Claude Opus 4.8 <noreply@anthropic.com>
Assisted-by: Claude Opus 4.8 <noreply@anthropic.com>
Move per-command MRtrix3 dependency declarations to a per-file model so that commands whose source spans multiple files distribute their MRTRIX_DEPENDENCIES across those files rather than aggregating them in __init__.py. Each package __init__.py now seeds the constant as an empty set, and every constituent source file augments it via "|=" with the commands it itself invokes (importing the shared set so the augmented assignment is valid in the submodule namespace); single-file commands declare a set literal directly. The cmake parser is rescoped to a single .py file (ParsePythonFileDependencies), while the resolver unions the per-file declarations across all of a command's sources before closing over transitive Python-to-Python dependencies. The dependency linter is updated to parse the set/"|=" syntax and verify each source file against its own declaration. Session prompts: 1. > In HEAD~1, Python commands were given the ability to specify the set of MRtrix3 commands on which they depend, so that for selectively targeted builds only those other commands requisite for operation of that Python command are built. For Python commands whose code is spread across multiple source files, prior instruction was to aggregate these dependencies into the corresponding __init__.py file. This is however sub-optimal for some Python commands that operate as an interface to a range of different algorithms, each with their own CLI sub-parser. It is instead preferred that constant MRTRIX_DEPENDENCIES be manipulated by every source code file. The build system must then aggregate these dependencies to produce a list of build target dependencies for that Python command. > 1. Modify cmake: ParsePythonCommandDependencies must parse an individual .py file, and be renamed accordingly; ResolvePythonCommandDependencies must produce for a single Python command a list of build dependencies for cmake that deals not only with transitive dependencies but also receiving a list of all dependencies of all files constituting the source code of that command. > 2. Modify script check_python_dependencies to reflect the fact that for Python commands whose source code is spread across multiple files, the dependencies will be provided on a per-file basis rather than in the command's __init__.py file. > 3. MRTRIX_DEPENDENCIES is to be defined as a set rather than a list. > 4. For Python commands whose source code is distributed across multiple .py files, file __init__.py is to define it as an empty set (assuming no MRtrix3 commands are invoked from inside __init__.py); each individual .py file is then to update it using the union update operator "|=" based on the set of MRtrix3 commands that are dependencies of the source code within that file. > 5. One all tasks are completed execute the /squash-session skill. Generated-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two CI regressions on the per-command Python dependency branch are addressed. The CMake logic that enumerates Python commands selected entries by blocklisting known non-commands, so an in-source build's generated "CMakeFiles" directory was mistaken for a command and aborted configuration; it now allow-lists genuine commands (a "<name>.py" file, or a directory containing "__init__.py"), which also immunises against other build artefacts deposited alongside the sources. Separately, the population_template command disabled too-many-positional-arguments inline, but that check exists only in newer pylint, so CI's older pylint rejected the directive as an unknown option value. The two affected functions now make their optional parameters keyword-only, dropping the positional-argument count below the threshold and removing the need for any version-specific disable. Session prompts: 1. > Compilation on CI Actions failed due to the error message pasted below. Logic that parses the set of Python commands for which MRtrix3 command dependencies must be discovered needs to exclude CMakeFiles.txt. 2. > Investigate pylint errors in population_template files in CI. Note that pylint versions may differ between CI and local machine. 3. > Generate plan for refactoring the two problematic lines to satisfy the too-many-positional-arguments check so that supporting check disabling across incompatible pylint versions is no longer necessary. 4. > Apply edits, perform any changes required to pass ./run_pylint, then execute the /squash-session skill. Generated-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
Lestropie
marked this pull request as ready for review
June 8, 2026 11:13
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intended to work in conjunction with MRtrix3/external-project-template#2.
I want for it to be possible for an external MRtrix3 project that provides one or more Python commands to be able to selectively build and package with it only those MRtrix3 commands that need to be built in order for that external Python command to function. In addition, during development if one is working on a Python command it would be beneficial if a targeted build of that command would only recompile the minimum set of other MRtrix3 commands that need to be built for the manifest behaviour of the command to reflect code changes.
There are multiple ways to do this. I decided on defining within the source code files themselves a set of strings encoding the commands the code in that file depends upon, which can be parsed by
cmake(without invoking the Python interpreter) to build the dependency graph. This will hopefully mean that the same mechanism can be extended to the building of external projects.New command
check_python_dependencies(which will probably eventually be moved intopython/along withrun_pylintin #3083) is a linter of limited capability: it will catch obvious omissions of commands from that list in CI, but it's not guaranteed to catch all (command names could be stored in variables).