Skip to content

Command format fix with backslashes on Windows System #5280

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

Merged
merged 5 commits into from
Oct 14, 2022

Conversation

Maxime-Perret
Copy link
Contributor

@Maxime-Perret Maxime-Perret commented Oct 6, 2022

Fixes MONAI/apps/Auto3DSeg/bundle_gen.py

Description

Commands ran in subprocess currently cause issues with string formatting and backslashes not being escaped properly. Changing from Back Flash to Forward Slash solves the issue.

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.

@wyli
Copy link
Contributor

wyli commented Oct 6, 2022

thanks, could you please make a minimal example to describe the bug following the issue template? https://github.com/Project-MONAI/MONAI/issues/new?assignees=&labels=&template=bug_report.md&title=

@Maxime-Perret
Copy link
Contributor Author

Hello @wyli and thank you for your answer.

I apologize for not properly introducing my pull request. This comes from the discussion I opened on one of the tutorial.

I was asked to submit a pull request to fix the first bug I mentioned in that discussion. It comes from running the auto3dseg_autorunner_ref_api.ipynb notebook, I believe the issue being because of Windows, and the pull request I submitted fixes the notebook for me at least.

Let me know if there is something else I should provide.

@wyli
Copy link
Contributor

wyli commented Oct 6, 2022

thanks for reporting, I think the issue is partly from the notebook https://github.com/Project-MONAI/tutorials/blob/main/auto3dseg/notebooks/auto3dseg_autorunner_ref_api.ipynb

root = "./"

which is an os-dependent path... perhaps we should use from pathlib import Path as much as possible cc@Nic-Ma @mingxin-zheng

@mingxin-zheng
Copy link
Contributor

@wyli @Maxime-Perret I will have access to a GPU windows system later today, and plan to update the path in the Auto3Dseg tutorial

@mingxin-zheng
Copy link
Contributor

@Maxime-Perret After some debugging, I would suggest replacing this line

config_yaml = os.path.join(config_dir, file)

with

config_yaml = Path(os.path.join(config_dir, file)).as_posix()

The path problem occurred when we add single quotes to the base_cmd

base_cmd += f"'{config_yaml}'"

when config_yaml contains double backslash \\ and Python Fire would mistake it as a single backslash later in the pipeline.

Changing the cmd in _run_cmd would also fix this problem, but it would limit future usage when someone wants to enforce \\ in the code somewhere.

@wyli
Copy link
Contributor

wyli commented Oct 13, 2022

@mingxin-zheng do you think double quotes would address this already and to escape some other characters such as white space in the filepath. https://stackoverflow.com/questions/36379789/python-subprocess-unable-to-escape-quotes

@mingxin-zheng
Copy link
Contributor

Hi @wyli , do you mean replacing single quotes with double? This is more of an issue from PythonFire rather than the subprocess call. Here is an example how to pass a str to Python Fire:
https://google.github.io/python-fire/guide/#argument-parsing

@wyli
Copy link
Contributor

wyli commented Oct 13, 2022

I see, strange that PythonFire doesn't have any discussions about this...

@mingxin-zheng
Copy link
Contributor

mingxin-zheng commented Oct 13, 2022

It is possible that file paths are not frequently used as inputs in Python Fire.

For a list of 2 file strings, one needs to put up something like:
"'C:\\\\pathA\\\\fileA','C:\\\\pathB\\\\fileB'"
on Windows platform to ensure Python Fire can parse it correctly, or instead, just use
"'C:/pathA/fileA','C:/pathB/fileB'"

I think the latter is better as I haven't experienced any errors using / for file paths on Windows.

Signed-off-by: Mingxin Zheng <[email protected]>
@mingxin-zheng mingxin-zheng requested review from mingxin-zheng and wyli and removed request for mingxin-zheng October 14, 2022 03:00
@mingxin-zheng
Copy link
Contributor

I have tested some of the Auto3DSeg codes on a Windows/GPU platform and the file path issues are gone.

It looks good to me now.

@mingxin-zheng mingxin-zheng changed the title Command format fix with backslashes Command format fix with backslashes on Windows System Oct 14, 2022
@mingxin-zheng mingxin-zheng requested a review from Nic-Ma October 14, 2022 05:46
@wyli
Copy link
Contributor

wyli commented Oct 14, 2022

/build

@wyli wyli enabled auto-merge (squash) October 14, 2022 05:56
@wyli wyli merged commit 731c743 into Project-MONAI:dev Oct 14, 2022
wyli pushed a commit to wyli/MONAI that referenced this pull request Oct 15, 2022
…5280)

Fixes MONAI/apps/Auto3DSeg/bundle_gen.py

### Description

Commands ran in subprocess currently cause issues with string formatting
and backslashes not being escaped properly. Changing from Back Flash to
Forward Slash solves the issue.

### 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: Maxime <[email protected]>
Signed-off-by: Mingxin Zheng <[email protected]>
Signed-off-by: Mingxin Zheng <[email protected]>
Co-authored-by: Maxime <[email protected]>
Co-authored-by: Mingxin Zheng <[email protected]>
Co-authored-by: Mingxin Zheng <[email protected]>
Co-authored-by: Mingxin Zheng <[email protected]
@Maxime-Perret
Copy link
Contributor Author

@Maxime-Perret After some debugging, I would suggest replacing this line

config_yaml = os.path.join(config_dir, file)

with

config_yaml = Path(os.path.join(config_dir, file)).as_posix()

The path problem occurred when we add single quotes to the base_cmd

base_cmd += f"'{config_yaml}'"

when config_yaml contains double backslash \\ and Python Fire would mistake it as a single backslash later in the pipeline.

Changing the cmd in _run_cmd would also fix this problem, but it would limit future usage when someone wants to enforce \\ in the code somewhere.

Hello @mingxin-zheng,
Forgive my late answer as I was out of office, thank you for your proper path fix.
This works well for me, and with the latest changes to the auto3dseg_autorunner_ref_api.ipynb notebook with task_04, it runs smooth. I'm grateful for the quick help!

bhashemian pushed a commit to JHancox/MONAI that referenced this pull request Oct 20, 2022
…5280)

Fixes MONAI/apps/Auto3DSeg/bundle_gen.py

### Description

Commands ran in subprocess currently cause issues with string formatting
and backslashes not being escaped properly. Changing from Back Flash to
Forward Slash solves the issue.

### 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: Maxime <[email protected]>
Signed-off-by: Mingxin Zheng <[email protected]>
Signed-off-by: Mingxin Zheng <[email protected]>
Co-authored-by: Maxime <[email protected]>
Co-authored-by: Mingxin Zheng <[email protected]>
Co-authored-by: Mingxin Zheng <[email protected]>
Co-authored-by: Mingxin Zheng <[email protected]
Signed-off-by: Behrooz <[email protected]>
@Maxime-Perret Maxime-Perret deleted the update branch October 26, 2022 10:03
KumoLiu pushed a commit that referenced this pull request Nov 2, 2022
Fixes MONAI/apps/Auto3DSeg/bundle_gen.py

### Description

Commands ran in subprocess currently cause issues with string formatting
and backslashes not being escaped properly. Changing from Back Flash to
Forward Slash solves the issue.

### 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: Maxime <[email protected]>
Signed-off-by: Mingxin Zheng <[email protected]>
Signed-off-by: Mingxin Zheng <[email protected]>
Co-authored-by: Maxime <[email protected]>
Co-authored-by: Mingxin Zheng <[email protected]>
Co-authored-by: Mingxin Zheng <[email protected]>
Co-authored-by: Mingxin Zheng <[email protected]
Signed-off-by: KumoLiu <[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.

3 participants