Generic downstream eval #160

mrudat-iais · 2024-06-24T14:00:16Z

What does this PR do?

We can now convert a modalities model into a HF model. This model can then be evaluated in eval harness.

(main) We now have a convert_pytorch_to_hf_checkpoint endpoint that takes the model_name, model_config_path and output_dir and creates HF files that can then be loaded with AutoModel.from_pretrained
(tests) we have a test that shows the functionality tests/checkpointing/test_checkpoint_conversion.py

General changes

none besides the above
PR feedback addressed and implemented

Breaking Changes

no breaking changes

Checklist before submitting final PR

My PR is minimal and addresses one issue / enhancement in isolation
I have merged main into this feature branch
I have reviewed my own code w.r.t. correct implementation, missing type hints, proper documentation, etc.
I have run a sample config for model training
I have fixed all failing tests (python tests/tests.py)

- Added test for checkpoint conversion.

- In process of adapting HuggingFaceAdapterConfig for loading the saved HF model.

…wnstream_eval

- In process of adapting HuggingFaceAdapterConfig for loading the saved HF model.

- Adapted HuggingFaceAdapterModel for loading the saved HF model.

… HF adapters for eval harness

- fixed end point for checkpoint conversion

…dels - test cases added for gpt2 and mamba - only one single HFAdapter and HFAdapterConfig - added gpt2 config example yaml file for testing

- type hints added - comments in toml deleted

- loading a model with registry is now in models/utils.py - both tests and hf_adapter call util method

src/modalities/checkpointing/checkpoint_conversion.py

le1nux

Really good work and awesome that you guys found a generic solution to it! :-) Thanks a lot for digging deep into the huggingface source code!

I left a few remarks here and there that you should address but overall the solution is great!

src/modalities/__main__.py

src/modalities/checkpointing/checkpoint_conversion.py

src/modalities/models/huggingface_adapters/hf_adapter.py

tests/checkpointing/test_checkpoint_conversion.py

tests/checkpointing/configs_for_testing/mamba_config_test.yaml

le1nux

Really good work and awesome that you guys found a generic solution to it! :-) Thanks a lot for digging deep into the huggingface source code!

I left a few remarks here and there that you should address but overall the solution is great!

- Add missing type hints - consolidate code - refactor tests - shorten test configs

src/modalities/models/huggingface_adapters/hf_adapter.py

src/modalities/models/utils.py

tests/checkpointing/test_checkpoint_conversion.py

le1nux

The PR is almost in a mergable state. I found one potential bug with the PosixPath to string conversion. And some minor things that I think should be addressed before merging. Other than that it's in a great state and we should be able to merge after including the requested changes and another person has reviewed it :-)

- Added type hints in test_checkpoint_conversion.py and hf_adapter.py - Added ConfigError in exceptions.py - Fixed the bug in forward() in hf_adapter.py where the return_dict condition was returning wrong data type. - Converted model_type in utils.py from str to Enum. - Added back the test to check the output of the model before and after conversion.

- Bug fix in convert_posixpath_to_str() for list values. - Added test for convert_posixpath_to_str() - Changed _convert_posixpath_to_str() to convert_posixpath_to_str()

- Added type arguments to the fixture arguments in test_checkpoint_conversion.py - Changed back convert_posixpath_to_str() to _convert_posixpath_to_str()

Added prediction_key argument in the convert_pytorch_to_hf_checkpoint end point

…ities into generic-downstream-eval

Changed this back. It was not a bug. Huggingface requires the output in the particular format when return_dict = True

Fixed the issue where the config of checkpointed model still needs checkpointed_model key and used the non-converted checkpoint in Eval Harness.

src/modalities/__main__.py

src/modalities/checkpointing/checkpoint_conversion.py

src/modalities/models/huggingface_adapters/hf_adapter.py

src/modalities/models/utils.py

tests/models/test_hf_adapter.py

Addressed the PR comments

…nversion

…ities into generic-downstream-eval

rrutmann and others added 20 commits June 10, 2024 13:33

feat(eval): Merge downstream PR into main

6c19c54

feat(checkpoints): we can now convert any loaded model into a HF model

de0e5f2

test(checkpointing):

a29ac2e

- Added test for checkpoint conversion.

Merge remote-tracking branch 'origin/main' into new_downstream_eval

7289fcf

feat(checkpointing):

1742f29

- In process of adapting HuggingFaceAdapterConfig for loading the saved HF model.

Merge remote-tracking branch 'origin/new_downstream_eval' into new_do…

2079a6b

…wnstream_eval

feat(checkpointing):

1bc5152

- In process of adapting HuggingFaceAdapterConfig for loading the saved HF model.

refactor(HuggingFaceModel)

d96f930

feat(checkpointing):

a795160

- Adapted HuggingFaceAdapterModel for loading the saved HF model.

refactor(tests): the checkpoint conversion test is now more readable

06821d6

refactor(tests): the checkpoint conversion test is now more readable

3762bf5

Merge remote-tracking branch 'origin/main' into new_downstream_eval

d77f14e

feat(checkpointing):

a2c855e

- Adapted HuggingFaceAdapterModel for loading the saved HF model.

refactor(hf_adapter): we now have an IF that defines how to implement…

df5cfcc

… HF adapters for eval harness

Merge remote-tracking branch 'origin/main' into new_downstream_eval

05f01e2

fix(test_hf_adapter): made path relative

019b807

fix(checkpointing):

422cac3

- fixed end point for checkpoint conversion

feat(hf_model_adapter): we can now cast models generically into HF mo…

a4e7df0

…dels - test cases added for gpt2 and mamba - only one single HFAdapter and HFAdapterConfig - added gpt2 config example yaml file for testing

refactor(hf_model_adapter): addressed PR feedback

c69089b

- type hints added - comments in toml deleted

refactor(hf_model_adapter): remove duplicate code

e47c7d9

- loading a model with registry is now in models/utils.py - both tests and hf_adapter call util method

mrudat-iais marked this pull request as ready for review June 25, 2024 13:14

mrudat-iais requested a review from le1nux June 25, 2024 13:14

mrudat-iais commented Jun 25, 2024

View reviewed changes

src/modalities/checkpointing/checkpoint_conversion.py Outdated Show resolved Hide resolved

le1nux added the enhancement New feature or request label Jun 26, 2024

le1nux assigned mrudat-iais Jun 26, 2024

le1nux reviewed Jun 26, 2024

View reviewed changes

le1nux requested changes Jun 26, 2024

View reviewed changes

This comment was marked as duplicate.

Sign in to view

feat(DownstreamEval): Address review on PR

4a94287

- Add missing type hints - consolidate code - refactor tests - shorten test configs

le1nux requested changes Jul 3, 2024

View reviewed changes

le1nux reviewed Jul 3, 2024

View reviewed changes

le1nux requested a review from mali-git July 3, 2024 14:56

ajude2s added 3 commits July 4, 2024 12:28

feat(DownstreamEval): Addressed review on PR:

141578d

- Bug fix in convert_posixpath_to_str() for list values. - Added test for convert_posixpath_to_str() - Changed _convert_posixpath_to_str() to convert_posixpath_to_str()

feat(DownstreamEval): Addressed review on PR:

8ccd8e6

- Added type arguments to the fixture arguments in test_checkpoint_conversion.py - Changed back convert_posixpath_to_str() to _convert_posixpath_to_str()

ajude2s self-assigned this Jul 5, 2024

ajude2s and others added 7 commits July 5, 2024 12:37

refactor(DownstreamEval): minor linting changes.

1f21806

refactor(DownstreamEval):

0797ff2

Added prediction_key argument in the convert_pytorch_to_hf_checkpoint end point

test(hf_adapter): Add test for _convert_posixpath_to_str

a8ba245

Merge branch 'generic-downstream-eval' of github.com:Modalities/modal…

2066c14

…ities into generic-downstream-eval

feat(CheckpointConversion): Make prediction_key an endpoint argument

70fe972

fix(DownstreamEval):

7617843

Changed this back. It was not a bug. Huggingface requires the output in the particular format when return_dict = True

fix(DownstreamEval):

5e54875

Fixed the issue where the config of checkpointed model still needs checkpointed_model key and used the non-converted checkpoint in Eval Harness.

mali-git requested changes Jul 8, 2024

View reviewed changes

ajude2s and others added 4 commits July 8, 2024 15:39

refactor(DownstreamEval):

6bc8b79

Addressed the PR comments

refactor: Update mamba configs & additional logging for checkpoint co…

a8b3d24

…nversion

Merge branch 'generic-downstream-eval' of github.com:Modalities/modal…

c835270

…ities into generic-downstream-eval

Merge remote-tracking branch 'origin/main' into generic-downstream-eval

26aedc5

rrutmann merged commit f25c018 into main Jul 9, 2024

rrutmann deleted the generic-downstream-eval branch July 9, 2024 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic downstream eval #160

Generic downstream eval #160

mrudat-iais commented Jun 24, 2024 •

edited

Loading

le1nux left a comment

le1nux left a comment

This comment was marked as duplicate.

le1nux left a comment

Generic downstream eval #160

Generic downstream eval #160

Conversation

mrudat-iais commented Jun 24, 2024 • edited Loading

What does this PR do?

General changes

Breaking Changes

Checklist before submitting final PR

le1nux left a comment

Choose a reason for hiding this comment

le1nux left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

le1nux left a comment

Choose a reason for hiding this comment

mrudat-iais commented Jun 24, 2024 •

edited

Loading