alignment-handbook icon indicating copy to clipboard operation
alignment-handbook copied to clipboard

Fix BitsAndBytes JSON Serializable

Open deep-diver opened this issue 1 year ago • 4 comments
trafficstars

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().

deep-diver avatar Aug 08 '24 07:08 deep-diver

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?

kashif avatar Aug 12 '24 09:08 kashif

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

deep-diver avatar Aug 12 '24 14:08 deep-diver

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

kashif avatar Aug 14 '24 20:08 kashif

@kashif

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

deep-diver avatar Aug 15 '24 00:08 deep-diver

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.

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 https://github.com/huggingface/alignment-handbook/issues/166.

alvarobartt avatar Aug 19 '24 07:08 alvarobartt

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

lewtun avatar Aug 19 '24 11:08 lewtun

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

deep-diver avatar Aug 20 '24 02:08 deep-diver

Thanks for iterating!

lewtun avatar Aug 20 '24 13:08 lewtun