Dpatel enable tests on spyre #77

dpatel-ops · 2025-02-10T14:55:42Z

This PR is to enable tests for spyre execution:
It allows users to achieve the following via environment variables:

to pass dynamic model directory
to pass back end type
to pass list of models

Note:

The default behavior values are set to assume that tests are being executed in CPU environment so if no value is set then it will execute based on existing CPU environment tests.

Expected behavior:

The tests should execute on cpu without any changes without any execution behavior changes
If correct environment values are set then, tests should successfully run on spyre environment

…ss values for spyre

bug fix: variable number of max decode tokens within batch (IBM#73)

github-actions · 2025-02-10T14:55:55Z

👋 Hi! Thank you for contributing to the vLLM project.
Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can do one of these:

Add ready label to the PR
Enable auto-merge.

🚀

sducouedic · 2025-02-11T20:11:08Z

tests/spyre/test_spyre_basic.py

 @pytest.mark.parametrize("prompts", [[
    "Provide a list of instructions for preparing"
    " chicken soup for a family of four.", "Hello",
    "What is the weather today like?", "Who are you?"
 ]])
 @pytest.mark.parametrize("warmup_shape", [(64, 20, 4), (64, 20, 8),
                                          (128, 20, 4), (128, 20, 8)]
+# @pytest.mark.parametrize("warmup_shape", [(64, 20, 1), (128, 20, 1)]


shouldn't this line be removed?

sducouedic · 2025-02-11T20:16:36Z

tests/spyre/test_spyre_embeddings.py

+# get model backend from env, if not set then default to "eager" 
+# For multiple values, export SPYRE_TEST_MODEL_DIR="eager,inductor"
+backend_type = os.environ.get("SPYRE_TEST_BACKEND_TYPE", "eager")
+# get model names from env, if not set then default to "llama-194m" 


Suggested change

# get model names from env, if not set then default to "llama-194m"

# get model names from env, if not set then default to "all-roberta-large-v1"

sducouedic · 2025-02-11T20:21:56Z

tests/spyre/test_spyre_max_prompt_length.py

+backend_type = os.environ.get("SPYRE_TEST_BACKEND_TYPE", "eager")
+# get model names from env, if not set then default to "llama-194m" 
+# For multiple values, export SPYRE_TEST_MODEL_DIR="llama-194m,all-roberta-large-v1"
+user_test_model_list = os.environ.get("SPYRE_TEST_MODEL_LIST","llama-194m")


Just a note: you named it SPYRE_TEST_MODEL_LIST because multiple models can be tested, but you named SPYRE_TEST_BACKEND_TYPE and not SPYRE_TEST_BACKEND_TYPE_LIST. Same for the other files.

sducouedic · 2025-02-11T20:23:50Z

tests/spyre/test_spyre_max_prompt_length.py

+#                          )  # (prompt_length/new_tokens/batch_size)
+# @pytest.mark.parametrize("warmup_shapes",
+#                          [[(64, 20, 1)], [(128, 20, 1)]]


same as before: should it be removed?

sducouedic · 2025-02-11T20:26:33Z

tests/spyre/test_spyre_basic.py

+# For multiple values, export SPYRE_TEST_MODEL_DIR="eager,inductor"
+backend_type = os.environ.get("SPYRE_TEST_BACKEND_TYPE", "eager")
+# get model names from env, if not set then default to "llama-194m" 
+# For multiple values, export SPYRE_TEST_MODEL_DIR="llama-194m,all-roberta-large-v1"


Suggested change

# For multiple values, export SPYRE_TEST_MODEL_DIR="llama-194m,all-roberta-large-v1"

# For multiple values, export SPYRE_TEST_MODEL_LIST="llama-194m,all-roberta-large-v1"

sducouedic · 2025-02-11T20:31:56Z

tests/spyre/test_spyre_tensor_parallel.py

+# get model directory path from env, if not set then default to "/models". 
+model_dir_path = os.environ.get("SPYRE_TEST_MODEL_DIR", "/models")
+# get model backend from env, if not set then default to "eager" 
+# For multiple values, export SPYRE_TEST_MODEL_DIR="eager,inductor"


Suggested change

# For multiple values, export SPYRE_TEST_MODEL_DIR="eager,inductor"

# For multiple values, export SPYRE_TEST_BACKEND_TYPE="eager,inductor"

sducouedic · 2025-02-11T20:32:09Z

tests/spyre/test_spyre_tensor_parallel.py

+# For multiple values, export SPYRE_TEST_MODEL_DIR="eager,inductor"
+backend_type = os.environ.get("SPYRE_TEST_BACKEND_TYPE", "eager")
+# get model names from env, if not set then default to "llama-194m" 
+# For multiple values, export SPYRE_TEST_MODEL_DIR="llama-194m,all-roberta-large-v1"


Suggested change

# For multiple values, export SPYRE_TEST_MODEL_DIR="llama-194m,all-roberta-large-v1"

# For multiple values, export SPYRE_TEST_MODEL_LIST="llama-194m,all-roberta-large-v1"

sducouedic · 2025-02-11T20:34:02Z

tests/spyre/test_spyre_warmup_shapes.py

+# get model directory path from env, if not set then default to "/models". 
+model_dir_path = os.environ.get("SPYRE_TEST_MODEL_DIR", "/models")
+# get model backend from env, if not set then default to "eager" 
+# For multiple values, export SPYRE_TEST_MODEL_DIR="eager,inductor"


Suggested change

# For multiple values, export SPYRE_TEST_MODEL_DIR="eager,inductor"

# For multiple values, export SPYRE_TEST_BACKEND_TYPE="eager,inductor"

sducouedic · 2025-02-11T20:34:15Z

tests/spyre/test_spyre_warmup_shapes.py

+# For multiple values, export SPYRE_TEST_MODEL_DIR="eager,inductor"
+backend_type = os.environ.get("SPYRE_TEST_BACKEND_TYPE", "eager")
+# get model names from env, if not set then default to "llama-194m" 
+# For multiple values, export SPYRE_TEST_MODEL_DIR="llama-194m,all-roberta-large-v1"


Suggested change

# For multiple values, export SPYRE_TEST_MODEL_DIR="llama-194m,all-roberta-large-v1"

# For multiple values, export SPYRE_TEST_MODEL_LIST="llama-194m,all-roberta-large-v1"

sducouedic · 2025-02-11T20:36:32Z

tests/spyre/test_spyre_warmup_shapes.py

 @pytest.mark.parametrize("backend",
-                         ["eager"])  #, "inductor", "sendnn_decoder"])
+                         test_backend_list)  #, "inductor", "sendnn_decoder"])


open question: should we still keep the #, "inductor", "sendnn_decoder"]) part if we use a list of backends? Maybe we can put that in the header part?

- fix comments

dpatel-ops · 2025-02-12T00:54:04Z

Thank you @sducouedic for all the inputs. I have made few changes. Please let me know if anything is missed.

sducouedic

LGTM

tdoublep · 2025-02-13T14:22:58Z

tests/spyre/test_spyre_warmup_shapes.py

+# get model directory path from env, if not set then default to "/models". 
+model_dir_path = os.environ.get("SPYRE_TEST_MODEL_DIR", "/models")
+# get model backend from env, if not set then default to "eager" 
+# For multiple values, export SPYRE_TEST_BACKEND_LIST="eager,inductor,sendnn_decoder"
+backend_list = os.environ.get("SPYRE_TEST_BACKEND_LIST", "eager")
+# get model names from env, if not set then default to "llama-194m" 
+# For multiple values, export SPYRE_TEST_MODEL_LIST="llama-194m,all-roberta-large-v1"
+user_test_model_list = os.environ.get("SPYRE_TEST_MODEL_LIST","llama-194m")
+test_model_list, test_backend_list = [],[]

-@pytest.mark.parametrize("model", ["/models/llama-194m"])
+for model in user_test_model_list.split(','):
+    test_model_list.append(f"{model_dir_path.strip()}/{model.strip()}")
+
+for backend in backend_list.split(','):
+    test_backend_list.append(backend.strip())
+


Since these environment variables (and the code to parse them) are used in multiple files, I think it would make sense to move them into a common place (spyre_util.py perhaps) and then have each test import them from there.

tdoublep

Please also ensure the changes are adhering to the formatting guidelines of vLLM (you can run bash format.sh to run through all the checks).

maxdebayser

@dpatel-007, you can move common helper functions to conftest.py as fixtures that can be used the tests. For example instead of redefining this in every file:

model_dir_path = os.environ.get("SPYRE_TEST_MODEL_DIR", "/models")

You could have

@pytest.fixture(scope="session")
def model_dir_path():
    return os.environ.get("SPYRE_TEST_MODEL_DIR", "/models")

(I'm not saying that this is the ideal solution for model_dir_path, it's just to illustrate the idea)

- fixed E501 erros - created functions in utils and removed repeated code

Fix embeddings tests failure

- fixed spyre_utils and embeddings

dpatel-ops and others added 4 commits January 30, 2025 12:38

- accept model path,model list, backend type from env variables to pa…

1e2a600

…ss values for spyre

fix typo

faa9ff4

Merge pull request #1 from IBM/main

421fb72

bug fix: variable number of max decode tokens within batch (IBM#73)

fix file name in run command

705669e

dpatel-ops requested review from tdoublep and sducouedic February 10, 2025 14:55

dpatel-ops added the enhancement New feature or request label Feb 10, 2025

sducouedic reviewed Feb 11, 2025

View reviewed changes

- update variable names

59a86da

- fix comments

sducouedic approved these changes Feb 12, 2025

View reviewed changes

tdoublep reviewed Feb 13, 2025

View reviewed changes

maxdebayser reviewed Feb 13, 2025

View reviewed changes

dpatel-ops added 3 commits February 15, 2025 18:47

Signed-off-by: Dhruval Patel <[email protected]>

6e3083b

- fixed E501 erros - created functions in utils and removed repeated code

Signed-off-by: Dhruval Patel <[email protected]>

df1261a

Fix embeddings tests failure

Signed-off-by: Dhruval Patel <[email protected]>

d40d69b

- fixed spyre_utils and embeddings

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dpatel enable tests on spyre #77

Dpatel enable tests on spyre #77

dpatel-ops commented Feb 10, 2025 •

edited by github-actions bot

Loading

github-actions bot commented Feb 10, 2025

sducouedic Feb 11, 2025

sducouedic Feb 11, 2025

sducouedic Feb 11, 2025

sducouedic Feb 11, 2025

sducouedic Feb 11, 2025

sducouedic Feb 11, 2025

sducouedic Feb 11, 2025

sducouedic Feb 11, 2025

sducouedic Feb 11, 2025

sducouedic Feb 11, 2025

dpatel-ops commented Feb 12, 2025

sducouedic left a comment

tdoublep Feb 13, 2025

tdoublep left a comment

maxdebayser left a comment

	# get model names from env, if not set then default to "llama-194m"
	# get model names from env, if not set then default to "all-roberta-large-v1"

	# For multiple values, export SPYRE_TEST_MODEL_DIR="llama-194m,all-roberta-large-v1"
	# For multiple values, export SPYRE_TEST_MODEL_LIST="llama-194m,all-roberta-large-v1"

Dpatel enable tests on spyre #77

Are you sure you want to change the base?

Dpatel enable tests on spyre #77

Conversation

dpatel-ops commented Feb 10, 2025 • edited by github-actions bot Loading

github-actions bot commented Feb 10, 2025

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

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

dpatel-ops commented Feb 12, 2025

sducouedic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdoublep left a comment

Choose a reason for hiding this comment

maxdebayser left a comment

Choose a reason for hiding this comment

dpatel-ops commented Feb 10, 2025 •

edited by github-actions bot

Loading