Skip to content

Conversation

@BenjaminBossan
Copy link
Member

@BenjaminBossan BenjaminBossan commented Nov 13, 2025

What does this PR do?

For some time now, loading PEFT adapters directly with transformers is broken when using revisions or subfolders.

To check, run:

RUN_SLOW=1 pytest tests/peft_integration/test_peft_integration.py -k test_peft_from_pretrained_hub_kwargs

This PR makes the PEFT tests pass.

The PR causing this is #41445 (bad commit: 1ee3b288a62c9de658e8be117d869c2a9b835a7c, previous good comit: cad74496ca19c463a5fcc0b35ef4a1c9da2b8c4e). However, that PR also caused other errors (see #41604), which is why this error was not immediately obvious.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

For some time now, loading PEFT adapters directly with transformers is
broken when using revisions or subfolders.

To check, run:

RUN_SLOW=1 pytest tests/peft_integration/test_peft_integration.py -k
test_peft_from_pretrained_hub_kwargs

This PR makes the PEFT tests pass.

The PR causing this is huggingface#41445 (bad commit:
1ee3b28, previous good comit:
cad7449). However, that PR also caused
other errors (see huggingface#41604), which is why this error was not immediately
obvious.
Comment on lines +292 to +293
if token is not None:
adapter_kwargs["token"] = token
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: This change is not directly related to the PR, but I think this is the correct way to handle the token.

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@BenjaminBossan
Copy link
Member Author

AFAICT, the failing tests are unrelated.

Copy link
Contributor

@githubnemo githubnemo left a comment

Choose a reason for hiding this comment

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

Looks good from my side. Thanks for handling this :)

@BenjaminBossan
Copy link
Member Author

ping @ArthurZucker

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

make sure to rebase and we want a test imo! ty for the fix

@BenjaminBossan
Copy link
Member Author

@ArthurZucker The branch is now up-to-date. AFAICT, failing tests are unrelated. IMO there is need for a new test because the old one already covers the mentioned cases, it's just that on main, this test fails but apparently nobody noticed.

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

lgtm 😉

@github-actions
Copy link
Contributor

[For maintainers] Suggested jobs to run (before merge)

run-slow: auto

@BenjaminBossan BenjaminBossan enabled auto-merge (squash) November 14, 2025 15:04
@BenjaminBossan
Copy link
Member Author

@ArthurZucker Hmm, it seems I can't merge because the PR is not up-to-date with main? I merged main twice now, but until CI is finished, it's already out of date. Auto-merge doesn't work, probably because of the red CI. Haaalp!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants