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

Add llama galaxy mlp to TG frequent tests #10274

Merged
merged 12 commits into from
Jul 19, 2024
Merged

Conversation

mikevin920
Copy link
Contributor

@mikevin920 mikevin920 commented Jul 15, 2024

Ticket
#9476

Problem description
We want to add llama galaxy mlp tests to TG frequent

What's changed

  • Added TG DRAM sharded-matmul unit tests
  • Added llama galaxy mlp unit tests
  • Fixed use_1d_systotic_array usage in ttnn.matmul

Checklist

  • Post commit CI passes
  • Model regression CI testing passes (if applicable)
  • New/Existing tests provide coverage for changes

@mikevin920
Copy link
Contributor Author

mikevin920 commented Jul 15, 2024

@ttmchiou
Copy link
Contributor

I'm not sure if we have a folder naming convention, should new folder be named tg or TG?
Otherwise, looks good to me.

@mikevin920
Copy link
Contributor Author

I'm not sure if we have a folder naming convention, should new folder be named tg or TG?

Otherwise, looks good to me.

@uaydonat Is the folder name TG under demos good for now?

@ttmchiou
Copy link
Contributor

one reason why i bring this up is because we have a scripts/tg folder*

I'm not sure if we have a folder naming convention, should new folder be named tg or TG?
Otherwise, looks good to me.

@uaydonat Is the folder name TG under demos good for now?

one reason why i bring this up is because we have a scripts/tg folder*

@mikevin920
Copy link
Contributor Author

mikevin920 commented Jul 19, 2024

one reason why i bring this up is because we have a scripts/tg folder*

I'm not sure if we have a folder naming convention, should new folder be named tg or TG?
Otherwise, looks good to me.

@uaydonat Is the folder name TG under demos good for now?

one reason why i bring this up is because we have a scripts/tg folder*

Hi @ttmchiou looking at other folder names i think it does make more sense to use lowercase, naming have be fixed
TG Frequent run after renaming: https://github.com/tenstorrent/tt-metal/actions/runs/10009557857

@@ -7,6 +7,8 @@ run_tg_tests() {
echo "LOG_METAL: running run_tg_frequent_tests"

pytest tests/ttnn/multichip_unit_tests/test_multidevice_TG.py
pytest models/demos/tg/llama3_70b/tests/test_llama_mlp_galaxy.py
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we should use pytest -n auto to use xdist and better timeout in these pipelines

Copy link
Contributor

Choose a reason for hiding this comment

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

To further expand on @vtangTT 's point
It would be good to expand the usage of xdist across all pytests being called for all model tests going forward.
This method lets us catch and handle hanging tests better and better control timeouts. (using xdist would discourage the use of pytest.skip() at the lower level)

Example reference call below.

pytest -n auto models/demos/falcon7b/tests/test_perplexity_falcon.py::test_perplexity[True-prefill_seq1024_dram] --timeout=1800 ; fail+=$?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified all pytest calls to use xdist and timeouts

Copy link
Contributor

Choose a reason for hiding this comment

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

(using xdist would discourage the use of pytest.skip() at the lower level)

@tt-rkim advices that we call test_llama_mlp_galaxy.py and use pytest.skip to omit tests.

fyi @ttmchiou

@mikevin920 mikevin920 merged commit eff6818 into main Jul 19, 2024
5 checks passed
@mikevin920 mikevin920 deleted the model-team/llama-mlp-galaxy branch July 19, 2024 19:54
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.

7 participants