#2418 replace os.path by pathlib.Path by yuta0821 · Pull Request #2461 · pytorch/ignite

yuta0821 · 2022-02-14T05:54:42Z

Description:

Environment
In order to construct the test environment, I used docker image continuumio/miniconda3 and install necessary dependencies, following CONTRIBUTING.md.
Update code
- I have replaced all os.path.exists(folder), os.path.join(a, b), os.makedirs(folder) by Path(folder).exists(), Path(a) / b, Path.mkdir(folder, parents=True), respectively.
- I have updated all folder/files being str to Union[str, Path] and make all folder/file-like attributes as Path objects.
  - example
    - filename: str -> filename: Union[str, Path]
    - assert isinstance(fname, str) -> assert isinstance(fname, str) or isinstance(fname, Path)
Test the code
- I ran the following command
  pytest --tx 4*popen//python=python --cov ignite --cov-report term-missing --cov-report xml -vvv tests "${skip_distrib_opt[@]}"
- results
  I got an error due to this replacement in two test files.
  /opt/conda/envs/pytorch-ignite-dev/lib/python3.8/urllib/parse.py:111: in _decode_args return tuple(x.decode(encoding, errors) if x else '' for x in args)
  => AttributeError: 'PosixPath' object has no attribute 'decode'
  - /tests/ignite/contrib/handlers/test_mlflow_logger.py
    - line253: mlflow_logger = MLflowLogger(tracking_uri=Path(dirname) / "mlruns")
    - line295: with MLflowLogger(Path(dirname) / "mlruns") as mlflow_logger:
    - line327: with MLflowLogger(Path(dirname) / "mlruns") as mlflow_logger:
  - /tests/ignite/contrib/engines/test_common.py
    - line488: mlf_logger = _test_setup_logging(
- my proposition
  I think it would be better to use the original code using os.path without this replacement for the part that causes the error.
Code Format
In order to check code style, I ran the following commands.
isort . --settings setup.cfg
black . --config pyproject.toml

Check list:

New tests are added (if a new feature is added)
New doc strings: description and/or example code are in RST format
Documentation is updated (if required)

vfdev-5

Thanks a lot for the PR @yuta0821 ! Great work ! I left few comments to improve the code. As there is a lot of modified files, it can take time to review everything. A solution to that could be splitting the PR into mutiple ones, but not required...

ignite/handlers/checkpoint.py

setup.py

tests/ignite/conftest.py

tests/ignite/contrib/conftest.py

tests/ignite/contrib/engines/test_common.py

vfdev-5 · 2022-02-14T11:07:40Z

I got an error due to this replacement in two test files.
/opt/conda/envs/pytorch-ignite-dev/lib/python3.8/urllib/parse.py:111: in _decode_args return tuple(x.decode(encoding, errors) if x else '' for x in args)
=> AttributeError: 'PosixPath' object has no attribute 'decode'

/tests/ignite/contrib/handlers/test_mlflow_logger.py
    line253: mlflow_logger = MLflowLogger(tracking_uri=Path(dirname) / "mlruns")

@yuta0821 this is OK due to the fact that mlflow uses URI for tracking_uri which could be a local path or URL to a server. Let's keep tracking_uri as str.

…h object

yuta0821 · 2022-02-15T00:36:34Z

@vfdev-5
I modify some code you commented above.
I ran the following tests, and they all passed.
pytest --tx 4*popen//python=python --cov ignite --cov-report term-missing --cov-report xml -vvv tests "${skip_distrib_opt[@]}"
I would appreciate it if you could check them.

vfdev-5 · 2022-02-17T22:29:14Z

@yuta0821 could you please resolve conflicting files

vfdev-5

@yuta0821 I checked a bit more in details the PR and left other comments how we can simplify the code

ignite/handlers/checkpoint.py

tests/ignite/handlers/test_checkpoint.py

tests/ignite/handlers/test_time_profilers.py

yuta0821 · 2022-02-18T04:01:41Z

@vfdev-5

I checked a bit more in details the PR and left other comments how we can simplify the code.

Thank you for your reply and a lot of comments that helps me very much.
I modified some codes to treat as a path object and resolved conflict.
I ran the following tests, and they all passed.

pytest --tx 4*popen//python=python --cov ignite --cov-report term-missing --cov-report xml -vvv tests "${skip_distrib_opt[@]}"

I would appreciate it if you could confirm.

vfdev-5 · 2022-02-18T17:08:54Z

pytest --tx 4*popen//python=python --cov ignite --cov-report term-missing --cov-report xml -vvv tests "${skip_distrib_opt[@]}"

I would appreciate it if you could confirm.

@yuta0821 you can run all tests with bash tests/run_cpu_tests.sh (https://github.com/pytorch/ignite/blob/master/CONTRIBUTING.md#run-tests)

vfdev-5

Thanks a lot for the update @yuta0821 !
Just few nits to pick and should be good to go.

ignite/handlers/checkpoint.py

tests/ignite/test_utils.py

…b.com/yuta0821/ignite into feature/#2418_replace_os_path_by_Path

yuta0821 · 2022-02-18T23:55:25Z

@yuta0821 you can run all tests with bash tests/run_cpu_tests.sh

@vfdev-5 Thank you for your advise.
At first, I cannot run shell scripts probably due to the windows environment.
However, I was able to run tests/run_cpu_tests.sh after executing the command dos2unix and passed all tests.

I modified some codes to be match with your suggested change.
I would appreciate it if you could confirm.

ignite/handlers/checkpoint.py

vfdev-5 · 2022-02-19T16:30:14Z

@yuta0821 sorry I tried to improve type hints but I have to come back to what you did. Sorry for the noise.

I restart the CI and if it is successful I think we can merge this PR

EDIT: I'll fix the issues reported by the CI and move forward this PR

docs/source/conf.py

yuta0821 · 2022-02-20T04:26:41Z

@yuta0821 sorry I tried to improve type hints but I have to come back to what you did. Sorry for the noise.

Thank you for your try to improve type hints very much.

I restart the CI and if it is successful I think we can merge this PR
EDIT: I'll fix the issues reported by the CI and move forward this PR

If there is anything I can do for this PR, please let me know.

…b.com/yuta0821/ignite into feature/#2418_replace_os_path_by_Path

vfdev-5 · 2022-02-20T11:21:58Z

Thanks for the fix @yuta0821
I merged the PR !

yuta0821 · 2022-02-20T14:58:10Z

@vfdev-5
Thank you for a lot of advice and improvements !

vfdev-5 · 2022-02-20T15:02:07Z

Hope you enjoyed and learnt while contributing :)

yuta0821 added 2 commits February 14, 2022 13:48

pytorch#2418 replace os.path by pathlib.Path

1978edf

pytorch#2418 replace os.path by pathlib.Path in setup.py

a512b0b

github-actions bot added docs examples Examples module: handlers Core Handlers module labels Feb 14, 2022

vfdev-5 reviewed Feb 14, 2022

View reviewed changes

yuta0821 added 8 commits February 15, 2022 09:07

make dirname as Path object

df6136e

revert to the original code using the os

b3eafb2

revert the code about Path.mkdir and change def dirname to return Pat…

44ce925

…h object

modify the dirname-related code

8dc5bfa

modify the dirname- or tracking- related code

c5b0325

modify the tracking-related code

634ffac

modify the dirname-related code

5e61d03

fix code style

e287db1

yuta0821 and others added 2 commits February 18, 2022 07:48

Merge branch 'master' into feature/#2418_replace_os_path_by_Path

19c0791

autopep8 fix

ad05bfc

vfdev-5 reviewed Feb 17, 2022

View reviewed changes

yuta0821 and others added 3 commits February 18, 2022 12:00

treat as a Path object

f850144

pytorch#2418 treat as a Path object

bf3fc41

autopep8 fix

ba8b07e

vfdev-5 reviewed Feb 18, 2022

View reviewed changes

ignite/handlers/checkpoint.py Outdated Show resolved Hide resolved

ignite/handlers/checkpoint.py Outdated Show resolved Hide resolved

tests/ignite/test_utils.py Outdated Show resolved Hide resolved

yuta0821 added 3 commits February 19, 2022 08:18

pytorch#2418 modify to be match with suggested change

1d599cf

pytorch#2418 modify code format

bd91ff0

Merge branch 'feature/#2417_replace_os_path_by_Path' of https://githu…

34ee1e6

…b.com/yuta0821/ignite into feature/#2418_replace_os_path_by_Path

pytorch#2418 modify some codes to pass the mypy test

47c1ca6