tenstorrent / tt-metal

:metal: TT-NN operator library, and TT-Metalium low level kernel programming model.
Apache License 2.0
467 stars 73 forks source link

Add requirement for models developers to invoke pytests w/ file names with if no statements in bash, rather than using direct test invocations to enable simpler pytest commands in CI and scripting and do this for multi-chip pipeline #7586

Open tt-rkim opened 6 months ago

tt-rkim commented 6 months ago

cc: @TT-billteng @vtangTT @tapspatel

Tagging some relevant model writers:

@mtairum @kpaigwar @cglagovichTT @kevinmiTT11 @boris-drazic

TT-billteng commented 6 months ago

if a suite has issues when multiple tests are running in the same process, we can fall back to running each test in its own process https://stackoverflow.com/questions/48234032/run-py-test-test-in-different-process

cglagovichTT commented 6 months ago

Is there a way to skip a test if pytest is being run in a specific CI action?

tt-rkim commented 6 months ago

we can fall back to running each test in its own process

I wanna note here that infra team has taken the position that this is a more nuclear option and it's something that we shouldn't encourage. We should only use it if we're backed into a really bad corner.

Falling back to separate processes will hide any bad state cleanup on the host side , as @TT-billteng has mentioned in both architecture and infra discussions around the entire stack.

Another way to do this is to enforce the timeout option with separate process on pytest in the nightly pipelines, rather than install another package pytest-xdist

tt-rkim commented 6 months ago

@cglagovichTT could you pls elaborate - do you mean you want a way to skip a test locally if it's already in CI>?

cglagovichTT commented 6 months ago

I mean we have a test file with 20 tests. They are useful when we are developing locally, but we only want 4 of those to run in the frequent-multi-device pipeline. Are we able to use skip_??? to skip those 16 other tests if this file is being run in a pipeline?

tt-rkim commented 6 months ago

How do you feel about refactoring out the CI tests into a separate file that we call instead?

For example, I see in models/demos/t3000/llama2_70b/tests/test_llama_mlp.py that you just run the T3000 parametrize branch. Is it possible to refactor those out into a separate test file with a common function that takes in all the fixtures?

We're thinking on the infra team to symlink all model tests files into symlink dirs like

etc.

and we would symlink whatever you would call my suggestion. ie. models/demos/t3000/llama2_70b/tests/test_llama_mlp_t3000.py

tt-rkim commented 6 months ago

@cglagovichTT @kpaigwar have both suggested the same thing, so I think that warrants something to do on infra side.

I've opened a new issue for infra team to work on a new such decorator that can skip tests https://github.com/tenstorrent/tt-metal/issues/7598

However, this should not block the above suggestions. Please still refactor your tests to be callable by a filename.

cglagovichTT commented 6 months ago

Thanks @tt-rkim . We will start refactoring Llama tests

tt-rkim commented 3 months ago

Note again that this is the relevant issue we opened a while ago that is one of the ways that will help ensuring that our tests are written in a clean manner that will ensure they all run, even if some fail @uaydonat