Skip to content

Add __main__ guards to ao tests #154612

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Add __main__ guards to ao tests #154612

wants to merge 2 commits into from

Conversation

AnthonyBarbier
Copy link
Collaborator

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

@AnthonyBarbier AnthonyBarbier requested review from a team, jerryzh168 and HDCharles as code owners May 29, 2025 13:24
@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented May 29, 2025 •
edited
Loading

🔗 Helpful Links

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

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

✅ You can merge normally! (1 Unrelated Failure)

As of commit ce7bb80 with merge base 450180f (image):

UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:

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

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented May 29, 2025 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@colesbury colesbury added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 29, 2025
@AnthonyBarbier AnthonyBarbier force-pushed the anthonyb/guarded_tests_ao branch from 70862cd to 80b10f2 Compare June 2, 2025 17:18
@jcaip
Copy link
Contributor

jcaip commented Jun 4, 2025

cc @AnthonyBarbier thanks for the PR, torch.ao.sparsity is currently being deprecated, I think you should start with another folder first.

@AnthonyBarbier AnthonyBarbier force-pushed the anthonyb/guarded_tests_ao branch from e8d0cbe to ce7bb80 Compare June 5, 2025 08:11
@pytorch-bot pytorch-bot bot added the topic: not user facing topic category label Jun 5, 2025
@AnthonyBarbier
Copy link
Collaborator Author

cc @AnthonyBarbier thanks for the PR, torch.ao.sparsity is currently being deprecated, I think you should start with another folder first.

@jcaip All the other folders have been done already:

#154728
#154715
#154725
#154628
#154716

@AnthonyBarbier
Copy link
Collaborator Author

@jcaip Please review this PR, there is no change in features / coverage, it just fixes the guards so that run_test() in the common code can be fixed in a follow up PR.

jcaip
jcaip reviewed Jun 10, 2025
View reviewed changes
Copy link
Contributor

@jcaip jcaip left a comment

Choose a reason for hiding this comment

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

cc @AnthonyBarbier Changes look fine, but why do we need this? I don't quite follow #134592. Can you explain a bit more what the issue is here?

Are these tests erroring when being run individually - i.e. python test/ao/sparsity/test_activation_sparsifier.py? And you're looking to fix that behavior?

@AnthonyBarbier
Copy link
Collaborator Author

cc @AnthonyBarbier Changes look fine, but why do we need this? I don't quite follow #134592. Can you explain a bit more what the issue is here?

Are these tests erroring when being run individually - i.e. python test/ao/sparsity/test_activation_sparsifier.py? And you're looking to fix that behavior?

The issue is that common_utils is a module with some argparse logic in its global state which means you can't import it without the sys.args getting parsed, etc.

The idea is to have all the tests which rely on this module to handle their main() to explicitly call run_tests() and then in a follow up PR to move the argparse logic from the global scope to the run_tests() function.

I believe when my colleague created the original PR he bumped into some issues where the tests fall in one of 3 categories:

  • in-tree but never executed / disabled (i.e masked out by discover_tests.py)
  • in-tree but executed indirectly: i.e masked out by discover_tests.py but imported in one of the top level test suites
  • in-tree and can be executed directly

The series of PRs is meant to make it clear which way the test is expected to be executed. (And to add the missing run_tests() for the tests which are expected to be run directly)

jcaip
jcaip approved these changes Jun 10, 2025
View reviewed changes
Copy link
Contributor

@jcaip jcaip left a comment

Choose a reason for hiding this comment

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

Gotcha, thanks for explaining. lgtm

@AnthonyBarbier
Copy link
Collaborator Author

@pytorchmergebot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Jun 10, 2025
@pytorchmergebot
Copy link
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

thatgeeman pushed a commit to thatgeeman/pytorch-docathon that referenced this pull request Jun 15, 2025
@AnthonyBarbier @thatgeeman
Add __main__ guards to ao tests (pytorch#154612)
253577d 
This is the first PR of a series in an attempt to get the content of pytorch#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: pytorch#154612
Approved by: https://github.com/jcaip
@AnthonyBarbier AnthonyBarbier mentioned this pull request Jun 24, 2025
Open
pytorchmergebot pushed a commit that referenced this pull request Aug 2, 2025
@AnthonyBarbier @pytorchmergebot
Stop parsing command line arguments every time common_utils is import…
310f901 
…ed. (#156703)

Last PR in the series to re-submit #134592 as smaller PRs:

#154612
#154628
#154715
#154716
#154725
#154728

Pull Request resolved: #156703
Approved by: https://github.com/clee2000
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcaip jcaip jcaip approved these changes

@jerryzh168 jerryzh168 Awaiting requested review from jerryzh168 jerryzh168 is a code owner

@HDCharles HDCharles Awaiting requested review from HDCharles

Assignees
No one assigned
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged open source release notes: AO frontend topic: not user facing topic category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AnthonyBarbier @jcaip @pytorchmergebot @colesbury @pytorchbot