Add unit test to verify target_modules defaults correctly

[willmj]

Description of the change

Add unit test for recent change (PR #269) to verify that HF library correctly changes target_modules to the default values.

Related issue number

How to verify the PR

Run unit tests.

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]

Thank you @willmj .
This is helpful!

• I would argue this test isn't just to test a launch script functionality, but rather should exist in test_sft_trainer. It is more about sft_trainer.
• I would also suggest to add another test in test_config_utils.py to test that when nothing is set in for target_modules by our local peft_config.LoraConfig, HF peft.LoraConfig still has target_modules None set (and not any other value like we set before).
• comment in line to be more specific/readable for future maintenance.

[willmj]

It seems like the second test you mentioned to add is covered already in test_get_hf_peft_config_returns_lora_config_correctly(). Let me know if there's any additional functionality there that needs testing.

    assert (
        config.target_modules is None
    )  # default value from local peft_config.LoraConfig

[aluu317]

It seems like the second test you mentioned to add is covered already in test_get_hf_peft_config_returns_lora_config_correctly(). Let me know if there's any additional functionality there that needs testing.

    assert (
        config.target_modules is None
    )  # default value from local peft_config.LoraConfig

Yup, this is fine. Thank you!

[aluu317]

This looks good thanks @willmj . I approved but please wait for Anh or Sukriti!

[anhuong]

Instead of importing from launch_scripts, you can create your own dict using the full set of args we defined within this test.

Suggested change

	TRAIN_KWARGS = {**BASE_LORA_KWARGS, **{"output_dir": tempdir}}
	TRAIN_KWARGS = {**MODEL_ARGS, **TRAIN_ARGS, **DATA_ARGS, **PEFT_LORA_ARGS, **{"output_dir": tempdir}}

this should be creating the same but keeps the params all in one file to track

[anhuong]

nit: these details are a little off

set to None in KWARGS sent to main

we are not passing args via KWARGS in this test, we are passing it via JSON

the default value by model type will be using in training.

should specify LoRA tuning

We use TRAIN_KWARGS from test_launch_script.py to run training with, which has no target_modules set.

Why pull the args from test_launch_script? That makes is confusing that if a user changes a param in test_launch_script that it could fail a test in test_sft_trainer

and "q_proj", "v_proj" will then exist in the resulting in adapter_config.json.

I don't think need to specify this since you say the correct target modules used for model type Llama and have HF link and the test shows this

model type Llama

Good to be more specific, use LlamaForCausalLM

[willmj]

Thanks for looking out! Yeah it's definitely confusing to pull from test_launch_script, I didn't want to add too much to test_sft_trainer for just one test, but that makes sense - I'll go ahead and change that.

Good to be more specific, use LlamaForCausalLM

I actually think the model type (the one that HF uses for defaulting) I mean is llama, let me know what you think.

[anhuong]

Would be good to add a test where run parse_arguments and verify that not passing in target_modules gives None still. This can be added to existing test on line 149

[anhuong]

Ahh I see since these aren't dicts, you can't combine them as easily. Nice way of combining them! Although a small refactor would be to combine your custom ones

**PEFT_LORA_ARGS.__dict__,
**{"peft_method": "lora", "output_dir": tempdir},

[anhuong]

nit: slight rewording for clarity and making a little more succinct

Suggested change

	"""Check that if target_modules is not set, or set to None via JSON, the
	default value by model type will be using in LoRA tuning.
	We use MODEL_ARGS to run tuning, which has no target_modules set. During HF
	training process, the correct default target modules will be used for model
	type llama and will then exist in the resulting in adapter_config.json.
	"""Check that if target_modules is not set, or set to None via JSON, the
	default value by model type will be using in LoRA tuning.
	The correct default target modules will be used for model
	type llama and will exist in the resulting adapter_config.json.

[anhuong]

Can remove comment

[anhuong]

This isn't used so can remove

[anhuong]

can also assert not hassattr(tune_config, "target_modules") so that it verifies the input from job_config_lora and this above assertion verifies that after parse_arguments run, the value is still None for target_modules

[willmj]

When I add this check I get the following error:

>       assert not hasattr(tune_config, "target_modules")
E       AssertionError: assert not True
E        +  where True = hasattr(LoraConfig(r=8, lora_alpha=32, target_modules=None, lora_dropout=0.05), 'target_modules')

I think this is because the attribute target_modules exists but is None. Would this suffice?
assert tune_config.target_modules is None

[anhuong]

yes that makes sense and yes that check looks good! i think you can refactor it down to assert not tune_config.target_modules

[anhuong]

LGTM, thanks Will!