Skip to content

Script changes#1449

Merged
jdtournier merged 158 commits intodevfrom
script_changes
Apr 16, 2019
Merged

Script changes#1449
jdtournier merged 158 commits intodevfrom
script_changes

Conversation

@Lestropie
Copy link
Member

Mostly addressing #1400, but a few other things in there too; it was kind of a long steady accumulation of various things, I've tried to chop it up into a few commits as best I could.

They're significant library changes, and I don't like making changes that break compatibility with any stand-alone scripts out there, but I think there's strong justification for making the changes. After all, how many times have we modified the C++ library code in getting to the point we're at now?

I'll try to enumerate everything of interest here for comments. Not going to be merging this PR quickly, I should probably be implementing more tests before accepting such large changes; but I wanted to open the proposed changes up for discussion.
(Also I need to get this stuff out of my head so I can focus on more pertinent demands...)

  1. Completely changed how executable script files invoke the core application library.
    Previously there was a set of compulsory library functions that needed to be executed in the correct order, e.g. app.initialise(), app.parse(), app.complete(). Now, one defines two functions: usage() and execute() (couldn't have usage() and run() like in C++, since the latter would clash with the run module). After definition of these functions, app.execute() is invoked. app.execute() then sets everything up, using the inspect module to find those two defined functions and run them when appropriate.
    The biggest advantage of this is that it enables the use of exceptions. Instead of calling app.error(), library functions will throw a custom MRtrix3 exception, which can either be caught by the script programmer, or result in similar error message & cleanup behaviour as app.error(). This substantially improves the capability for scripts to catch when things don't quite work as intended and adjust appropriately.
    run.command() gets its own exception, which includes all information about the command and the stdout / stderr contents. This means that the error messaging and cleanup can occur entirely within app.execute() when the exception is called; as opposed to the current solution, which is partially managed by run.command() writing a text file that app.complete() then reads, which is kinda clunky.

  2. Moved some basic capabilities, e.g. listing available MRtrix3 binaries / importing the config file(s), to __init.py__. Anyone using the library functions but not invoking the app module in its entirety should still be able to access this information.

  3. Changes to run.command() to support multi-threaded usage.
    Note that this required the use of tempfile.mkstemp() rather than tempfile.TemporaryFile() due to an apparent race condition in the latter on Python3 on Windows (not present on Windows Python2; not yet tested on Linux). This is despite the Python documentation suggesting that TemporaryFile() uses mkstemp()... The side-effect of this is that the stdout / stderr contents of executed commands are written to files in /tmp/ and deleted after they are read; there could potentially be issues with users not having write access to that location and/or accumulation of stale files due to cleanup issues.

  4. Porting of foreach to Python. In addition to appearing in the online documentation, this also means that the terminal will present a simple progress bar showing the execution of however many instances of the command are run, and at completion only invocations that failed, along with their stdout / stderr contents, will be printed. Note that the command-line usage is fractionally different to the current bash script.

  5. New script responsemean, which has substantial changes from the basis average_response, but I think the name is more consistent with the rest of MRtrix3. In addition to making use of the libraries (and hence appearing in the online documentation), having better error messages, and being a little more Pythonic in places, the (default) averaging process is different, as proposed in average_response: De-mean before averaging #1096.

  6. Merge file and path modules into fsys. Really didn't like having to use pylint directives to hide the fact that a Python built-in was being over-ridden.

  7. Much nicer terminal feedback when running population_template, as you don't get humongous run.command() strings when running e.g. mrmath across all subjects (implemented the solution proposed in this comment in scripts: allow list input to run.command #928).

Partially addresses #692.
Hoping that #927 can be closed, but point 3 may raise other issues regarding the solution.
Closes #928.
Closes #1096.
Closes #1275.
Closes #1338 (script author can simply catch the exception along with stdout / stderr contents).
Closes #1377 (command returncode is returned in exception raised by run.command()).
Closes #1400.

thijsdhollander and others added 29 commits February 12, 2018 15:01
…uring development (will eventually again become a single dhollander algo though, if dhollander_new performs to expectations)
Previously, executable scripts would call a set of functions within the mrtrix3.app module in the appropriate order in order to initialise the consistent command-line interface, parsing, etc.. A copy-and-paste block of code at the top of each executable file would ensure that this module was present within the Python path.
This change makes the executable interface more similar to that of the C++ files within the cmd/ folder of MRtrix3. Two functions must be defined within the executable file: 'usage()' and 'execute()'. Then, at the bottom of the executable file, the copy-and-paste code block for locating and importing the mrtrix3.app module appears, but also calls the app.execute() function. This function defined within the app module sets up the interface, and uses the Python inspect module to locate and run the two functions defined within the executable file as appropriate.
A major advantage of this approach is that it allows Python exceptions to be thrown by library functions (instead of calling the deprecated app.error() function), which may optionally be caught by the calling code in various circumstances. Standard Python exceptions can also be caught by the app module, and displayed in a manner consistent with the script interface. Additionally, when commands executed via the mrtrix3.run module fail, a dedicated exception class is thrown, which allows both the catching of failed commands in the executing code where permissible, and moves all of the code responsible for handling such command failures to the app module (previously the run module would write a text file, which the app module would then load).
A couple of generic capabilities (e.g. reading the MRtrix3 config file(s)) have been moved to __init__.py, such that they are more easily accessible when using the library but not exploiting the app module interface.
Various changes to the mrtrix3.run module have been made to support multi-threaded use of the run.command() function. This includes use of tempfile.mkstemp() rather than tempfile.TemporaryFile(), as the latter was found to not be thread-safe in Python3 on Windows. The run.command() function additionally now supports list input, where individual entries may themselves be lists (allowing e.g. concatenation of multiple file paths into a command call, but the terminal display will show only the common prefix/suffix of the list rather than the entire contents). run.command() now additionally supports '||' and '&&' operators.
- dwibiascorrect: Fix cmdline function call.
- population_template: mrtrix3.app module needs to be loaded earlier for root-level functions.
- generate_user_docs.sh: Fix grep call based on new script syntax.
- 5ttgen gif: Add missing import call.
This script replaces average_response, performing the same function but with an executable name more consistent with the rest of MRtrix3.
It utilises the MRtrix3 Python scripting library, and therefore appears in the auto-generated documentation.
Additionally, while average_response simply averaged response function coefficients directly (meaning that subjects with greater image intensity would have a greater effect on the group average response function shape), responsemean (by default) takes into account any global differences in scaling between subjects when generating the mean. The former behaviour can however be accessed using the -legacy command-line option.
This commit contains a re-implementation of the 'foreach' script using the MRtrix3 Python scripting library, as opposed to the original implementation which is simply a Bash script. This means that the script will be included in the auto-generated documentation. It also means that the terminal output from multiple running commands can be better controlled.
This does however involve a change to the user interface of this command: Rather than placing a colon character as a 'no-op' to separate the 'foreach' invocation from the command to be executed multiple times, this new version instead expects the final positional command-line argument input to be a string that contains the command to be executed (including any substitution flags). The documentation has been updated to reflect this.
Python modules 'file' and 'path' have been merged into a single module 'fsys' (filesystem).
This in particular prevents overriding of the Python built-in 'file', which was very much not ideal.
@jdtournier jdtournier merged commit 1616942 into dev Apr 16, 2019
@jdtournier jdtournier deleted the script_changes branch April 16, 2019 18:39
@thijsdhollander
Copy link
Contributor

What a delight to wake up to this! 😄 🎉

@Lestropie
Copy link
Member Author

I'd pushed a change to rename the ANSI named tuple to VT100, but looking into it a bit more, it seems ANSI is appropriate

I made the same evolution myself. 👍

@Lestropie
Copy link
Member Author

What a delight to wake up to this!

I hadn't realised that you were such a big fan of all of the work I've done here.

Lestropie added a commit that referenced this pull request May 2, 2019
Residual issue with changes in #1449 that only arises when the CUDA version of eddy does not complete successfully.
Lestropie added a commit that referenced this pull request Aug 9, 2019
Error in resolving divergence between #1626 and #1449.
Lestropie added a commit that referenced this pull request Sep 25, 2019
Only two remaining functions within the Python API where it was considered more appropriate to use keyword arguments rather than ordered arguments. Was raised in #1736, but is ultimately downstream from #1449.
Lestropie added a commit that referenced this pull request Sep 26, 2019
Python3 on TravisCI have introduced new tests with pylint 2.4.0. This change should permit testing on master to pass:
- Use sys.exit() rather than exit().
- Remove some unnecessary comprehensions in dwipreproc.
- Tweak run_pylint script to not print erroneous warnings due to cache directories or empty directories left over from checking out other branches.
- Ignore import-outside-toplevel; given the overhaul of the Python API in #1449 conformance with this recommendation will instead be performed on the dev branch.
Lestropie added a commit that referenced this pull request Sep 26, 2019
Mirrors f9d1d54, which was applied to master; doing changes independently due to major Python API alterations in #1449.
- Remove unnecessary comprehensions in dwifslpreproc.
- Move all import statements to outer block at top of file.
- build: Use sys.exit(); remove unnecessary comprehension.
 Python signal handler: Do not use sys.exit() (recommended that signal handlers use os._exit() instead).
- run_pylint: Prevent error messages in case of empty directories in lib/mrtrix3/.
Lestropie added a commit that referenced this pull request Jan 30, 2020
Command return value integer is no longer a part of the CommandReturn named tuple, as since #1449 any command returning a non-zero value will instead raise an MRtrixCmdError.
Lestropie added a commit that referenced this pull request Mar 20, 2020
Erroneous merge in #1937 failed to account for parallel changes in Python API (#1449).
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.

4 participants