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 BitsAndBytes JSON Serializable #191

Merged
merged 9 commits into from
Aug 20, 2024

Conversation

deep-diver
Copy link
Contributor

This PR is related to #189

Current transformers tries to convert every config objects into Dictionary(link), but it does not resolve in nested config case (BitsAndBytesConfig) with the following error. This is because BitsAndBytesConfig is stored under quantization_config of the training args:

TypeError: Object of type BitsAndBytesConfig is not JSON serializable

caused by [this(https://github.com/huggingface/transformers/blob/e0d82534cc95b582ab072c1bbc060852ba7f9d51/src/transformers/training_args.py#L2446)].

The easiest solution for this is to call to_dict() method after get_quantization_config().

@kashif
Copy link
Contributor

kashif commented Aug 12, 2024

thanks @deep-diver do you think it makes more sense to change the get_quantization_config method to return a dict? Then one would just need to update the tests for this function and it could be a cleaner way?

scripts/run_sft.py Outdated Show resolved Hide resolved
@deep-diver
Copy link
Contributor Author

@kashif and @alvarobartt, Thanks for the comments.
scripts are not provided as a part of alignment-handbook package while other parts are. By changing the internal code base of the package could break other applications' behaviour. So, I made a small change inside the run_sft.py.

That is how I perceived about this. But, I am OK with you guys' proposes too.

@kashif
Copy link
Contributor

kashif commented Aug 14, 2024

as we tag the lib. with versions perhaps with this change we can tag it to a new version... so lets update the API to return dicts

@deep-diver
Copy link
Contributor Author

@kashif

removed to_dict() call from the scripts, and add to_dict() in get_quantization_config()`

@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.

@kashif kashif requested a review from alvarobartt August 18, 2024 09:37
@kashif kashif self-requested a review August 19, 2024 07:08
@alvarobartt
Copy link
Member

Hi here @deep-diver thanks for this PR! Indeed there's a broken unit test:

tests/test_model_utils.py::GetQuantizationConfigTest::test_8bit - AttributeError: 'dict' object has no attribute 'load_in_8bit'

If you could fix that before merging that would be great! I can create a PR to fix the rest of the unit tests as one's already reported at #166.

Copy link
Member

@lewtun lewtun left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! I'll merge once I get the PR green in #196

@lewtun
Copy link
Member

lewtun commented Aug 19, 2024

Ah the test is failing because it previously assumed quantization_config was an object. Can you fix the test please and then we merge?

@deep-diver
Copy link
Contributor Author

Tested out and made sure everything works ok.
@lewtun @alvarobartt

@lewtun lewtun merged commit 27f7dbf into huggingface:main Aug 20, 2024
3 checks passed
@lewtun
Copy link
Member

lewtun commented Aug 20, 2024

Thanks for iterating!

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.

5 participants