Skip to content

Use absolute path path.resolve() -> path.absolute()#129409

Closed
XuehaiPan wants to merge 32 commits intogh/XuehaiPan/70/basefrom
gh/XuehaiPan/70/head
Closed

Use absolute path path.resolve() -> path.absolute()#129409
XuehaiPan wants to merge 32 commits intogh/XuehaiPan/70/basefrom
gh/XuehaiPan/70/head

Conversation

@XuehaiPan
Copy link
Copy Markdown
Collaborator

@XuehaiPan XuehaiPan commented Jun 24, 2024

Stack from ghstack (oldest at bottom):

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.

cc @H-Huang @awgu @kwen2501 @wanchaol @fegin @fduwjj @wz337 @wconstab @d4l3k @c-p-i-o @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @ipiszy @yf225 @chenyang78 @kadeng @muchulee8 @ColinPeppler @amjames @desertfire @chauhang @aakhundov @peterbell10

[ghstack-poisoned]
@pytorch-bot
Copy link
Copy Markdown

pytorch-bot bot commented Jun 24, 2024

🔗 Helpful Links

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

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

✅ No Failures

As of commit 01870ec with merge base e141cb9 (image):
💚 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-poisoned]
[ghstack-poisoned]
@pytorch-bot pytorch-bot bot added module: dynamo oncall: distributed Add this issue/PR to distributed oncall triage queue labels Dec 22, 2024
[ghstack-poisoned]
@XuehaiPan XuehaiPan requested a review from albanD as a code owner December 22, 2024 22:05
XuehaiPan added a commit that referenced this pull request Dec 22, 2024
ghstack-source-id: 2813aa2
Pull Request resolved: #129409
Copy link
Copy Markdown
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit but SGTM once they're fixed

"Union",
"defaultdict"
"defaultdict",
"Path"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add new entries to this file is not ok. We should fix the callsite.
Given the particular folder, we can most likely just add a __all__ to the file.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@XuehaiPan this was not resolved ?

Copy link
Copy Markdown
Collaborator Author

@XuehaiPan XuehaiPan Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The torch.utils.data.datapipes.gen_pyi module does not intend to export anything. The __all__ list should be empty. Then we will have a longer public allow list in the json file if we set an empty __all__ list.

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@XuehaiPan
Copy link
Copy Markdown
Collaborator Author

@pytorchbot merge

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

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

@malfet
Copy link
Copy Markdown
Contributor

malfet commented Dec 26, 2024

@pytorchbot revert -m "need to revert to as dependency of #129374" -c nosignal

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

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

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

@XuehaiPan your PR has been successfully reverted.

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@XuehaiPan
Copy link
Copy Markdown
Collaborator Author

@pytorchbot merge

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

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

@jeanschmidt
Copy link
Copy Markdown
Contributor

@pytorchbot revert -m "Breaking internal CI, @albanD please help get this PR merged" -c ghfirst

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

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

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

@XuehaiPan your PR has been successfully reverted.


sys.path.insert(0, str(REPO_ROOT))

from tools.stats.import_test_stats import get_disabled_tests
Copy link
Copy Markdown
Contributor

@atalman atalman Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like internal failure on this line:
ModuleNotFoundError: No module named 'tools'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the path insert above doesn't work anymore.
cc @eellison why is this needed? We really shouldn't play with python path to find tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

better-engineering Relatively self-contained tasks for better engineering contributors ci-no-td Do not run TD on this PR ciflow/inductor ciflow/rocm Trigger "default" config CI on ROCm ciflow/trunk Trigger trunk jobs on your pull request Merged module: dynamo module: inductor oncall: distributed Add this issue/PR to distributed oncall triage queue open source release notes: quantization release notes category release notes: releng release notes category Reverted Stale topic: not user facing topic category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants