Skip to content

Avoid args being parsed when common_utils imported #134592

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

nicholasw-gc
Copy link
Contributor

@nicholasw-gc nicholasw-gc commented Aug 27, 2024

  • Change the parsing of args using argparse to only occur a file explicitly calls run_tests. This means that other modules importing common_utils can use argparse without inteference
  • Add and use a common raise_on_run_directly method for when a user runs a test file directly which should not be run this way. Print the file which the user should have run.
  • Ensure all files which should call run_tests do call run_tests.
  • Raise a RuntimeError on tests which have been disabled (not run)
  • Remove any remaining uses of "unittest.main()""

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 @amjames @chauhang @aakhundov @XilunWu @ColinPeppler @desertfire

@nicholasw-gc nicholasw-gc requested review from a team, jerryzh168 and HDCharles as code owners August 27, 2024 17:01
Copy link

pytorch-bot bot commented Aug 27, 2024

🔗 Helpful Links

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

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

❌ 56 New Failures, 2 Unrelated Failures

As of commit 2c6bb82 with merge base 0431d47 (image):

NEW FAILURES - The following jobs have failed:

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

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

@pytorch-bot pytorch-bot bot added module: inductor oncall: distributed Add this issue/PR to distributed oncall triage queue release notes: quantization release notes category release notes: AO frontend labels Aug 27, 2024
@janeyx99 janeyx99 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Aug 30, 2024
@nicholasw-gc
Copy link
Contributor Author

@jerryzh168 @HDCharles pinging as it's been 5 working days though I recognise it's close to holiday season and the diff is huge. Would be good allow workflow approval to run more tests over weekend.

@nicholasw-gc
Copy link
Contributor Author

@jerryzh168 @HDCharles pinging again: if this is good in principle, I can resolve conflicts.

@@ -821,3 +822,4 @@ def dw_runner():
nprocs=world_size,
args=(world_size, rdvz_file),
)
run_tests()
Copy link
Contributor

@clee2000 clee2000 Oct 8, 2024

Choose a reason for hiding this comment

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

I'm not sure this should be here or in the other distributed files, cc @wconstab

Other than that I think it looks ok, but I think CI will be the final judge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My only goal here was consistency. I make 127 instances of run_tests() in test/distributed but happy to erase them. Or would it just be the pipelining subdir where there are 6?. @wconstab

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you reckon for this?

@clee2000 clee2000 added the ci-no-td Do not run TD on this PR label Oct 8, 2024
@nicholasw-gc nicholasw-gc force-pushed the PYT-117 branch 2 times, most recently from b6becac to 49c8c3f Compare October 15, 2024 12:54
@nicholasw-gc
Copy link
Contributor Author

@jerryzh168 @HDCharles @wconstab Are you able to review?

@nicholasw-gc
Copy link
Contributor Author

@jerryzh168 @HDCharles @wconstab Are you able to review?

@jerryzh168 pinging as it looks like you're reviewing PRs today?

@@ -286,3 +286,10 @@ def packed_params_data_with_int32_indices(data_as_state_and_weight_bias):
packed_params_data_with_int32_indices(packed_params_data_2a),
packed_params_data_with_int32_indices(packed_params_data_2b),
)


if __name__ == "__main__":
Copy link
Contributor

Choose a reason for hiding this comment

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

why not enable this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enabled now, will see what happens!

Copy link
Contributor

@jerryzh168 jerryzh168 left a comment

Choose a reason for hiding this comment

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

thanks! quantization and sparsity changes LGTM

@nicholasw-gc
Copy link
Contributor Author

@jerryzh168 @HDCharles @wconstab
Please can I get workflows approved and review. Many thanks!

- Change the parsing of args using argparse to only occur when a file
  explicitly calls run_tests. This means that other modules importing
  common_utils can use argparse without inteference
- Add and use a common raise_on_run_directly method for when a user runs
  a test file directly which should not be run this way. Print the file
  which the user should have run.
- Ensure all files which should call run_tests do call run_tests.
- Raise a RuntimeError on tests which have been disabled (not run)
- Remove any remaining uses of "unittest.main()""
- Enable "test_qlinear_packed_params.py"
@nicholasw-gc
Copy link
Contributor Author

@jerryzh168 @HDCharles @wconstab
Happy New Year! I have rebased onto the latest main. Any chance of a quick review as it will likely need rebasing before long? Many thanks!

@jerryzh168
Copy link
Contributor

changes to quantization test files LGTM, please ask @clee2000 and @wconstab for reviews of the other files

Copy link
Contributor

github-actions bot commented Mar 4, 2025

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

AnthonyBarbier added a commit to graphcore/pytorch-fork that referenced this pull request Jun 2, 2025
This PR is part of a series attempting to re-submit pytorch#134592 as smaller PRs.

In fx tests:

- Add and use a common raise_on_run_directly method for when a user runs a test file directly which should not be run this way. Print the file which the user should have run.
- Raise a RuntimeError on tests which have been disabled (not run)
- Remove any remaining uses of "unittest.main()""
AnthonyBarbier added a commit to graphcore/pytorch-fork that referenced this pull request Jun 2, 2025
This is the first PR of a series in an attempt to re-submit pytorch#134592 as smaller PRs.
pytorchmergebot pushed a commit that referenced this pull request Jun 4, 2025
This PR is part of a series attempting to re-submit #134592 as smaller PRs.

Add missing `if __name__ == "__main__":` guards to some tests.

Pull Request resolved: #154716
Approved by: https://github.com/Skylion007
pytorchmergebot pushed a commit that referenced this pull request Jun 4, 2025
This PR is part of a series attempting to re-submit #134592 as smaller PRs.

In fx tests:

- Add and use a common raise_on_run_directly method for when a user runs a test file directly which should not be run this way. Print the file which the user should have run.
- Raise a RuntimeError on tests which have been disabled (not run)
- Remove any remaining uses of "unittest.main()""

Pull Request resolved: #154715
Approved by: https://github.com/Skylion007
pytorchmergebot pushed a commit that referenced this pull request Jun 4, 2025
This is the first PR of a series in an attempt to re-submit #134592 as smaller PRs.

In distributed tests:

- Ensure all files which should call run_tests do call run_tests.
- Raise a RuntimeError on tests which have been disabled (not run)
- Remove any remaining uses of "unittest.main()""

Cc @wconstab @clee2000

Pull Request resolved: #154628
Approved by: https://github.com/Skylion007
pytorchmergebot pushed a commit that referenced this pull request Jun 4, 2025
This PR is part of a series attempting to re-submit #134592 as smaller PRs.

In jit tests:

- Add and use a common raise_on_run_directly method for when a user runs a test file directly which should not be run this way. Print the file which the user should have run.
- Raise a RuntimeError on tests which have been disabled (not run)

Pull Request resolved: #154725
Approved by: https://github.com/Skylion007
angelayi pushed a commit to angelayi/pytorch that referenced this pull request Jun 5, 2025
This PR is part of a series attempting to re-submit pytorch#134592 as smaller PRs.

Add missing `if __name__ == "__main__":` guards to some tests.

Pull Request resolved: pytorch#154716
Approved by: https://github.com/Skylion007
angelayi pushed a commit to angelayi/pytorch that referenced this pull request Jun 5, 2025
This PR is part of a series attempting to re-submit pytorch#134592 as smaller PRs.

In fx tests:

- Add and use a common raise_on_run_directly method for when a user runs a test file directly which should not be run this way. Print the file which the user should have run.
- Raise a RuntimeError on tests which have been disabled (not run)
- Remove any remaining uses of "unittest.main()""

Pull Request resolved: pytorch#154715
Approved by: https://github.com/Skylion007
angelayi pushed a commit to angelayi/pytorch that referenced this pull request Jun 5, 2025
This is the first PR of a series in an attempt to re-submit pytorch#134592 as smaller PRs.

In distributed tests:

- Ensure all files which should call run_tests do call run_tests.
- Raise a RuntimeError on tests which have been disabled (not run)
- Remove any remaining uses of "unittest.main()""

Cc @wconstab @clee2000

Pull Request resolved: pytorch#154628
Approved by: https://github.com/Skylion007
angelayi pushed a commit to angelayi/pytorch that referenced this pull request Jun 5, 2025
This PR is part of a series attempting to re-submit pytorch#134592 as smaller PRs.

In jit tests:

- Add and use a common raise_on_run_directly method for when a user runs a test file directly which should not be run this way. Print the file which the user should have run.
- Raise a RuntimeError on tests which have been disabled (not run)

Pull Request resolved: pytorch#154725
Approved by: https://github.com/Skylion007
pytorchmergebot pushed a commit that referenced this pull request Jun 10, 2025
This is the first PR of a series in an attempt to get the content of #134592 merged as smaller PRs (Given that the original one was closed due to a lack of reviewers).

This specific PR contains:
- Add and use a common raise_on_run_directly method for when a user runs a test file directly which should not be run this way. Print the file which the user should have run.
- Update ao tests.

There will be follow up PRs to update the other test suites but I don't have permissions to create branches directly on pytorch/pytorch so I can't create a stack and therefore will have to create them one at the time.

Cc @jerryzh168
Pull Request resolved: #154612
Approved by: https://github.com/jcaip
pytorchmergebot pushed a commit that referenced this pull request Jun 10, 2025
This PR is part of a series attempting to re-submit #134592 as smaller PRs.

In quantization tests:

- Add and use a common raise_on_run_directly method for when a user runs a test file directly which should not be run this way. Print the file which the user should have run.
- Raise a RuntimeError on tests which have been disabled (not run)

Pull Request resolved: #154728
Approved by: https://github.com/ezyang
pytorchmergebot pushed a commit that referenced this pull request Jun 16, 2025
This PR is part of a series attempting to re-submit #134592 as smaller PRs.

In jit tests:

- Add and use a common raise_on_run_directly method for when a user runs a test file directly which should not be run this way. Print the file which the user should have run.
- Raise a RuntimeError on tests which have been disabled (not run)

Pull Request resolved: #154725
Approved by: https://github.com/clee2000
pytorchmergebot pushed a commit that referenced this pull request Aug 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-no-td Do not run TD on this PR ciflow/inductor module: inductor oncall: distributed Add this issue/PR to distributed oncall triage queue open source release notes: AO frontend release notes: quantization release notes category Stale triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants