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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quote $PY_EXE variable to deal with Python path that contain spaces in Bash #7268

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

ytl0623
Copy link
Contributor

@ytl0623 ytl0623 commented Nov 29, 2023

Fixes #5857.

Description

When dealing with paths that contain spaces in Bash, it's important to properly quote the variables to ensure that spaces are handled correctly. So, maybe we can replace all $PY_EXE variables to "$PY_EXE" in the runtests.sh file.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@ericspod
Copy link
Member

Hi @ytl0623 thanks for the contribution. Could you please remove the changes to the source files that have been introduced? These are unrelated to the runtests.sh script file. Once that's done we can check that the script still works manually.

@ericspod ericspod requested a review from KumoLiu November 29, 2023 13:11
@ytl0623
Copy link
Contributor Author

ytl0623 commented Nov 29, 2023

Hi @ericspod, I've removed other unrelated changes to the runtests.sh script file. Could you please help me review it again?

Thanks in advance!

Copy link
Contributor

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

LGTM, I have not tried it yet.

Copy link
Member

@ericspod ericspod left a comment

Choose a reason for hiding this comment

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

Looks good now, thanks!

@KumoLiu
Copy link
Contributor

KumoLiu commented Nov 29, 2023

/build

@KumoLiu KumoLiu merged commit a501252 into Project-MONAI:dev Nov 30, 2023
23 checks passed
@dzenanz
Copy link
Contributor

dzenanz commented Nov 30, 2023

This partly works:

Dzenan@Ryzenator MINGW64 /m/Dev/MONAI (dev)
$ ./runtests.sh --quick --unittests  --disttests
Run tests under /m/Dev/MONAI
PYTHONPATH: /m/Dev/MONAI:
Python: /c/Program Files/Python39/python
MONAI version: 1.3.0+38.gac6bd5dd
Numpy version: 1.22.4
Pytorch version: 2.0.1+cu117
MONAI flags: HAS_EXT = False, USE_COMPILED = False, USE_META_DICT = False
MONAI rev id: ac6bd5dd4ccd6c1e19c259dc93e1eb61610c7876
MONAI __file__: M:\Dev\MONAI\monai\__init__.py

Optional dependencies:
Pytorch Ignite version: 0.4.11
ITK version: 5.3.0
Nibabel version: 3.2.1
scikit-image version: 0.22.0
scipy version: 1.11.4
Pillow version: 9.2.0
Tensorboard version: 2.6.0
gdown version: 4.7.1
TorchVision version: 0.15.2+cu117
tqdm version: 4.62.2
lmdb version: 1.4.1
psutil version: 5.8.0
pandas version: 1.3.2
einops version: 0.7.0
transformers version: 4.21.3
mlflow version: 2.8.1
pynrrd version: 1.0.0
clearml version: 1.13.2

For details about installing the optional dependencies, please visit:
    https://docs.monai.io/en/latest/installation.html#installing-the-recommended-dependencies

--------------------------------------------------------------------------------
quick
--------------------------------------------------------------------------------
coverage
--------------------------------------------------------------------------------
unittests
2.0.1+cu117
tensor([[0.3538, 0.6480, 0.5858],
        [0.6119, 0.5092, 0.2991],
        [0.1511, 0.2344, 0.4977],
        [0.5254, 0.6991, 0.4507],
        [0.0519, 0.8435, 0.1419]])
./runtests.sh: line 723: /c/Program: No such file or directory

@ericspod
Copy link
Member

Yes it looks like we missed a few places, wherever ${cmd} occurs it needs to be quoted as well. @ytl0623 could you do that fix and propose a new PR? Thanks!

@dzenanz
Copy link
Contributor

dzenanz commented Nov 30, 2023

I could give it a try.

@dzenanz
Copy link
Contributor

dzenanz commented Nov 30, 2023

Uh-oh, a few quick tries all failed. The closest I got was: ./runtests.sh: line 723: /c/Program Files/Python39/python -m coverage run --append ./tests/runner.py -p "^(?!test_integration).*(?<!_dist)$": No such file or directory. I will leave this to someone more familiar with bash syntax.

@ytl0623
Copy link
Contributor Author

ytl0623 commented Dec 5, 2023

Hi, @dzenanz @ericspod, maybe you can try ${cmdPrefix}"${cmd}" ./tests/runner.py -p "^(?!test_integration).*(?<!_dist)$" in runtests.sh file.

This works for me.

unittests
Running tests in folder: '.'
With file pattern: '^(?!test_integration).*(?<!_dist)$'
monai test runner: excluding tests.test_rankfilter_dist
monai test runner: excluding tests.test_integration_determinism
monai test runner: excluding tests.test_integration_autorunner
monai test runner: excluding tests.test_integration_fast_train
monai test runner: excluding tests.test_integration_gpu_customization
monai test runner: excluding tests.test_call_dist
monai test runner: excluding tests.test_integration_stn
monai test runner: excluding tests.test_cumulative_average_dist
monai test runner: excluding tests.test_sampler_dist
monai test runner: excluding tests.test_integration_bundle_run
monai test runner: excluding tests.test_integration_segmentation_3d
monai test runner: excluding tests.test_integration_workers
monai test runner: excluding tests.test_timedcall_dist
monai test runner: excluding tests.test_handler_metrics_saver_dist
monai test runner: excluding tests.test_integration_nnunetv2_runner
monai test runner: excluding tests.test_handler_confusion_matrix_dist
monai test runner: excluding tests.test_integration_unet_2d
monai test runner: excluding tests.test_integration_lazy_samples
monai test runner: excluding tests.test_integration_workflows_gan
monai test runner: excluding tests.test_persistentdataset_dist
monai test runner: excluding tests.test_weighted_random_sampler_dist
monai test runner: excluding tests.test_lmdbdataset_dist
monai test runner: excluding tests.test_handler_rocauc_dist
monai test runner: excluding tests.test_handler_classification_saver_dist
monai test runner: excluding tests.test_prepare_batch_default_dist
monai test runner: excluding tests.test_handler_regression_metrics_dist
monai test runner: excluding tests.test_evenly_divisible_all_gather_dist
monai test runner: excluding tests.test_integration_workflows
monai test runner: excluding tests.test_integration_sliding_window
monai test runner: excluding tests.test_integration_classification_2d
monai test runner: excluding tests.test_fl_monai_algo_dist
monai test runner: excluding tests.test_cv2_dist

@dzenanz
Copy link
Contributor

dzenanz commented Dec 5, 2023

That's one of the first things I tried. That gives me: ./runtests.sh: line 723: /c/Program Files/Python39/python -m coverage run --append: No such file or directory

marksgraham pushed a commit to marksgraham/MONAI that referenced this pull request Jan 30, 2024
…n Bash (Project-MONAI#7268)

Fixes Project-MONAI#5857.

### Description
When dealing with paths that contain spaces in Bash, it's important to
properly quote the variables to ensure that spaces are handled
correctly. So, maybe we can replace all `$PY_EXE` variables to
`"$PY_EXE"` in the `runtests.sh` file.

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] Non-breaking change (fix or new feature that would not break
existing functionality).
- [ ] Breaking change (fix or new feature that would cause existing
functionality to change).
- [ ] New tests added to cover the changes.
- [ ] Integration tests passed locally by running `./runtests.sh -f -u
--net --coverage`.
- [ ] Quick tests passed locally by running `./runtests.sh --quick
--unittests --disttests`.
- [ ] In-line docstrings updated.
- [ ] Documentation updated, tested `make html` command in the `docs/`
folder.

Signed-off-by: ytl0623 <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
juampatronics pushed a commit to juampatronics/MONAI that referenced this pull request Mar 25, 2024
…n Bash (Project-MONAI#7268)

Fixes Project-MONAI#5857.

### Description
When dealing with paths that contain spaces in Bash, it's important to
properly quote the variables to ensure that spaces are handled
correctly. So, maybe we can replace all `$PY_EXE` variables to
`"$PY_EXE"` in the `runtests.sh` file.

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] Non-breaking change (fix or new feature that would not break
existing functionality).
- [ ] Breaking change (fix or new feature that would cause existing
functionality to change).
- [ ] New tests added to cover the changes.
- [ ] Integration tests passed locally by running `./runtests.sh -f -u
--net --coverage`.
- [ ] Quick tests passed locally by running `./runtests.sh --quick
--unittests --disttests`.
- [ ] In-line docstrings updated.
- [ ] Documentation updated, tested `make html` command in the `docs/`
folder.

Signed-off-by: ytl0623 <[email protected]>
Signed-off-by: Juan Pablo de la Cruz Gutiérrez <[email protected]>
Yu0610 pushed a commit to Yu0610/MONAI that referenced this pull request Apr 11, 2024
…n Bash (Project-MONAI#7268)

Fixes Project-MONAI#5857.

### Description
When dealing with paths that contain spaces in Bash, it's important to
properly quote the variables to ensure that spaces are handled
correctly. So, maybe we can replace all `$PY_EXE` variables to
`"$PY_EXE"` in the `runtests.sh` file.

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] Non-breaking change (fix or new feature that would not break
existing functionality).
- [ ] Breaking change (fix or new feature that would cause existing
functionality to change).
- [ ] New tests added to cover the changes.
- [ ] Integration tests passed locally by running `./runtests.sh -f -u
--net --coverage`.
- [ ] Quick tests passed locally by running `./runtests.sh --quick
--unittests --disttests`.
- [ ] In-line docstrings updated.
- [ ] Documentation updated, tested `make html` command in the `docs/`
folder.

Signed-off-by: ytl0623 <[email protected]>
Signed-off-by: Yu0610 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

runtests.sh does not work on Windows
4 participants