RFC: Add CI Check in Pull Requests to Run the Unit Tests

[courtneypacheco]

Problem Statement / Motivation

This repository has a handful of unit tests scattered throughout. Here are a few examples, but by no means an exhaustive list:

• CLI tests: https://github.com/meta-llama/llama-stack/tree/main/llama_stack/cli/tests
• Provider tests: https://github.com/meta-llama/llama-stack/tree/main/llama_stack/providers/tests

However, when a contributor creates a pull request, there is no CI check or workflow that runs the existing unit tests against the PR's proposed code changes. This means PR code is not validated on the unit test level before being merged unless the reviewer pulls the PR author's code and runs the unit tests manually.

Proposed Solution

Add a CI check or workflow that automatically executes the unit tests for each pull request that contains code changes. (In other words, there is no need to run all the unit tests against documentation, like READMEs.) Since unit tests already exist in the repository, those can be leveraged.

Benefits & Value

If the unit tests are required to pass before each PR is merged, then the likelihood of code breakage will lessen over time. Of course, functional tests, E2E tests, etc. would very likely be beneficial too, but this particular discussion strictly focuses on unit testing because the unit test exist right now in the repository

High-Level Functional & Technical Requirements

1. The unit tests will need to run somewhere -- a GitHub Runner, an EC2 instance, etc.
2. The unit test results will need to be reported back to the CI
3. If one or more unit tests fail, the CI should block a PR from merging

Additional Notes

If the existing unit tests do not pass, they should ultimately be fixed so that they do pass, but I do not believe that failing unit tests are a blocker for this particular enhancement. If one or more unit tests are presently failing, they should be omitted (skipped / commented) for now, and an issue can be filed later on to fix the broken unit tests. (And I'd be more than willing to help with this.)

[yanxi0830]

Thanks for formalizing the proposal, it is something we have thought about for a long time! Indeed we have a bunch of unit tests scattered throughout that we want to unify (#652).

During our release process, we run 2 sets of integration tests:

1. E2E client-sdk tests in https://github.com/meta-llama/llama-stack/tree/main/tests/client-sdk, which we invoke

2. Notebook tests via https://github.com/meta-llama/llama-stack-ops/blob/76ee0c83f9f5eb7d414537e7a917ceeb2c7c2942/actions/test-published-package/main.sh#L71-L86

However, these tests depends on API calls & it will be costly to run CI on all PRs + all providers. Additionally, GPU instances is needed to setup CI tests on providers like vLLM, which we have not done yet.

I think we should decide first on which sets of tests to run on PRs (lightweight) and which sets of tests (more heavyweight) to run before release. E.g.
(a) CLI / build / run tests can run a PR base (e.g. to make sure at least llama stack CLI do not break)
(b) Integration tests costing $$ run on per release basis.

cc @ashwinb for thoughts.

[courtneypacheco]

Hey @yanxi0830 ! That makes a lot of sense.

Yes, it can certainly be costly to make API calls and use GPU instances for GPU-specific workloads!

For the existing unit tests, have you considered dividing them into three major categories? I suggest:

• Unit tests where all API calls and GPU calls are mocked (e.g., using unittest.mock.patch)
• Functional (live-dependency) tests where the code actually does make API calls, but does not involve GPUs
• Functional (live-dependency) tests where GPUs are actually tested

If we divide the existing unit tests in this manner, the first category of tests can theoretically run on an inexpensive, CPU-only instance in each PR build since you will have abstracted away the need for API and GPU testing. Plus, when you design a GitHub Action that runs on a PR build automatically, it is generally recommended to be cautious when using API keys, SSH keys, etc. :) (There is lots of opportunity for bad actors to manipulate the codebase and steal/use your API keys if they are stored in env variables or even mounted into a container somehow.)

The latter two (functional) tests could be run at your own discretion, like for PR builds which you think are high risk.

[franciscojavierarceo]

+1, I think a lot of the vector_io could be easily tested without GPUs.