MONAI icon indicating copy to clipboard operation
MONAI copied to clipboard

Command format fix with backslashes

Open Maxime-Perret opened this issue 3 years ago • 4 comments

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

  • [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.

Maxime-Perret avatar Oct 06 '22 14:10 Maxime-Perret

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=

wyli avatar Oct 06 '22 15:10 wyli

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.

Maxime-Perret avatar Oct 06 '22 18:10 Maxime-Perret

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

wyli avatar Oct 06 '22 18:10 wyli

@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 avatar Oct 11 '22 03:10 mingxin-zheng

@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.

mingxin-zheng avatar Oct 13 '22 13:10 mingxin-zheng

@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

wyli avatar Oct 13 '22 14:10 wyli

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

mingxin-zheng avatar Oct 13 '22 14:10 mingxin-zheng

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

wyli avatar Oct 13 '22 14:10 wyli

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.

mingxin-zheng avatar Oct 13 '22 15:10 mingxin-zheng

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 avatar Oct 14 '22 03:10 mingxin-zheng

/build

wyli avatar Oct 14 '22 05:10 wyli

@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!

Maxime-Perret avatar Oct 20 '22 14:10 Maxime-Perret