Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up FSLeyes version checking to be less verbose/confusing #3820

Merged
merged 5 commits into from Jul 8, 2022

Conversation

joshuacwnewton
Copy link
Member

@joshuacwnewton joshuacwnewton commented Jun 27, 2022

Checklist

GitHub

PR contents

Description

This PR is meant to simplify the complex error code checking for FSLeyes. Rather than give detailed custom descriptions for every possible case, I've instead opted to echo the native error thrown by the shell.

Our goal with the custom error messages was to try to explain that FSLeyes wasn't present, but that we recommend the tool. That messaging was a bit muddled, though, given that multiple users have interpreted it as a critical error, when instead FSLeyes is optional. So, I was hoping that OPTIONAL DEPENDENCIES would be sufficient for conveying that FSLeyes is a nice to have, but not necessary.

On Ubuntu, it looks like this:

joshua@tadpole:~$ sct_check_dependencies

--
Spinal Cord Toolbox (git-jn/3816-clean-up-fsleyes-dependency-check-dd3bc041faa08e1ae10ab3dcdccd78aa203a674b)

sct_check_dependencies 
--


SYSTEM INFORMATION
------------------
SCT info:
- version: git-jn/3816-clean-up-fsleyes-dependency-check-dd3bc041faa08e1ae10ab3dcdccd78aa203a674b
- path: /home/joshua/repos/spinalcordtoolbox
OS: linux (Linux-5.17.5-76051705-generic-x86_64-with-debian-bookworm-sid)
CPU cores: Available: 4, Used by ITK functions: 4
RAM: Total: 15699MB, Used: 5824MB, Available: 8537MB

MANDATORY DEPENDENCIES
----------------------
Check Python executable.............................[OK]
  Using bundled python 3.7.13 (default, Mar 29 2022, 02:18:16) 
[GCC 7.5.0] at /home/joshua/repos/spinalcordtoolbox/python/envs/venv_sct/bin/python
Check if data are installed.........................[OK]
Check if dipy is installed..........................[OK] (1.5.0)
Check if ivadomed is installed......................[OK] (2.9.6)
Check if matplotlib is installed....................[OK] (3.5.2)
Check if nibabel is installed.......................[OK] (3.2.2)
Check if numpy is installed.........................[OK] (1.21.6)
Check if onnxruntime is installed...................[OK] (1.7.0)
Check if pandas is installed........................[OK] (1.3.5)
Check if portalocker is installed...................[OK] (2.4.0)
Check if psutil is installed........................[OK] (5.9.1)
Check if pyqt5 (5.11.3) is installed................[OK] (5.11.3)
Check if pytest is installed........................[OK] (7.1.2)
Check if pytest-cov is installed....................[OK] (3.0.0)
Check if raven is installed.........................[OK]
Check if requests is installed......................[OK] (2.28.0)
Check if requirements-parser is installed...........[OK]
Check if scipy is installed.........................[OK] (1.7.3)
Check if scikit-image is installed..................[OK] (0.19.3)
Check if scikit-learn is installed..................[OK] (1.0.2)
Check if xlwt is installed..........................[OK] (1.3.0)
Check if tqdm is installed..........................[OK] (4.64.0)
Check if transforms3d is installed..................[OK] (0.3.1)
Check if urllib3 is installed.......................[OK] (1.26.9)
Check if pytest_console_scripts is installed........[OK]
Check if pyyaml is installed........................[OK] (6.0)
Check if wquantiles is installed....................[OK] (0.4)
Check if spinalcordtoolbox is installed.............[OK]
Check ANTs compatibility with OS ...................[OK]
[OK]
Check if figure can be opened with matplotlib.......[OK] (Using GUI backend: 'QtAgg')
Check if figure can be opened with PyQt.............[OK]

OPTIONAL DEPENDENCIES
---------------------
Check FSLeyes version...............................[  ]
   (127, '/bin/sh: 1: fsleyes: not found') 

But, perhaps this is still too intrusive? We could move "Optional Dependencies" to be before "Mandatory Dependencies" so that it isn't the first thing the user sees upon running sct_check_dependencies. That way, it's less likely to spook anyone into thinking that it might be an error, but will still be present for our own debugging (on the forum, etc.)

Very open to discussion here on how it would be best to display this information... 馃

Linked issues

Fixes #3816.

This is supposed to help convey to the user that if an error occurs while checking the FSLeyes version, then it's non-critical and won't affect the processing of SCT.
@joshuacwnewton joshuacwnewton force-pushed the jn/3816-clean-up-fsleyes-dependency-check branch from dd3bc04 to 303f9f6 Compare June 27, 2022 19:10
@codecov
Copy link

codecov bot commented Jun 27, 2022

Codecov Report

Merging #3820 (a5e1e24) into master (9c6441c) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Flag Coverage 螖
api-tests 21.87% <0.00%> (+<0.01%) 猬嗭笍
cli-tests 58.77% <0.00%> (+<0.01%) 猬嗭笍

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage 螖
...pinalcordtoolbox/scripts/sct_check_dependencies.py 0.00% <0.00%> (酶)

    # NB: We put this section first because typically, it will error out, since FSLeyes isn't installed by default.
    #     SCT devs want to have access to this information, but we don't want to scare our users into thinking that
    #     there's a critical error. So, we put it up top to allow the installation to end on a nice "OK" note.
Copy link
Member

@mguaypaq mguaypaq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The output is as discussed in the SCT dev meeting, so LGTM. Thanks!

@joshuacwnewton joshuacwnewton merged commit df74db8 into master Jul 8, 2022
@joshuacwnewton joshuacwnewton deleted the jn/3816-clean-up-fsleyes-dependency-check branch July 8, 2022 19:01
@mguaypaq mguaypaq added this to the 5.7 milestone Jul 28, 2022
@mguaypaq mguaypaq added enhancement category: improves performance/results of an existing feature sct_check_dependencies context: labels Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement category: improves performance/results of an existing feature sct_check_dependencies context:
Projects
None yet
Development

Successfully merging this pull request may close these issues.

non-elegant error when checking FSLeyes
2 participants