Skip to content
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

Fix HFTrainer overloads #869

Merged
merged 5 commits into from
Oct 28, 2024
Merged

Conversation

kylesayrs
Copy link
Collaborator

Purpose

  • Fix failing tests in tests/llmcompressor/transformers/finetune/test_finetune_no_recipe_custom_dataset.py

Background

  • The HFTrainer function contracts were changed in the most recent transformers release 4.46.0 (offending commit)

Changes

  • Change mixin overloads to use the same function contract as the original functions
  • This change is backwards compatible with earlier versions of transformers since the new arguments are kwargs

Testing

  • tests/llmcompressor/transformers/finetune/test_finetune_no_recipe_custom_dataset.py now passes

Copy link

👋 Hi! Thank you for contributing to llm-compressor. Please add the ready label when the PR is ready for review.

@kylesayrs kylesayrs force-pushed the kylesayrs/fix-trainer-function-contracts branch from 9d51505 to 9a7711e Compare October 27, 2024 17:19
mgoin
mgoin previously approved these changes Oct 27, 2024
src/llmcompressor/transformers/finetune/session_mixin.py Outdated Show resolved Hide resolved
rahul-tuli
rahul-tuli previously approved these changes Oct 27, 2024
@kylesayrs kylesayrs dismissed stale reviews from rahul-tuli and mgoin via 2db27c5 October 28, 2024 04:47
dsikka
dsikka previously approved these changes Oct 28, 2024
rahul-tuli
rahul-tuli previously approved these changes Oct 28, 2024
@rahul-tuli
Copy link
Collaborator

Should we add *args, **kwargs and pass them to the super functions, to protect against future changes like this?

mgoin
mgoin previously approved these changes Oct 28, 2024
src/llmcompressor/transformers/finetune/session_mixin.py Outdated Show resolved Hide resolved
@kylesayrs
Copy link
Collaborator Author

@rahul-tuli Since both functions require some information from the inputs, I think I'd prefer to expose all the arguments. There are tradeoffs either way w.r.t. erroring when arguments change

@kylesayrs kylesayrs dismissed stale reviews from mgoin, rahul-tuli, and dsikka via b6b54fb October 28, 2024 17:14
Signed-off-by: Kyle Sayers <[email protected]>
Signed-off-by: Kyle Sayers <[email protected]>
Signed-off-by: Kyle Sayers <[email protected]>
Signed-off-by: Kyle Sayers <[email protected]>
@kylesayrs kylesayrs force-pushed the kylesayrs/fix-trainer-function-contracts branch from b6b54fb to 84be172 Compare October 28, 2024 17:15
@dsikka dsikka merged commit c0c23b1 into main Oct 28, 2024
6 of 7 checks passed
@dsikka dsikka deleted the kylesayrs/fix-trainer-function-contracts branch October 28, 2024 19:00
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