-
Notifications
You must be signed in to change notification settings - Fork 18
[CI] Tests for graph comparison between vllm and AFTU #286
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Wallas Santos <wallashss@ibm.com>
…-compare-graph Signed-off-by: Wallas Santos <wallashss@ibm.com>
Signed-off-by: Wallas Santos <wallashss@ibm.com>
👋 Hi! Thank you for contributing to vLLM support on Spyre.
Or this can be done with
Now you are good to go 🚀 |
Signed-off-by: Wallas Santos <wallashss@ibm.com>
Signed-off-by: Wallas Santos <wallashss@ibm.com>
Signed-off-by: Wallas Santos <wallashss@ibm.com>
Signed-off-by: Wallas Santos <wallashss@ibm.com>
@@ -150,6 +151,7 @@ dev = [ | |||
"pytest-timeout==2.3.1", | |||
"requests==2.32.3", | |||
"sentence-transformers==3.4.1", | |||
"aiu_fms_testing_utils@git+https://github.com/foundation-model-stack/aiu-fms-testing-utils.git#1a77f630104a5661fff554164c1e536ea08393e3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if we need to update the uv.lock
file as well 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
Signed-off-by: Wallas Santos <wallashss@ibm.com>
# Get G1 graphs, it assumes the input_dir has the folder export_dtcompiler | ||
# where are the files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: something like
# Get G1 graphs, it assumes the input_dir has the folder export_dtcompiler | |
# where are the files | |
# Get G1 graphs. | |
# Assumes the 'input_dir' contains 'export_dtcompiler' with the files. |
# Silly regex to find all s#. | ||
# We are only considered those that surrounds by space (whole word) | ||
# or started with space and terminated with comma | ||
# examples: | ||
# ' s1 ' | ||
# ' s1,' | ||
# ' s1 s2 ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a nit: comment could be a bit more succinct, something like:
# Silly regex to find all s#. | |
# We are only considered those that surrounds by space (whole word) | |
# or started with space and terminated with comma | |
# examples: | |
# ' s1 ' | |
# ' s1,' | |
# ' s1 s2 ' | |
# Regex to find all 's#' patterns surrounded by spaces, | |
# or starting with a space and ending with a comma. | |
# Examples: ' s1 ', ' s1,', ' s1 s2 ' |
|
||
@pytest.mark.aftu | ||
@pytest.mark.parametrize("model", get_spyre_model_list()) | ||
@pytest.mark.parametrize("backend", ["sendnn"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this test can only run on the sendnn
backend then we don't need to parameterize it, we can just set the backend as sendnn in the test.
Also, we should mark all tests running with the sendnn backend with pytest.mark.spyre
for consistency with the rest of the test suite
|
||
|
||
def get_aftu_script_dir() -> str: | ||
# TODO: since AFTU is not a lib yet, this function does the best |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does AFTU is not a lib yet
mean here? Is the problem that inference.py
is not shipped with the AFTU wheel?
@@ -131,6 +131,7 @@ markers = [ | |||
"multi: Tests that require >1 cards", | |||
"utils: Tests for utility functions", | |||
"worker: Tests for worker logic", | |||
"aftu: Tests to compare graphs from aiu-fms-testing-utils", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to avoid creating more custom markers unless it's completely necessary. (Unrelated but it looks like utils
and worker
are unused and we should delete them as well)
These tests seem to be important to catch problems early so I do want them running with our default set of markers if possible
Description
This PR adds tests that compare graphs generated by vLLM vs AFTU (aiu-fms-testing-utils). The tests do an inference with vLLM and execute inference.py from AFTU. For both execution, the tests dump the compiled graphs to files by setting the env variable
DEE_DUMP_GRAPHS=name
. Then, they compare the corresponding graphs generated for both method.The main limitation of this implementation is AFTU is not a proper package. So, I had to make some workarounds:
VLLM_SPYRE_TEST_AFTU_SCRIPTS_DIR
. If set, we can use to inform where is the scripts dirs (where it has theinference.py
) to copy from there and execute the script within the tests.aiu_fms_testing_utils.__file__
variable to find the directory.I added a marker
aftu
to ease select the tests of this PR, and also to filter out from other test suite.