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

[GPU] llama compile regression test for sharktank exported mlir #19306

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

dan-garvey
Copy link
Contributor

@dan-garvey dan-garvey commented Nov 26, 2024

Pulls mlir from public azure file and compiles. Next steps are including bin files to run with real inputs.

@dan-garvey
Copy link
Contributor Author

@rsuderman fyi

Copy link
Member

@ScottTodd ScottTodd left a comment

Choose a reason for hiding this comment

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

Thanks for the test!

Please add more context to the PR title and description.

@dan-garvey dan-garvey changed the title llama compile test llama compile regression test for sharktank exported mlir Nov 26, 2024
@dan-garvey dan-garvey changed the title llama compile regression test for sharktank exported mlir [GPU] llama compile regression test for sharktank exported mlir Nov 26, 2024
@dan-garvey
Copy link
Contributor Author

woop:
image

###############################################################################

# This allows using as part of a matrix strat that includes cpu
def test_pass_cpu():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ScottTodd any thoughts on this? It gives me mildly gross vibes but enables us to add a test here later if desired and makes it so we dont have to branch on the matrix strategy for this suite of tests. Open to alternatives

Copy link
Member

Choose a reason for hiding this comment

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

Given the existing test setup, I'd rather use `@pytest.mark.skip("reason") instead of "passing" as a no-op: https://docs.pytest.org/en/stable/how-to/skipping.html

So

@pytest.mark.skip("llama tests not implemented for CPU yet")
def test_compile_llama_cpu():
  pass

A better method is used in these older (stale, currently not run) tests, which add marks for the platform and then run using filters:

@pytest.mark.presubmit
@pytest.mark.unstable_linalg
@pytest.mark.plat_rdna3_vulkan
def test_step_rdna3_vulkan_stripped(llama2_7b_f16qi4_stripped_rdna3_vulkan_vmfb):
@pytest.mark.presubmit
@pytest.mark.unstable_linalg
@pytest.mark.plat_host_cpu
def test_step_host_cpu_stripped(llama2_7b_f16qi4_stripped_host_cpu_vmfb):
# TODO(#17344): regenerate .mlirbc files, test plat_rdna3_rocm on rocm
# # In-tree tests
# - name: Run experimental/regression_suite tests
# run: |
# source ${VENV_DIR}/bin/activate
# pytest \
# -rA -s -m "plat_host_cpu and presubmit" \
# experimental/regression_suite

That lets new platforms be added in a structured way, without just relying on test case names following a convention.

Signed-off-by: dan <[email protected]>
Copy link
Member

@ScottTodd ScottTodd left a comment

Choose a reason for hiding this comment

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

Mostly LGTM but would like the generation process to be documented somewhere.

###############################################################################

# This allows using as part of a matrix strat that includes cpu
def test_pass_cpu():
Copy link
Member

Choose a reason for hiding this comment

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

Given the existing test setup, I'd rather use `@pytest.mark.skip("reason") instead of "passing" as a no-op: https://docs.pytest.org/en/stable/how-to/skipping.html

So

@pytest.mark.skip("llama tests not implemented for CPU yet")
def test_compile_llama_cpu():
  pass

A better method is used in these older (stale, currently not run) tests, which add marks for the platform and then run using filters:

@pytest.mark.presubmit
@pytest.mark.unstable_linalg
@pytest.mark.plat_rdna3_vulkan
def test_step_rdna3_vulkan_stripped(llama2_7b_f16qi4_stripped_rdna3_vulkan_vmfb):
@pytest.mark.presubmit
@pytest.mark.unstable_linalg
@pytest.mark.plat_host_cpu
def test_step_host_cpu_stripped(llama2_7b_f16qi4_stripped_host_cpu_vmfb):
# TODO(#17344): regenerate .mlirbc files, test plat_rdna3_rocm on rocm
# # In-tree tests
# - name: Run experimental/regression_suite tests
# run: |
# source ${VENV_DIR}/bin/activate
# pytest \
# -rA -s -m "plat_host_cpu and presubmit" \
# experimental/regression_suite

That lets new platforms be added in a structured way, without just relying on test case names following a convention.

Comment on lines +223 to 224
ROCM_CHIP: ${{ matrix.rocm-chip }}
# Note: mi250 benchmark times are more lenient than mi300 (allowing about
Copy link
Member

Choose a reason for hiding this comment

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

Please add a newline between groups in this file

Suggested change
ROCM_CHIP: ${{ matrix.rocm-chip }}
# Note: mi250 benchmark times are more lenient than mi300 (allowing about
ROCM_CHIP: ${{ matrix.rocm-chip }}
# Note: mi250 benchmark times are more lenient than mi300 (allowing about

Comment on lines +19 to +22
llama_mlir = fetch_source_fixture(
"https://sharkpublic.blob.core.windows.net/sharkpublic/halo-models/llm-dev/llama3_8b/8b_f16_decomposed_11_22.mlir",
group="llama_fp16_8b",
)
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment here indicating how this was generated? What version of the packages were used, what command were run, etc.

Ideally link to some documentation in https://github.com/nod-ai/shark-ai/tree/main/docs

See the earlier comment on #14915 (comment)

We ultimately want all of these coming from common places like that which we update with a generation script. However, as long as we're in this phase with this one of it being a bit hand-massaged, I'm ok with Dan taking responsibility to keep it updated.

;)

(the tests that PR updated are also linked in my other comment... and have been outdated + disabled for months. IDK how to update them since that isn't documented)

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.

2 participants