Enhance lora tests with more layer and rank variations #3243

tterrysun · 2024-03-06T22:19:14Z

Enhance lora tests with more layer and rank variations.

pcmoritz · 2024-03-06T22:40:14Z

requirements.txt

@@ -8,6 +8,7 @@ transformers >= 4.38.0  # Required for Gemma.
 xformers == 0.0.23.post1  # Required for CUDA 12.1.
 fastapi
 uvicorn[standard]
+peft == 0.8.2


Given that this is only used for testing, it should go to requirements-dev.txt

pcmoritz · 2024-03-06T22:48:20Z

tests/lora/test_layer_variation.py

+    "[system] Given a target sentence construct the underlying meaning representation\nof the input sentence as a single function with attributes and attribute\nvalues. This function should describe the target string accurately and the\nfunction must be one of the following ['inform', 'request', 'give_opinion',\n'confirm', 'verify_attribute', 'suggest', 'request_explanation',\n'recommend', 'request_attribute'].\n\nThe attributes must be one of the following:\n['name', 'exp_release_date', 'release_year', 'developer', 'esrb', 'rating',\n'genres', 'player_perspective', 'has_multiplayer', 'platforms',\n'available_on_steam', 'has_linux_release', 'has_mac_release', 'specifier'] [/system] [user] Here is the target sentence:\nI wanted to like Grimlore Games' 2017 entry, but in SpellForce 3 they just didn't get anything right. [/user] [assistant]",
+    "[system] Given a target sentence construct the underlying meaning representation\nof the input sentence as a single function with attributes and attribute\nvalues. This function should describe the target string accurately and the\nfunction must be one of the following ['inform', 'request', 'give_opinion',\n'confirm', 'verify_attribute', 'suggest', 'request_explanation',\n'recommend', 'request_attribute'].\n\nThe attributes must be one of the following:\n['name', 'exp_release_date', 'release_year', 'developer', 'esrb', 'rating',\n'genres', 'player_perspective', 'has_multiplayer', 'platforms',\n'available_on_steam', 'has_linux_release', 'has_mac_release', 'specifier'] [/system] [user] Here is the target sentence:\nBioShock is a good role-playing, action-adventure, shooter that released for PlayStation, Xbox, and PC in 2007. It is available on Steam, and it has a Mac release but not a Linux release. [/user] [assistant]",
+]
+TMP_PATH = "/mnt/local_storage/"


remove this :)

pcmoritz · 2024-03-06T22:49:06Z

tests/lora/conftest.py

@@ -121,6 +124,14 @@ def sql_lora_files():
    return snapshot_download(repo_id="yard1/llama-2-7b-sql-lora-test")


+@pytest.fixture(scope="session")


you don't need this fixture and TMP_PATH above. Pytest already has a built in tmpdir fixture that you can use https://docs.pytest.org/en/6.2.x/tmpdir.html#the-tmpdir-fixture

pcmoritz · 2024-03-06T23:01:44Z

tests/lora/test_layer_variation.py

+# Test the functionality when layer and rank are varied
+@pytest.mark.parametrize("target_modules", TARGET_MODULES_LIST)
+@pytest.mark.parametrize("rank", [8, 16, 32, 64])
+def test_layer_variation_functionality(target_modules, rank, tmp_path):


We should just remove this test -- it is completely subsumed by test_layer_variation_verify_reference, right?

pcmoritz · 2024-03-06T23:21:36Z

tests/lora/test_layer_variation.py

+TMP_PATH = "/mnt/local_storage/"
+
+
+def get_lora_model(model_id: str, target_modules: List[str], rank: int):


I currently don't understand this function -- what are the lora model weights that are actually applied on top of the meta-llama/Llama-2-7b-hf base model?

it's a default initialized lora, we use the merged one as golden reference to verify the correctness, the lora weights won't matter as long as we're using the same one

Can you point to where in the docs it says it is a default LoRA and what it is? That part was not clear to me (maybe add a comment)

if i understand correctly this is the init config https://github.com/huggingface/peft/blob/main/src/peft/tuners/lora/config.py#L158-L159

pcmoritz · 2024-03-06T23:25:48Z

tests/lora/test_layer_variation.py

+        generated_texts.append(generated_text)
+        print(f"Prompt: {prompt!r}, Generated text: {generated_text!r}")
+        generated_logprobs.append([
+            list(logprob.keys()) for logprob in outputs[0].outputs[0].logprobs


Should this be output.outputs[0].logprobs? Otherwise you will only ever use the first prompt, right?

Wait, but this is only ever using the first prompt (i.e. PROMPT[0]) if I understand this correctly -- that can't possibly be your intention. Otherwise why even include the other prompts?

yes you're right, I misread the comment. fixed

pcmoritz · 2024-03-07T00:11:42Z

On a high level, I think these tests would be much better understandable if it was a single test that evaluates the reference and then tests against the target, instead of having two tests and hardcoding the taget.

I also find it very irritating that we only test correctness of the first token for each sequence. Is there a way to test the rest of the sequence too and still have it be robust? Other tests do similar things, right? Maybe instead of testing the predicted tokens, you can test that the logits are numerically close within some error.

pcmoritz · 2024-03-07T00:14:13Z

tests/lora/conftest.py

@@ -21,6 +21,8 @@
 from vllm.model_executor.parallel_utils.parallel_state import (
    destroy_model_parallel, initialize_model_parallel)

+TMP_PATH = "/mnt/local_storage/"


This should be removed

pcmoritz · 2024-03-07T18:00:22Z

tests/lora/test_layer_variation.py

+@pytest.mark.parametrize("target_modules", TARGET_MODULES_LIST)
+@pytest.mark.parametrize("rank", [8, 16, 32, 64])
+def test_layer_variation_correctness(tp_size, target_modules, rank, tmpdir):
+    if torch.cuda.device_count() < tp_size:


This will make it skip in the CI -- try tp_size 1 like in test_llama.py

pcmoritz · 2024-03-07T18:01:32Z

tests/lora/test_layer_variation.py

+    merged_probs = do_sample(llm, tmp_dir_lora, 1, logprobs=5, n_tokens=32)
+    del llm
+    cleanup()
+    shutil.rmtree(str(tmpdir))


If you need to delete the temp dir, it will be better to use

with tempfile.TemporaryDirectory(delete=True) as tmpdir: # Code that uses temp dir here

pcmoritz · 2024-03-07T18:03:04Z

tests/lora/test_layer_variation.py

+              n_tokens: int = 256):
+    prompts = PROMPTS
+    sampling_params = vllm.SamplingParams(temperature=0,
+                                          max_tokens=256,


Is there a reason btw you are not setting max_tokens=n_tokens here and then skip the slicing below?

pcmoritz · 2024-03-07T22:54:57Z

tests/lora/test_layer_variation.py

+
+    model = get_lora_model(MODEL_PATH, target_modules, rank)
+    with tempfile.TemporaryDirectory() as tmpdir:
+        tmp_dir_merged = os.path.join(tmpdir, "tmp_dir_merged")


Is there a need to introduce an additional layer of tmp directories? Same above.

pcmoritz · 2024-03-07T22:55:22Z

tests/lora/test_layer_variation.py

+                       tokenizer=MODEL_PATH,
+                       enable_lora=False,
+                       max_num_seqs=16,
+                       tensor_parallel_size=4,


This shouldn't be hard coded (this won't work in the CI)

pcmoritz

Nice! There are other tests where

    if torch.cuda.device_count() < tp_size:

is causing problems, it might be better to remove that?

richardliaw · 2024-03-09T09:57:00Z

can we merge?

…3243)

tterrysun added 3 commits March 6, 2024 14:09

enhance lora tests

221aaee

add test fixture

d0cc1c9

update requirements

ccb1872

pcmoritz reviewed Mar 6, 2024

View reviewed changes

tterrysun added 2 commits March 6, 2024 15:58

remove redundant tests

b35b7f1

minor fix

02ec9f9

pcmoritz reviewed Mar 7, 2024

View reviewed changes

tterrysun added 4 commits March 6, 2024 18:18

verify all tokens and refactor

2c820da

minorfix

997f6a5

minor fix

0baf6ba

check 32 tokens

cd2211e

tterrysun requested a review from pcmoritz March 7, 2024 02:48

minor fix

fa1f6f0

pcmoritz reviewed Mar 7, 2024

View reviewed changes

tterrysun added 2 commits March 7, 2024 12:47

minor refactor

c80ad53

Merge branch 'main' into lora_test_enhancement

991c577

tterrysun marked this pull request as ready for review March 7, 2024 20:49

minor fix

2fda771

pcmoritz reviewed Mar 7, 2024

View reviewed changes

tterrysun added 2 commits March 7, 2024 15:49

minor fixes

1e48f26

ablation study on ci

b430726

use smaller model

4543e8f

tterrysun requested a review from pcmoritz March 9, 2024 00:23

add doc string

ce3dd58

Yard1 approved these changes Mar 9, 2024

View reviewed changes

pcmoritz approved these changes Mar 9, 2024

View reviewed changes

remove redundant code

0033822

simon-mo merged commit 0bba88d into vllm-project:main Mar 10, 2024
23 checks passed

dtransposed pushed a commit to afeldman-nm/vllm that referenced this pull request Mar 26, 2024

Enhance lora tests with more layer and rank variations (vllm-project#…

a20b244

…3243)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance lora tests with more layer and rank variations #3243

Enhance lora tests with more layer and rank variations #3243

tterrysun commented Mar 6, 2024

pcmoritz Mar 6, 2024

pcmoritz Mar 6, 2024

pcmoritz Mar 6, 2024

pcmoritz Mar 6, 2024

tterrysun Mar 6, 2024

pcmoritz Mar 6, 2024 •

edited

tterrysun Mar 6, 2024

pcmoritz Mar 6, 2024

tterrysun Mar 7, 2024

pcmoritz Mar 6, 2024

pcmoritz Mar 7, 2024

tterrysun Mar 7, 2024 •

edited

pcmoritz commented Mar 7, 2024 •

edited

pcmoritz Mar 7, 2024

pcmoritz Mar 7, 2024

pcmoritz Mar 7, 2024

pcmoritz Mar 7, 2024

pcmoritz Mar 7, 2024

pcmoritz Mar 7, 2024

pcmoritz left a comment

richardliaw commented Mar 9, 2024

		@@ -121,6 +124,14 @@ def sql_lora_files():
		return snapshot_download(repo_id="yard1/llama-2-7b-sql-lora-test")


		@pytest.fixture(scope="session")

		TMP_PATH = "/mnt/local_storage/"


		def get_lora_model(model_id: str, target_modules: List[str], rank: int):

Enhance lora tests with more layer and rank variations #3243

Enhance lora tests with more layer and rank variations #3243

Conversation

tterrysun commented Mar 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcmoritz Mar 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tterrysun Mar 7, 2024 • edited

Choose a reason for hiding this comment

pcmoritz commented Mar 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcmoritz left a comment

Choose a reason for hiding this comment

richardliaw commented Mar 9, 2024

pcmoritz Mar 6, 2024 •

edited

tterrysun Mar 7, 2024 •

edited

pcmoritz commented Mar 7, 2024 •

edited