[BE][Easy] use `pathlib.Path` instead of `dirname` / `".."` / `pardir` #129374

XuehaiPan · 2024-06-24T14:32:16Z

Stack from ghstack (oldest at bottom):

Changes by apply order:

Replace all ".." and os.pardir usage with os.path.dirname(...).
Replace nested os.path.dirname(os.path.dirname(...)) call with str(Path(...).parent.parent).
Reorder .absolute() ~~/ .resolve()~~ and .parent: always resolve the path first.

.parent{...}.absolute() -> .absolute().parent{...}
Replace chained .parent x N with .parents[${N - 1}]: the code is easier to read (see 5.)

.parent.parent.parent.parent -> .parents[3]
~~Replace .parents[${N - 1}] with .parents[${N} - 1]: the code is easier to read and does not introduce any runtime overhead.~~

~~.parents[3] -> .parents[4 - 1]~~
~~Replace .parents[2 - 1] with .parent.parent: because the code is shorter and easier to read.~~

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @ipiszy @yf225 @chenyang78 @kadeng @muchulee8 @ColinPeppler @amjames @desertfire @chauhang @aakhundov @rec @peterbell10

[ghstack-poisoned]

pytorch-bot · 2024-06-24T14:32:19Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/129374

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 8df444f with merge base 9694158 ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[ghstack-poisoned]

ghstack-source-id: 6291baa Pull Request resolved: pytorch#129374

torchgen/shape_functions/gen_jit_shape_functions.py

malfet

Before landing this change, can you please explain why:

You sometime replace parents[1] with .parent.parent
Why parents[x - 1] notation?
Path("/").parents[0] will throw an exception, while Path("/").parent is always safe to call, should this be a considered in some of the cases?

.github/scripts/test_gitutils.py

test/mobile/test_bytecode.py

justinchuby

onnx changes lgtm

pytorchmergebot · 2024-12-21T16:49:44Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

Skylion007 · 2024-12-21T23:48:45Z

FYI. Probably fine in this context but Pathlib can be really slow in performance critical situations compared to string handling.

wdvr · 2024-12-26T17:20:25Z

@pytorchmergebot revert -m "failing internal ROCM builds with error: ModuleNotFoundError: No module named hipify" -c ghfirst

File "/re_cwd/buck-out/v2/gen/fbcode/a82b0712bf2bb755/caffe2/tools/amd_build/build_amd/build_amd#link-tree/build_amd.py", line 13, in
from hipify import hipify_python # type: ignore[import]
ModuleNotFoundError: No module named 'hipify'

pytorchmergebot · 2024-12-26T17:21:57Z

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

pytorchmergebot · 2024-12-26T17:22:08Z

Reverting PR 129374 failed

Reason: Command git -C /home/runner/work/pytorch/pytorch revert --no-edit 2293fe1024812d6349f6e2b3b7de82c6b73f11e4 returned non-zero exit code 1

Auto-merging .github/scripts/delete_old_branches.py
CONFLICT (content): Merge conflict in .github/scripts/delete_old_branches.py
Auto-merging .github/scripts/ensure_actions_will_cancel.py
CONFLICT (content): Merge conflict in .github/scripts/ensure_actions_will_cancel.py
Auto-merging .github/scripts/generate_binary_build_matrix.py
Auto-merging .github/scripts/gitutils.py
CONFLICT (content): Merge conflict in .github/scripts/gitutils.py
Auto-merging .github/scripts/lint_native_functions.py
CONFLICT (content): Merge conflict in .github/scripts/lint_native_functions.py
Auto-merging .github/scripts/test_gitutils.py
Auto-merging test/jit/test_backend_nnapi.py
CONFLICT (content): Merge conflict in test/jit/test_backend_nnapi.py
Auto-merging test/quantization/core/test_docs.py
Auto-merging tools/amd_build/build_amd.py
CONFLICT (content): Merge conflict in tools/amd_build/build_amd.py
Auto-merging tools/code_coverage/package/util/setting.py
CONFLICT (content): Merge conflict in tools/code_coverage/package/util/setting.py
Auto-merging tools/onnx/update_default_opset_version.py
CONFLICT (content): Merge conflict in tools/onnx/update_default_opset_version.py
Auto-merging tools/setup_helpers/generate_code.py
Auto-merging tools/stats/export_test_times.py
CONFLICT (content): Merge conflict in tools/stats/export_test_times.py
Auto-merging tools/stats/import_test_stats.py
CONFLICT (content): Merge conflict in tools/stats/import_test_stats.py
Auto-merging tools/test/heuristics/test_heuristics.py
CONFLICT (content): Merge conflict in tools/test/heuristics/test_heuristics.py
Auto-merging tools/test/heuristics/test_interface.py
CONFLICT (content): Merge conflict in tools/test/heuristics/test_interface.py
Auto-merging tools/test/heuristics/test_utils.py
CONFLICT (content): Merge conflict in tools/test/heuristics/test_utils.py
Auto-merging tools/test/test_test_run.py
CONFLICT (content): Merge conflict in tools/test/test_test_run.py
Auto-merging tools/test/test_test_selections.py
CONFLICT (content): Merge conflict in tools/test/test_test_selections.py
Auto-merging tools/test/test_upload_stats_lib.py
CONFLICT (content): Merge conflict in tools/test/test_upload_stats_lib.py
Auto-merging tools/testing/discover_tests.py
CONFLICT (content): Merge conflict in tools/testing/discover_tests.py
Auto-merging tools/testing/do_target_determination_for_s3.py
CONFLICT (content): Merge conflict in tools/testing/do_target_determination_for_s3.py
Auto-merging tools/testing/explicit_ci_jobs.py
CONFLICT (content): Merge conflict in tools/testing/explicit_ci_jobs.py
Auto-merging tools/testing/modulefinder_determinator.py
CONFLICT (content): Merge conflict in tools/testing/modulefinder_determinator.py
Auto-merging tools/testing/target_determination/gen_artifact.py
CONFLICT (content): Merge conflict in tools/testing/target_determination/gen_artifact.py
Auto-merging tools/testing/target_determination/heuristics/filepath.py
CONFLICT (content): Merge conflict in tools/testing/target_determination/heuristics/filepath.py
Auto-merging tools/testing/target_determination/heuristics/llm.py
CONFLICT (content): Merge conflict in tools/testing/target_determination/heuristics/llm.py
Auto-merging tools/testing/target_determination/heuristics/previously_failed_in_pr.py
CONFLICT (content): Merge conflict in tools/testing/target_determination/heuristics/previously_failed_in_pr.py
Auto-merging tools/testing/target_determination/heuristics/utils.py
CONFLICT (content): Merge conflict in tools/testing/target_determination/heuristics/utils.py
Auto-merging tools/testing/test_selections.py
CONFLICT (content): Merge conflict in tools/testing/test_selections.py
Auto-merging torch/testing/_internal/common_utils.py
CONFLICT (content): Merge conflict in torch/testing/_internal/common_utils.py
error: could not revert 2293fe10248... [BE][Easy] use `pathlib.Path` instead of `dirname` / `".."` / `pardir` (#129374)
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git revert --continue".
hint: You can instead skip this commit with "git revert --skip".
hint: To abort and get back to the state before "git revert",
hint: run "git revert --abort".
hint: Disable this message with "git config advice.mergeConflict false"

Details for Dev Infra team

Raised by workflow job

)" This reverts commit 135c7db. Reverted #129409 on behalf of https://github.com/malfet due to need to revert to as dependency of #129374 ([comment](#129409 (comment)))

Changes: 1. Always explicit `.absolute()`: `Path(__file__)` -> `Path(__file__).absolute()` 2. Replace `path.resolve()` with `path.absolute()` if the code is resolving the PyTorch repo root directory. Pull Request resolved: #129409 Approved by: https://github.com/albanD

malfet · 2024-12-26T17:30:49Z

@pytorchmergebot revert -m "failing internal ROCM builds with error: ModuleNotFoundError: No module named hipify" -c ghfirst

pytorchmergebot · 2024-12-26T17:32:14Z

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

pytorchmergebot · 2024-12-26T17:32:25Z

@XuehaiPan your PR has been successfully reverted.

… `pardir` (#129374)" This reverts commit 2293fe1. Reverted #129374 on behalf of https://github.com/malfet due to failing internal ROCM builds with error: ModuleNotFoundError: No module named hipify ([comment](#129374 (comment)))

malfet · 2024-12-26T17:34:20Z

tools/amd_build/build_amd.py

+REPO_ROOT = Path(__file__).absolute().parents[2]
+sys.path.append(str((REPO_ROOT / "torch" / "utils").resolve()))


I vaguely remember talking about it in the past: it will resolve to a different folder if __file__ is a symlink to some other place. I.e. imagine a folder structure like (which is used by build system that bazel/buck, but could be the case of many other builds):

- actual_files - build_amd.py - hipify.py - deploy - tools - amd_build - build_amd.py -> ../../../build_amd.py - torch - utils - hipify.py -> ../../../hipify.py

In the previous version of the logic, it will resolve path correctly, but after the reorder it will point to non-existing /torch/utils/ folder

Alright, today I have learned os.path.join(file, os.pardir) may have different behavior of os.path.dirname(file).

If file is a regular file, both paths resolve to the same path.

If file is a symlink, resolve will treat the "file" as a directory.

import os from pathlib import Path REPO_ROOT = Path(__file__).absolute().parents[2] print(__file__) print(os.path.realpath(os.path.join(os.path.dirname(__file__), os.pardir, os.pardir, 'torch', 'utils'))) print(os.path.realpath(os.path.join(__file__, os.pardir, os.pardir, os.pardir, 'torch', 'utils'))) print((REPO_ROOT / "torch" / "utils").resolve())

The output is:

/Users/PanXuehai/Projects/pytorch/tools/amd_build/test.py /Users/PanXuehai/Projects/pytorch/torch/utils /Users/PanXuehai/Projects/torch/utils /Users/PanXuehai/Projects/pytorch/torch/utils

[ghstack-poisoned]

XuehaiPan · 2024-12-27T12:28:05Z

tools/amd_build/build_amd.py

+# NOTE: `tools/amd_build/build_amd.py` could be a symlink.
+# The behavior of `symlink / '..'` is different from `symlink.parent`.
+# Use `pardir` three times rather than using `path.parents[2]`.
+REPO_ROOT = (
+    Path(__file__).absolute() / os.path.pardir / os.path.pardir / os.path.pardir
+).resolve()
+sys.path.append(str(REPO_ROOT / "torch" / "utils"))


I have updated the patch and added a note for it.

pytorch#129374) Changes by apply order: 1. Replace all `".."` and `os.pardir` usage with `os.path.dirname(...)`. 2. Replace nested `os.path.dirname(os.path.dirname(...))` call with `str(Path(...).parent.parent)`. 3. Reorder `.absolute()` ~/ `.resolve()`~ and `.parent`: always resolve the path first. `.parent{...}.absolute()` -> `.absolute().parent{...}` 4. Replace chained `.parent x N` with `.parents[${N - 1}]`: the code is easier to read (see 5.) `.parent.parent.parent.parent` -> `.parents[3]` 5. ~Replace `.parents[${N - 1}]` with `.parents[${N} - 1]`: the code is easier to read and does not introduce any runtime overhead.~ ~`.parents[3]` -> `.parents[4 - 1]`~ 6. ~Replace `.parents[2 - 1]` with `.parent.parent`: because the code is shorter and easier to read.~ Pull Request resolved: pytorch#129374 Approved by: https://github.com/justinchuby, https://github.com/malfet ghstack-source-id: bc880ec

XuehaiPan · 2024-12-29T17:15:52Z

@pytorchbot merge

pytorchmergebot · 2024-12-29T17:17:33Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

pytorch#129374) Changes by apply order: 1. Replace all `".."` and `os.pardir` usage with `os.path.dirname(...)`. 2. Replace nested `os.path.dirname(os.path.dirname(...))` call with `str(Path(...).parent.parent)`. 3. Reorder `.absolute()` ~/ `.resolve()`~ and `.parent`: always resolve the path first. `.parent{...}.absolute()` -> `.absolute().parent{...}` 4. Replace chained `.parent x N` with `.parents[${N - 1}]`: the code is easier to read (see 5.) `.parent.parent.parent.parent` -> `.parents[3]` 5. ~Replace `.parents[${N - 1}]` with `.parents[${N} - 1]`: the code is easier to read and does not introduce any runtime overhead.~ ~`.parents[3]` -> `.parents[4 - 1]`~ 6. ~Replace `.parents[2 - 1]` with `.parent.parent`: because the code is shorter and easier to read.~ Pull Request resolved: pytorch#129374 Approved by: https://github.com/justinchuby, https://github.com/malfet

Update

b87fc29

[ghstack-poisoned]

XuehaiPan requested review from a team, andrewkho, gokulavasan, jeffdaily, jerryzh168, jithunnair-amd, justinchuby, shubhambhokare1, titaiwangms and wschin as code owners June 24, 2024 14:32

pytorch-bot bot added ciflow/inductor module: dynamo module: inductor release notes: quantization release notes category release notes: releng release notes category labels Jun 24, 2024

This was referenced Jun 24, 2024

[BE][Easy] let usort recognize yaml as stdlib #129373

Closed

[BE][Easy] enable postponed annotations in tools #129375

Closed

[BE][Easy] enable postponed annotations in torchgen #129376

Closed

XuehaiPan added better-engineering Relatively self-contained tasks for better engineering contributors topic: not user facing topic category labels Jun 24, 2024

XuehaiPan changed the title ~~[BE][Easy] use pathlib.Path in tools and torchgen~~ [BE][Easy] use pathlib.Path instead of dirname / ".." / pardir Jun 24, 2024

Update

2b24617

[ghstack-poisoned]

XuehaiPan added a commit to XuehaiPan/pytorch that referenced this pull request Jun 24, 2024

[BE][Easy] use pathlib.Path in tools and torchgen

17663eb

ghstack-source-id: 6291baa Pull Request resolved: pytorch#129374

pytorchbot added the open source label Jun 24, 2024

malfet reviewed Jun 24, 2024

View reviewed changes

torchgen/shape_functions/gen_jit_shape_functions.py Outdated Show resolved Hide resolved

malfet reviewed Jun 24, 2024

View reviewed changes

.github/scripts/test_gitutils.py Show resolved Hide resolved

test/mobile/test_bytecode.py Outdated Show resolved Hide resolved

justinchuby approved these changes Jun 24, 2024

View reviewed changes

pytorchmergebot closed this in 2293fe1 Dec 21, 2024

pytorchmergebot removed the merging label Dec 21, 2024

pytorchmergebot added the ci-no-td Do not run TD on this PR label Dec 26, 2024

pytorchmergebot reopened this Dec 26, 2024

malfet reviewed Dec 26, 2024

View reviewed changes

wdvr mentioned this pull request Dec 26, 2024

stack revert does not work pytorch/test-infra#6115

Closed

XuehaiPan added 2 commits December 27, 2024 20:15

Update

a499c58

[ghstack-poisoned]

Update

8df444f

[ghstack-poisoned]

XuehaiPan commented Dec 27, 2024

View reviewed changes

pytorchmergebot added the merging label Dec 29, 2024

pytorchmergebot closed this in b6bdb67 Dec 29, 2024

pytorchmergebot removed the merging label Dec 29, 2024

github-actions bot deleted the gh/XuehaiPan/67/head branch January 29, 2025 02:02

		REPO_ROOT = Path(__file__).absolute().parents[2]
		sys.path.append(str((REPO_ROOT / "torch" / "utils").resolve()))

[BE][Easy] use pathlib.Path instead of dirname / ".." / pardir #129374

[BE][Easy] use pathlib.Path instead of dirname / ".." / pardir #129374

Uh oh!

Conversation

XuehaiPan commented Jun 24, 2024 • edited by pytorch-bot bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/129374

✅ No Failures

Uh oh!

Uh oh!

malfet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

justinchuby left a comment

Choose a reason for hiding this comment

Uh oh!

pytorchmergebot commented Dec 21, 2024

Merge started

Uh oh!

Skylion007 commented Dec 21, 2024

Uh oh!

wdvr commented Dec 26, 2024

Uh oh!

pytorchmergebot commented Dec 26, 2024

Uh oh!

pytorchmergebot commented Dec 26, 2024

Reverting PR 129374 failed

Uh oh!

malfet commented Dec 26, 2024

Uh oh!

pytorchmergebot commented Dec 26, 2024

Uh oh!

pytorchmergebot commented Dec 26, 2024

Uh oh!

malfet Dec 26, 2024

Choose a reason for hiding this comment

Uh oh!

XuehaiPan Dec 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XuehaiPan Dec 27, 2024

Choose a reason for hiding this comment

Uh oh!

XuehaiPan commented Dec 29, 2024

Uh oh!

pytorchmergebot commented Dec 29, 2024

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

[BE][Easy] use `pathlib.Path` instead of `dirname` / `".."` / `pardir` #129374

[BE][Easy] use `pathlib.Path` instead of `dirname` / `".."` / `pardir` #129374

XuehaiPan commented Jun 24, 2024 •

edited by pytorch-bot bot

Loading

pytorch-bot bot commented Jun 24, 2024 •

edited

Loading

XuehaiPan Dec 27, 2024 •

edited

Loading