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

[test automation] support run tests on config file #730

Merged
merged 10 commits into from
Jan 16, 2025
Merged

Conversation

sixianyi0721
Copy link
Contributor

@sixianyi0721 sixianyi0721 commented Jan 7, 2025

Context

For test automation, the end goal is to run a single pytest command from root test directory (llama_stack/providers/tests/.) such that we execute push-blocking tests

The work plan:

  1. trigger pytest from llama_stack/providers/tests/.
  2. use config file to determine what tests and parametrization we want to run

What does this PR do?

  1. consolidates the "inference-models" / "embedding-model" / "judge-model" ... options in root conftest.py. Without this change, we will hit into error when trying to run pytest /Users/sxyi/llama-stack/llama_stack/providers/tests/. because of duplicated addoptions definitions across child conftest files.

  2. Add a config option to specify test config in YAML. (see ci_test_config.yaml for example config file)

For provider_fixtures, we allow users to use either a default fixture combination or define their own {api:provider} combinations.


memory:
....
  fixtures:
    provider_fixtures:
    - default_fixture_param_id: ollama // use default fixture combination with param_id="ollama" in [providers/tests/memory/conftest.py](https://fburl.com/mtjzwsmk)
    - inference: sentence_transformers
      memory: faiss
    - default_fixture_param_id: chroma

  1. generate tests according to the config. Logic lives in two places:
    a) in {api}/conftest.py::pytest_generate_tests, we read from config to do parametrization.
    b) after test collection, in pytest_collection_modifyitems, we filter the tests to include only functions listed in config.

Test Plan

  1. pytest /Users/sxyi/llama-stack/llama_stack/providers/tests/. --collect-only --config=ci_test_config.yaml

Using --collect-only tag to print the pytests listed in the config file (ci_test_config.yaml).

output: gist

  1. sanity check on --inference-model option
pytest -v -s -k "ollama" --inference-model="meta-llama/Llama-3.1-8B-Instruct" ./llama_stack/providers/tests/inference/test_text_inference.py

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Ran pre-commit to handle lint / formatting issues.
  • Read the contributor guideline,
    Pull Request section?
  • Updated relevant documentation.
  • Wrote necessary unit or integration tests.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Jan 7, 2025
@sixianyi0721 sixianyi0721 force-pushed the ci-test-new branch 5 times, most recently from b672bb0 to 0b3637f Compare January 7, 2025 08:16
@sixianyi0721 sixianyi0721 changed the title [ez][test automation] move addoption to root conftest and fix broken imports [test automation] support run tests on config file Jan 7, 2025
@sixianyi0721 sixianyi0721 marked this pull request as ready for review January 7, 2025 08:52
@sixianyi0721 sixianyi0721 force-pushed the ci-test-new branch 2 times, most recently from e64eac2 to 3d9d714 Compare January 7, 2025 18:00
@sixianyi0721 sixianyi0721 force-pushed the ci-test-new branch 5 times, most recently from 74bf48e to cd62656 Compare January 9, 2025 21:53
new_items, deselected_items = [], []
for item in items:
if item.fspath in required_tests:
func_name = item.name.split("[")[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like a somewhat fragile way to identify the function name. is there no other way?

- test_chat_completion_with_tool_calling
- test_chat_completion_with_tool_calling_streaming

inference_providers:
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be named inference_fixtures so it is clearer?

def try_load_config_file_cached(config_file):
if config_file is None:
return None
if CONFIG_CACHE is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need any caching? what was the reason you introduced it?

@@ -0,0 +1,76 @@
# Copyright (c) Meta Platforms, Inc. and affiliates.
Copy link
Contributor

Choose a reason for hiding this comment

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

we could put these functions all in the top-level conftest.py?

- inference/test_text_inference.py::test_chat_completion_with_tool_calling_streaming

fixtures:
provider_fixtures:
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a confusing thing -- why are there provider_fixtures within fixtures? also it is unclear why we have a "default_fixture_param_id" and what it indicates. Can you explain a bit?



class TestConfig(BaseModel):
inference: APITestConfig = Field(default=None)
Copy link
Contributor

Choose a reason for hiding this comment

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

all of these should be Optional and should look like

: Optional[...Config] = None



def get_provider_fixture_overrides_from_test_config(
config, api, default_provider_fixture_combination
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this could be named metafunc_config so it is clearer what it is

Copy link
Contributor

Choose a reason for hiding this comment

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

another nit: the last parameter should be a plural (combinations)

Copy link
Contributor

@ashwinb ashwinb left a comment

Choose a reason for hiding this comment

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

looks good, thanks for the iteration. a couple minor nits.

@sixianyi0721 sixianyi0721 merged commit c79b087 into main Jan 16, 2025
2 checks passed
@sixianyi0721 sixianyi0721 deleted the ci-test-new branch January 16, 2025 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants