MONAI
MONAI copied to clipboard
Command format fix with backslashes
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 htmlcommand in thedocs/folder.
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=
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.
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 @Maxime-Perret I will have access to a GPU windows system later today, and plan to update the path in the Auto3Dseg tutorial
@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 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
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
I see, strange that PythonFire doesn't have any discussions about this...
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.
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.
/build
@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_cmdbase_cmd += f"'{config_yaml}'"when
config_yamlcontains double backslash\\and Python Fire would mistake it as a single backslash later in the pipeline.Changing the
cmdin_run_cmdwould 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!