Use TestDefinion in strategies instead of TestTemplate

[amaslenn]

Summary

TestTemplate is an additional indirection and usually an empty class without any implementation. It serves two purposes:

1. Creating test types.
2. Holding strategies and some high level methods.

While (2) is still relevant, (1) is now better served by TestDefinition classes.

Test Plan

1. CI
2. Run install for conf/common/test.

Additional Notes

—

[TaekyungHeo]

It looks good to me, but let's discuss it in the design meeting and ensure we have two approvals for this PR.

cc: @srinivas212 , @srivatsankrishnan

[srivatsankrishnan]

Let's chat more about this in the design meeting. I was curious about this empty definition before and was under the opinion that it was some placeholder for some future feature. I want to know more about the rationale for designing it this way. Let's chat more.

Shouldn't block approval or merge.

[TaekyungHeo]

This is essential. I can approve it once you resolve the conflicts and complete the necessary tests, @amaslenn .

[TaekyungHeo]

Let's have two approvals.

[TaekyungHeo]

@amaslenn Please make sure that you have a PR ready for CloudAIX, as it needs to be updated accordingly. After merging this, please create a pre-release and update CloudAIX.

[srivatsankrishnan]

In today's call we discussed on testing on some real workloads to ensure it doesn't break anything? or we were okay with acceptance test as that criteria?

[TaekyungHeo]

@srivatsankrishnan, I believe the acceptance test is strong enough, but it might be worthwhile to run a simple test on a real system, such as nccl-test. While I consider it optional, it could still be beneficial to do - @amaslenn

[amaslenn]

@srivatsankrishnan, I believe the acceptance test is strong enough, but it might be worthwhile to run a simple test on a real system, such as nccl-test. While I consider it optional, it could still be beneficial to do - @amaslenn

Tested with cloudai run --system-config ... --tests-dir conf/common/test --test-scenario conf/common/test_scenario/nccl_test.toml

[srivatsankrishnan]

Andrei validated this on NCCL test on a real system. LGTM (Would be good to update the test plan with NCCL output as well).