Skip to content

Correctly use test parameters #166726

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 4 commits into from
Closed

Correctly use test parameters #166726

wants to merge 4 commits into from
+6 −7

Conversation

@cyyever
Copy link
Collaborator

@cyyever cyyever commented Oct 31, 2025 •
edited
Loading

This PR uses unused arguments in some tests.

cc @H-Huang @awgu @wanchaol @fegin @fduwjj @wz337 @wconstab @d4l3k @pragupta @msaroufim @dcci

@pytorch-bot pytorch-bot bot added oncall: distributed Add this issue/PR to distributed oncall triage queue topic: not user facing topic category labels Oct 31, 2025
@pytorch-bot
Copy link

pytorch-bot bot commented Oct 31, 2025 •
edited
Loading

🔗 Helpful Links

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

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

⏳ No Failures, 12 Pending

As of commit 930afe2 with merge base f91899c (image):
💚 Looks good so far! There are no failures yet. 💚

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

@cyyever cyyever changed the title Pass parameters in tests Pass arguments in tests Oct 31, 2025
@cyyever cyyever changed the title Pass arguments in tests Correctly use test parameters Oct 31, 2025
@cyyever cyyever requested review from Lucaskabela, albanD, ezyang, peterbell10 and rec and removed request for peterbell10 October 31, 2025 14:33
@cyyever cyyever requested a review from jerryzh168 as a code owner October 31, 2025 14:39
@pytorch-bot pytorch-bot bot added the release notes: quantization release notes category label Oct 31, 2025
rec
rec approved these changes Oct 31, 2025
View reviewed changes
Copy link
Collaborator

@rec rec left a comment

Choose a reason for hiding this comment

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

Good stuff! This isn't just cosmetic, in two tests it is actually covering more and doing what it was obviously intended to do.

test/distributed/fsdp/test_fsdp_misc.py
ref_model = DDP(deepcopy(model), process_group=gloo_pg)
model = FSDP(
model,
sharding_strategy=ss,
Copy link
Collaborator

@rec rec Oct 31, 2025

Choose a reason for hiding this comment

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

Nice catch!

albanD
albanD approved these changes Oct 31, 2025
View reviewed changes
Copy link
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.

Thx!

@Skylion007
Copy link
Collaborator

Skylion007 commented Oct 31, 2025

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 31, 2025
@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Oct 31, 2025

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

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Oct 31, 2025

Merge failed

Reason: 1 jobs have failed, first few of them are: trunk / linux-jammy-cuda12.8-py3.10-gcc11 / test (distributed, 1, 3, linux.g4dn.12xlarge.nvidia.gpu)

Details for Dev Infra team Raised by workflow job

@cyyever
Pass parameters in tests
1e651cb 
Signed-off-by: Yuanyuan Chen <cyyever@outlook.com>
@cyyever
Correctly use test parameters
ceb4d4f 
Signed-off-by: Yuanyuan Chen <cyyever@outlook.com>
@cyyever
Fix test
59a5b5c 
Signed-off-by: Yuanyuan Chen <cyyever@outlook.com>
@cyyever
Skip unsuppored ShardingStrategy.HYBRID_SHARD
930afe2 
Signed-off-by: Yuanyuan Chen <cyyever@outlook.com>
@cyyever
Copy link
Collaborator Author

cyyever commented Nov 1, 2025

@pytorchmergebot merge

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Nov 1, 2025

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

@cyyever
Copy link
Collaborator Author

cyyever commented Nov 1, 2025

@pytorchmergebot merge -f "CI was green"

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Nov 1, 2025

The merge job was canceled or timed out. This most often happen if two merge requests were issued for the same PR, or if merge job was waiting for more than 6 hours for tests to finish. In later case, please do not hesitate to reissue the merge command
For more information see pytorch-bot wiki.

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Nov 1, 2025

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

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

@cyyever cyyever deleted the more_fix branch November 1, 2025 04:46
etaf pushed a commit to etaf/pytorch-inductor-xpu that referenced this pull request Nov 4, 2025
@cyyever @etaf
Correctly use test parameters (pytorch#166726)
13a8a57 
This PR uses unused arguments in some tests.

Pull Request resolved: pytorch#166726
Approved by: https://github.com/rec, https://github.com/albanD, https://github.com/Skylion007
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rec rec rec approved these changes

@Skylion007 Skylion007 Skylion007 approved these changes

@albanD albanD albanD approved these changes

@peterbell10 peterbell10 Awaiting requested review from peterbell10

@ezyang ezyang Awaiting requested review from ezyang

@Lucaskabela Lucaskabela Awaiting requested review from Lucaskabela

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

Assignees

No one assigned

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged oncall: distributed Add this issue/PR to distributed oncall triage queue open source release notes: quantization release notes category topic: not user facing topic category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@cyyever @Skylion007 @pytorchmergebot @rec @albanD @pytorchbot