Python CLI changes #2678

Lestropie · 2023-07-12T11:23:10Z

Somewhat requisite for #2665.

Closes #1392.

Many outstanding requirements as per checkbox list in #1392.

Created custom callable classes for handling different datatypes for positional and optional arguments on the command line. Used ArgumentTypeError for exception management instead of ValueError since the former allows allows adding custom error messages.

Added newly defined callables (in app.py) for each positional and optional command line argument of mrtrix_cleanup and dwicat

Updated the logic for TypeBoolean class for consistency with C++ command-line behaviour Added new checks in TypeInputTractogram class for file validation

Added callables for each positional and optional command line argument in dwifslpreproc, dwigradcheck, dwishellmath, labelsgmfix, mask2glass, population_template and responsemean

Co-authored-by: Ankita Sanil <[email protected]>

Used the new syntax as "type=app.Parser.TypeInputImage()" across all Python API commands

…ent) Replaced the traditional for loop with list comprehension in TypeIntegerSequence and TypeFloatSequence classes

Applies to both population-template and mrregister. Makes "none" a valid selection of robust estimator in both cases.

Updated class names across all commands to be in sync with C++ code

Implemented class inheritance to avoid duplicate checks for tractogram input files. Instead, reused the checks from ArgFileIn type via inheritance.

Changes for handling piped images in the Python API scripts. However, this implementation does not include deletion of the temp/piped images at the end of the command execution.

The current implementation is temporary since it doesn't cover all the use-cases. However, it supports a working scenario.

Primarily renaming of classes to more closely echo the modifier functions that are used in the C++ usage() function rather than the enumeration that is hidden from most developers.

bin/dwi2response

bin/dwishellmath

cmd/mrregister.cpp

core/signal_handler.cpp

lib/mrtrix3/dwi2mask/fslbet.py

lib/mrtrix3/dwi2response/dhollander.py

core/signal_handler.cpp

Addressing multiple comments in PR #2678.

In particular, it is desired for function make_temporary() to be accessible from within the app module.

Lestropie · 2024-05-14T08:29:19Z

Documenting here a gotcha I encountered during the process of merge conflict resolution:

Here argparse returns pathlib objects for user-specified filesystem paths. Thesee may include whitespace characters, and will have double quotation marks inserted if written to an f-string. However, if one uses such information as an input to os.path.join()---for instance, to access a file within a user-specified input directory---then the result of that will be a string, which could then cause problems if invoking run.command() with a string rather than list-of-strings input. Currently I'm falling back to using list inputs to run.command() where necessary. An alternative would be to take relevant instances of os.path.join() and either do that operation with, or cast the result to, the custom derivative of pathlib.Path that quote-escapes when formatted to an f-string; which I'm not a massive fan of. I'm at least setting the precedent here to be testing operation of scripts where filesystem paths may contain whitespace.

- Fix syntax error in mask2glass introduced in 99dd927. - population_template: F-string syntax fix. - 5ttgen hsvs: Fix execution when input directory path contains whitespace. - app.Parser: Change type of Exception thrown when filesystem path argument is impermissible, so that argparse catches it. - dwi2mask consensus: Allow execution when the -template option is not specified; just omit from the list any algorithms that depend on such. - dwinormalise group: Fix path to population_template scratch directory. - Testing: Many fixes to individual test bash scripts, particularly around the verification of image pipe and filesystem path whitespace characters.

In #2678 the working directory for unit tests is set to new directory testing/data/. But in #2865, bash tests are executed as-is, rather than being imported and executed on a per-line basis. Resolving these requires setting the absolute path to the test bash script.

- Restore solution in #2845 not properly propagated through prior merge conflict. - Use FileExistsError when checking for pre-existing output files / directories, and catch it to yield a well-formatted error message. - Update CLI test data to reflect changes in 05b68d5. - Modify tests that check for command error due to inappropriate CLI usage.

Lestropie · 2024-05-19T01:45:22Z

Annoying CI failures that I don't think have anything to do with this PR.

Linux Clang

: && /usr/bin/clang++  -O3 -DNDEBUG   cmd/CMakeFiles/warpinit.dir/warpinit.cpp.o  -o bin/warpinit  -Wl,-rpath,/home/runner/work/mrtrix3/mrtrix3/build/src:/home/runner/work/mrtrix3/mrtrix3/build/core:  src/libmrtrix-headless.so  src/libmrtrix-exec-version-lib.a  core/libmrtrix-core.so  /usr/lib/x86_64-linux-gnu/libpng.so  /usr/lib/x86_64-linux-gnu/libtiff.so  /usr/lib/x86_64-linux-gnu/libz.so  /usr/lib/x86_64-linux-gnu/libfftw3.so  /usr/lib/x86_64-linux-gnu/libfftw3f.so  /usr/lib/x86_64-linux-gnu/libfftw3l.so  /usr/lib/x86_64-linux-gnu/libfftw3.so  /usr/lib/x86_64-linux-gnu/libfftw3f.so  /usr/lib/x86_64-linux-gnu/libfftw3l.so  core/libmrtrix-core-version-lib.a && :
/usr/bin/ld: src/libmrtrix-headless.so: undefined reference to `std::ios_base_library_init()'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

(occurs for multiple targets)

MSYS2

FAILED: src/CMakeFiles/mrtrix-gui.dir/gui/opengl/gl_core_3_3.cpp.obj 
C:\hostedtoolcache\windows\sccache\0.8.0\x64\sccache C:\msys64\ucrt64\bin\c++.exe -DMINGW_HAS_SECURE_API=1 -DMRTRIX_BASE_VERSION=\"3.0.4\" -DMRTRIX_BUILD_TYPE=\"Release\" -DMRTRIX_PNG_SUPPORT -DMRTRIX_TIFF_SUPPORT -DMRTRIX_WINDOWS -DQT_CORE_LIB -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_NO_DEBUG -DQT_OPENGLWIDGETS_LIB -DQT_OPENGL_LIB -DQT_WIDGETS_LIB -DUNICODE -DWIN32 -DWIN64 -DWINVER=0x0A00 -D_ENABLE_EXTENDED_ALIGNED_STORAGE -D_UNICODE -D_WIN32_WINNT=0x0A00 -D_WIN64 -Dmrtrix_gui_EXPORTS -ID:/a/mrtrix3/mrtrix3/build/src/mrtrix-gui_autogen/include -ID:/a/mrtrix3/mrtrix3/src -ID:/a/mrtrix3/mrtrix3/core -isystem C:/msys64/ucrt64/include/eigen3 -isystem C:/msys64/ucrt64/include/qt6/QtCore -isystem C:/msys64/ucrt64/include/qt6 -isystem C:/msys64/ucrt64/share/qt6/mkspecs/win32-g++ -isystem C:/msys64/ucrt64/include/qt6/QtGui -isystem C:/msys64/ucrt64/include/qt6/QtWidgets -isystem C:/msys64/ucrt64/include/qt6/QtOpenGL -isystem C:/msys64/ucrt64/include/qt6/QtNetwork -isystem C:/msys64/ucrt64/include/qt6/QtOpenGLWidgets -O3 -DNDEBUG -std=gnu++17 -Werror -Winvalid-pch -include D:/a/mrtrix3/mrtrix3/build/src/CMakeFiles/mrtrix-gui.dir/cmake_pch.hxx -MD -MT src/CMakeFiles/mrtrix-gui.dir/gui/opengl/gl_core_3_3.cpp.obj -MF src\CMakeFiles\mrtrix-gui.dir\gui\opengl\gl_core_3_3.cpp.obj.d -o src/CMakeFiles/mrtrix-gui.dir/gui/opengl/gl_core_3_3.cpp.obj -c D:/a/mrtrix3/mrtrix3/src/gui/opengl/gl_core_3_3.cpp
D:/a/mrtrix3/mrtrix3/src/gui/opengl/gl_core_3_3.cpp:64:30: error: 'PROC' does not name a type
   64 | static int TestPointer(const PROC pTest) {

daljit46 · 2024-05-20T17:34:10Z

: && /usr/bin/clang++ -O3 -DNDEBUG cmd/CMakeFiles/warpinit.dir/warpinit.cpp.o -o bin/warpinit -Wl,-rpath,/home/runner/work/mrtrix3/mrtrix3/build/src:/home/runner/work/mrtrix3/mrtrix3/build/core: src/libmrtrix-headless.so src/libmrtrix-exec-version-lib.a core/libmrtrix-core.so /usr/lib/x86_64-linux-gnu/libpng.so /usr/lib/x86_64-linux-gnu/libtiff.so /usr/lib/x86_64-linux-gnu/libz.so /usr/lib/x86_64-linux-gnu/libfftw3.so /usr/lib/x86_64-linux-gnu/libfftw3f.so /usr/lib/x86_64-linux-gnu/libfftw3l.so /usr/lib/x86_64-linux-gnu/libfftw3.so /usr/lib/x86_64-linux-gnu/libfftw3f.so /usr/lib/x86_64-linux-gnu/libfftw3l.so core/libmrtrix-core-version-lib.a && :
/usr/bin/ld: src/libmrtrix-headless.so: undefined reference to `std::ios_base_library_init()'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

After troubleshooting this for a while (it was quite puzzling), I thought that this was probably caused by some mismatch between libstdc++ and clang. I tried to reproduce this locally in Docker without success and this directed me to think that the problem is likely with caching artefacts by sccache. Clearing the cache manually (using Github CLI) fixed the problem! It's likely there was some update on Ubuntu 22.04 which changed something in libstdc++, but the cached artefacts were incompatible with the change.

For the Windows failure, I'll need to have a further look.

Lestropie · 2024-05-20T23:47:09Z

Awesome, thanks for the effort.

Lestropie · 2024-05-23T09:41:18Z

😬 💣 🙉 💥

Resolves addition of peakscheck in #2918 with Python CLI changes in #2678.

Errors in refactoring in cd33930 as part of #2678.

ankitasanil and others added 16 commits February 6, 2023 11:25

Used callable types in commands - mrtrix_cleanup and dwicat

075db52

Added newly defined callables (in app.py) for each positional and optional command line argument of mrtrix_cleanup and dwicat

Changes done as instructed in the review

8d91a38

Updated the logic for TypeBoolean class for consistency with C++ command-line behaviour Added new checks in TypeInputTractogram class for file validation

Used callable types in few more commands

ebd36cf

Added callables for each positional and optional command line argument in dwifslpreproc, dwigradcheck, dwishellmath, labelsgmfix, mask2glass, population_template and responsemean

Python API: Integrate argument types across commands

2988229

Co-authored-by: Ankita Sanil <[email protected]>

Changed the syntax as per ankitasanil#1 (comment)

481462f

Used the new syntax as "type=app.Parser.TypeInputImage()" across all Python API commands

Python API: Used Python list comprehension as per ankitasanil#1 (comm…

54bca48

…ent) Replaced the traditional for loop with list comprehension in TypeIntegerSequence and TypeFloatSequence classes

population_template: Refine cmdline interface

cd33930

Change interfaces for robust linear registration estimators

9f3d624

Applies to both population-template and mrregister. Makes "none" a valid selection of robust estimator in both cases.

Python API: Updated class names as per ankitasanil#1 (comment)

51ccc67

Updated class names across all commands to be in sync with C++ code

Modifications in existing code to handle new argument types

47fec95

Modifications in existing code to handle new argument types (contd.)

b72c211

Added inheritance for TracksIn checks as per (ankitasanil#1 (comment))

6dcfe53

Implemented class inheritance to avoid duplicate checks for tractogram input files. Instead, reused the checks from ArgFileIn type via inheritance.

Python API: Ability to handle piped commands

935a8aa

Changes for handling piped images in the Python API scripts. However, this implementation does not include deletion of the temp/piped images at the end of the command execution.

Changes for output image piping

e749fe9

The current implementation is temporary since it doesn't cover all the use-cases. However, it supports a working scenario.

Python CLI: Tweaks to custom types

a830bf4

Primarily renaming of classes to more closely echo the modifier functions that are used in the C++ usage() function rather than the enumeration that is hidden from most developers.

Lestropie added enhancement Python API labels Jul 12, 2023

Lestropie self-assigned this Jul 12, 2023