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

Add config_utils tests #262

Merged
merged 5 commits into from
Jul 30, 2024

Conversation

aluu317
Copy link
Collaborator

@aluu317 aluu317 commented Jul 24, 2024

Description of the change

Add unit tests for the config_utils with 100% test coverage

Screenshot 2024-07-23 at 4 58 19 PM

Related issue number

Part of #75

How to verify the PR

Was the PR tested

  • I have added >=1 unit test(s) for every new method I have added.
  • I have ensured all unit tests pass

@aluu317 aluu317 force-pushed the test_config_utils branch from 5495027 to 168c9a3 Compare July 24, 2024 16:31
@Ssukriti Ssukriti requested review from Abhishek-TAMU and removed request for alex-jw-brooks and Ssukriti July 24, 2024 18:01
@Abhishek-TAMU
Copy link
Collaborator

Abhishek-TAMU commented Jul 25, 2024

Thank you for the PR!
Just curious to know one thing:

In the test function test_get_hf_peft_config_returns_PT_config_correctly, below code checks and raises error tokenizer_name_or_path cannot be None.

    tuning_config = peft_config.PromptTuningConfig(prompt_tuning_init="TEXT")
    with pytest.raises(ValueError) as err:
        config_utils.get_hf_peft_config(None, tuning_config, None)
        assert "tokenizer_name_or_path can't be None" in err.value

As this check and raising of error is done by PromptTuningConfig Class of peft library and not by our code, so was just curious if we comment this code do we still get 100% coverage in file tuning/utils/config_utils.py?

Apart from this doubt, everything looks good to me! Thanks!

@aluu317
Copy link
Collaborator Author

aluu317 commented Jul 25, 2024

Yea, you're right @Abhishek-TAMU . That test doesn't increase test coverage for tuning/utils/config_utils.py. I just wanted to test that the value None for tokenizer_name_or_path was passed down to PromptTuningConfig init and post_init properly and if there's error/exception, we are able to catch it in our utils.

Additional testing wasn't necessary but I'm sure wouldn't hurt

Copy link
Collaborator

@anhuong anhuong left a comment

Choose a reason for hiding this comment

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

Amazing tests Angel, thank you so much for adding them! Have a few small comments on things to change, such as your test methods are testing such good and varied use cases. But since they are testing different functionality, you should separate out each test case, each test method should test a specific aspect/use-case of the function or method. Use long and descriptive names for testing functions to show what you are testing.

tests/utils/test_config_utils.py Outdated Show resolved Hide resolved
tests/utils/test_config_utils.py Outdated Show resolved Hide resolved
# Default values: r=8, lora_alpha=32, lora_dropout=0.05, target_modules=["q_proj", "v_proj"]
tuning_config = peft_config.LoraConfig(r=3, lora_alpha=3)

config = config_utils.get_hf_peft_config("CAUSAL_LM", tuning_config, "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another test could be, what happens if a tokenizer is passed? Since tokenizer_name_or_path is by default set to model_name_or_path, then this value will always get passed when it's run in sft_trainer.py. I believe tokenizer is used in PromptTuning but would be ignored for LoRA?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the only comment that I think wasn't addressed, what do you think about adding a test where you run with lora and pass in a tokenizer (which we expect to be ignored)?

tests/utils/test_config_utils.py Outdated Show resolved Hide resolved
tests/utils/test_config_utils.py Show resolved Hide resolved
tests/utils/test_config_utils.py Outdated Show resolved Hide resolved
tuning_config = peft_config.PromptTuningConfig(prompt_tuning_init="RANDOM")
config = config_utils.get_hf_peft_config(None, tuning_config, None)
assert isinstance(config, PromptTuningConfig)
assert config.tokenizer_name_or_path is None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting! So that means others like prompt_tuning_init_text which is also only used if prompt_tuning_init is TEXT would also become None?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the function call to be more readable. The function call is

config = config_utils.get_hf_peft_config(task_type=None, tuning_config=tuning_config, tokenizer_name_or_path=None)

So the tokenizer_name_or_path is allowed to be None only when prompt_tuning_init is type RANDOM. When prompt_tuning_init is TEXT, you cannot pass None for tokenizer. It raises an exception. Reference

Other fields are not affected.

tests/utils/test_config_utils.py Show resolved Hide resolved
tests/utils/test_config_utils.py Show resolved Hide resolved
tests/utils/test_config_utils.py Show resolved Hide resolved
aluu317 added 3 commits July 30, 2024 11:34
Signed-off-by: Angel Luu <[email protected]>
Signed-off-by: Angel Luu <[email protected]>
@aluu317 aluu317 force-pushed the test_config_utils branch from 19e39f9 to 906ce02 Compare July 30, 2024 16:42
Copy link
Collaborator

@anhuong anhuong left a comment

Choose a reason for hiding this comment

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

LGTM, many thanks Angel! Love having more test coverage

@aluu317 aluu317 merged commit a9b8ec8 into foundation-model-stack:main Jul 30, 2024
7 checks passed
@aluu317 aluu317 deleted the test_config_utils branch July 30, 2024 20:29
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.

3 participants