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.

Sign up for GitHub

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

Jump to bottom

[Test automation] create a CI config for running required tests #743

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

[Test automation] create a CI config for running required tests #743

wants to merge 5 commits into from

Conversation

sixianyi0721
Copy link
Contributor

@sixianyi0721 sixianyi0721 commented Jan 10, 2025 •
edited
Loading

What does this PR do?

Add basic inference testing to github ci

Test Plan

Please describe:
Test run: https://fburl.com/fomrxefq

The failed fireworks vision streaming test seems to be flaky - it passes 50/50 locally.

Sources

Please link relevant resources if necessary.

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 10, 2025
@sixianyi0721 sixianyi0721 force-pushed the ci branch 6 times, most recently from c2ef2e5 to d2f09e1 Compare January 13, 2025 04:06
@yanxi0830 yanxi0830 added this to the v0.1 milestone Jan 15, 2025
@sixianyi0721 sixianyi0721 force-pushed the ci_test_report branch 2 times, most recently from 01cfefe to f24793d Compare January 15, 2025 22:46
@sixianyi0721 sixianyi0721 changed the base branch from ci_test_report to main January 15, 2025 23:04
@sixianyi0721 sixianyi0721 force-pushed the ci branch 6 times, most recently from 459ca15 to 32c988b Compare January 17, 2025 02:05
@sixianyi0721 sixianyi0721 force-pushed the ci branch 2 times, most recently from 3d115c9 to 3d56121 Compare January 17, 2025 02:15
@sixianyi0721 sixianyi0721 force-pushed the ci branch 2 times, most recently from fb613e1 to e0a27cd Compare January 17, 2025 11:47
sixianyi0721
sixianyi0721 commented Jan 17, 2025
View reviewed changes
.github/workflows/publish-to-test-pypi.yml
@@ -241,4 +243,15 @@ jobs:
pytest -v -s --nbval-lax ./docs/notebooks/Llama_Stack_Building_AI_Applications.ipynb
pytest -v -s --nbval-lax ./docs/notebooks/Llama_Stack_Benchmark_Evals.ipynb

# TODO: add trigger for integration test workflow & docker builds
- name: Integration tests
if: always()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

execute integration tests even if previous steps failed

@sixianyi0721 sixianyi0721 marked this pull request as ready for review January 17, 2025 12:05
yanxi0830
yanxi0830 reviewed Jan 17, 2025
View reviewed changes
.github/workflows/publish-to-test-pypi.yml Outdated
- name: Integration tests
if: always()
run: |
pip install -U \
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we directly do llama stack build --template fireworks --image-type venv to install these dependencies?

@yanxi0830
Copy link
Contributor

image

For these 2 flaky test cases? It seems like an issue with fireworks vision model inference, should we add re-tries for these tests or disable them in CI?

@yanxi0830
Copy link
Contributor

yanxi0830 commented Jan 17, 2025 •
edited
Loading

Shall we add the client-sdk tests as well for fireworks & together? We can consolidate the providers and client-sdk tests as follow up.

LLAMA_STACK_CONFIG="./llama_stack/templates/fireworks/run.yaml" pytest -v tests/client-sdk/ --html=report.html --self-contained-html

LLAMA_STACK_CONFIG="./llama_stack/templates/together/run.yaml" pytest -v tests/client-sdk/ --html=report.html --self-contained-html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yanxi0830 yanxi0830 yanxi0830 left review comments

@ashwinb ashwinb Awaiting requested review from ashwinb ashwinb is a code owner

@hardikjshah hardikjshah Awaiting requested review from hardikjshah hardikjshah is a code owner

@dltn dltn Awaiting requested review from dltn dltn is a code owner

@raghotham raghotham Awaiting requested review from raghotham raghotham is a code owner

@dineshyv dineshyv Awaiting requested review from dineshyv dineshyv is a code owner

@vladimirivic vladimirivic Awaiting requested review from vladimirivic vladimirivic is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Milestone
v0.1
Development

Successfully merging this pull request may close these issues.
3 participants
@sixianyi0721 @yanxi0830 @facebook-github-bot