Skip to content

Bump transformers pin #159291

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

Bump transformers pin #159291

wants to merge 12 commits into from
+116 −437

Conversation

anijain2305
Copy link
Contributor

@anijain2305 anijain2305 commented Jul 28, 2025 •
edited by pytorch-bot bot
Loading

@anijain2305 anijain2305 requested a review from a team as a code owner July 28, 2025 19:49
@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Jul 28, 2025 •
edited
Loading

🔗 Helpful Links

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

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

❌ 4 New Failures

As of commit 61fc201 with merge base 334ecbd (image):

NEW FAILURES - The following jobs have failed:

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

@anijain2305 anijain2305 force-pushed the hf_update branch 6 times, most recently from fb506c0 to 1013de5 Compare July 29, 2025 20:49
BoyuanFeng
BoyuanFeng approved these changes Jul 30, 2025
View reviewed changes
benchmarks/dynamo/check_accuracy.py
@@ -14,6 +14,7 @@
"detectron2_maskrcnn_r_101_c4",
"timm_efficientnet", # see https://github.com/pytorch/pytorch/issues/148699
"XGLMForCausalLM", # discovered in https://github.com/pytorch/pytorch/pull/128148
"moondream", # discovered in https://github.com/pytorch/pytorch/pull/159291
Copy link
Contributor

Choose a reason for hiding this comment

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

should we file an issue?

benchmarks/dynamo/torchbench.py
# Real transformers benchmarks will be added soon using a different
# infra.
if model_name.startswith("hf") and hasattr(model.config, "use_cache"):
model.config.use_cache = False
Copy link
Contributor

Choose a reason for hiding this comment

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

what would be the issue if we don't turn off kv cache? We probably want to track a real-world setting for regression..

Copy link
Contributor Author

@anijain2305 anijain2305 Jul 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We should just rely on Angela's transformers models here, instead of trying to work with these old models. There are a few different types of caches in transfomers DynamicCache, StaticCache etc, and its better to rely on the generate API (which Angela uses in the new model benchmarks).

My other extreme suggestion is to just remove these models from the CI, but thats a discussion for another day.

@BoyuanFeng
Copy link
Contributor

Thanks for bumping the transfomers pin! Should we add screenshot of dashboard before/after this pr to the description? So we know the dashboard change from pin update.

@anijain2305 anijain2305 force-pushed the hf_update branch 2 times, most recently from f79f1fe to 07dfd59 Compare July 30, 2025 04:26
@anijain2305 anijain2305 removed request for huydhn and malfet July 30, 2025 04:27
@anijain2305
Copy link
Contributor Author

Converting to draft because the dashboard is red even though CI passes.

@anijain2305 anijain2305 marked this pull request as draft July 30, 2025 04:54
@anijain2305 anijain2305 force-pushed the hf_update branch 4 times, most recently from 97a64f2 to 3302756 Compare July 30, 2025 22:46
@anijain2305 anijain2305 marked this pull request as ready for review July 31, 2025 04:33
@anijain2305 anijain2305 force-pushed the hf_update branch 2 times, most recently from 07a9eae to 4ff8560 Compare July 31, 2025 04:37
@huydhn huydhn deleted the hf_update branch August 8, 2025 03:35
@huydhn huydhn reopened this Aug 8, 2025
@huydhn huydhn requested a review from jeffdaily as a code owner August 8, 2025 03:36
@huydhn
Copy link
Contributor

huydhn commented Aug 8, 2025

(accidentally delete the branch, but I'm re-opening the PR now)

@huydhn
Wrong TorchBench path?
0c36d68 
Signed-off-by: Huy Do <huydhn@gmail.com>
@huydhn
Copy link
Contributor

huydhn commented Aug 8, 2025 •
edited
Loading

The dashboard run is almost done. I will update the rest of the accuracy values tomorrow to get a green PR. Among them, https://github.com/pytorch/pytorch/actions/runs/16821307931/job/47652611212 looks like a real regression but it comes from my change, so I will also push a fix for this in one go.

@anijain2305
Copy link
Contributor Author

@huydhn from the failing tuns, it seems that the new version was not picked up

huydhn added 4 commits August 8, 2025 10:09
@huydhn
Refresh expected values
505c7b3 
Signed-off-by: Huy Do <huydhn@gmail.com>
@huydhn
Update transformers pinned commit, why?
5c6f509 
Signed-off-by: Huy Do <huydhn@gmail.com>
@huydhn
Revert "Refresh expected values"
a5c7844 
This reverts commit 505c7b3.
@huydhn
Revert expected accuracy value updates
cfba76e 
Signed-off-by: Huy Do <huydhn@gmail.com>
huydhn
huydhn reviewed Aug 8, 2025
View reviewed changes
@huydhn huydhn mentioned this pull request Aug 8, 2025
Closed
huydhn added 4 commits August 8, 2025 15:36
@huydhn
Put this back
a794a0d 
Signed-off-by: Huy Do <huydhn@gmail.com>
@huydhn
Merge branch 'main' into hf_update
5ef9532
@huydhn
Chown /opt/conda after installing TorchBench
8878f61 
Signed-off-by: Huy Do <huydhn@gmail.com>
@huydhn
Install soxr (older librosa)
61fc201 
Signed-off-by: Huy Do <huydhn@gmail.com>
@huydhn
Copy link
Contributor

huydhn commented Aug 12, 2025

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Aug 12, 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

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: 1 mandatory check(s) failed. The first few are:

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job

Failing merge rule: Core Maintainers

@anijain2305
Copy link
Contributor Author

@pytorchbot merge -i

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged while ignoring the following 4 checks: pull / linux-jammy-py3.9-gcc11 / test (default, 5, 5, linux.2xlarge), pull / linux-jammy-py3.9-clang12 / test (default, 2, 5, linux.4xlarge), inductor / unit-test / linux-jammy-cpu-py3.9-gcc11-inductor / test (inductor_amx, 1, 2, linux.8xlarge.amx), inductor / unit-test / linux-jammy-cpu-py3.9-gcc11-inductor / test (inductor_avx2, 1, 2, linux.10xlarge.avx2)

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

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

@huydhn huydhn huydhn approved these changes

@BoyuanFeng BoyuanFeng BoyuanFeng approved these changes

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

Assignees
No one assigned
Labels
ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: dynamo module: inductor topic: not user facing topic category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@anijain2305 @BoyuanFeng @huydhn @pytorchmergebot