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

Reduce code duplication for ci-llama tests #1031

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

erman-gurses
Copy link

Currently it is a trial using action.yml

@erman-gurses erman-gurses requested review from ScottTodd and marbre March 4, 2025 03:10
@ScottTodd ScottTodd requested a review from renxida March 4, 2025 17:33
Copy link
Member

@marbre marbre left a comment

Choose a reason for hiding this comment

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

Some first comments.

shell: bash
run: python -m venv ${{ inputs.venv-dir }}

- name: Install Dependencies
Copy link
Member

Choose a reason for hiding this comment

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

You might want to cache those.

Copy link
Author

@erman-gurses erman-gurses Mar 5, 2025

Choose a reason for hiding this comment

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

Could you please elaborate with an example?

Copy link
Member

Choose a reason for hiding this comment

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

If using UV you could look at

- name: Setup UV caching

and
- name: Cache UV packages

where the actions/cache action is used. The first step is normally skipped when using pip but could be considered as well. In addition to that, the setup-python action also comes with builtin cache support, see https://github.com/actions/setup-python?tab=readme-ov-file#caching-packages-dependencies. Thus there are multiple options and we use several of them in the different workflows.

@renxida
Copy link
Contributor

renxida commented Mar 5, 2025

The tests are currently failing but this should be good to merge if it's the same failure as on main. LGTM but I think you should get @ScottTodd 's review as well.

This might come in as a separate PR, but might be a good idea to eventually merge this action with

https://github.com/nod-ai/shark-ai/blob/main/.github/actions/pkgci-setup/action.yml

and make the different behavior between the two depend on an option
such that if we e.g. add caching / migrate to UV, changing one place suffices.

Copy link
Member

Choose a reason for hiding this comment

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

This might come in as a separate PR, but might be a good idea to eventually merge this action with

https://github.com/nod-ai/shark-ai/blob/main/.github/actions/pkgci-setup/action.yml

and make the different behavior between the two depend on an option such that if we e.g. add caching / migrate to UV, changing one place suffices.

Let's do that here.

Our integration test workflows should have the same architecture:

  1. Sanity check the runner environment and perform any necessary bookkeeping
  2. Prepare the environment by installing shark-ai packages
  3. Run tests/benchmarks
  4. Report test/benchmark results

The "install" part of step 2 is critical for an integration test. Installing should be either from stable releases, nightly releases, or dev releases (e.g. pkgci.yml). Installing should not be using pip install -e with editable sources.

See also https://iree.dev/reference/bindings/python/#prebuilt-packages. The concepts are the same here.


I have three concrete suggestions for the "quick" llama tests, in order of preference:

  1. Get the llama tests running as part of
    - name: Run LLM Integration Tests
    run: |
    source ${VENV_DIR}/bin/activate
    pytest -v --test_device=${{ matrix.test_device }} \
    --junitxml=integration-test-${{ matrix.name }}.xml \
    app_tests/integration_tests/llm/shortfin/open_llama_3b_llm_server_test.py \
    --log-cli-level=INFO
  2. Add a new job to https://github.com/nod-ai/shark-ai/blob/main/.github/workflows/pkgci_shark_ai.yml
  3. Add a new workflow in the style of https://github.com/nod-ai/shark-ai/blob/main/.github/workflows/pkgci_shark_ai.yml and call it from https://github.com/nod-ai/shark-ai/blob/main/.github/workflows/pkgci.yml

I would strongly prefer option (1) there. There should be a unified way to run all tests across the project. Fragmenting across different test commands and workflows is going to be an ongoing source of complexity and confusion. Option (2) keeps the workflows relatively simple while allowing for different pytest commands and other job steps. Option (3) allows for more custom workflow code per test type.

As for the "large" tests, I could see either a single "nightly_ci" workflow like "pkgci" that uses a common trigger to launch subjobs or the status quo of individual workflows that each have their own scheduled triggers. In either case, those workflows should be using a setup action that either builds and installs dev packages (à la pkgci.yml) or installs nightly packages. Only unit tests and package workflows should be building the projects from source. Integration tests should be only using already built packages.

@erman-gurses erman-gurses force-pushed the users/erman-gurses/eliminate-code-duplication branch from d334ca2 to a7f61d6 Compare March 6, 2025 03:12
Signed-off-by: erman-gurses <[email protected]>
@renxida
Copy link
Contributor

renxida commented Mar 8, 2025

Got a question. Can this be pkgci rather than CI?

So that we only build the packages once per PR?

@erman-gurses erman-gurses force-pushed the users/erman-gurses/eliminate-code-duplication branch 5 times, most recently from bb8e90b to 40a9dad Compare March 10, 2025 06:28
@erman-gurses erman-gurses force-pushed the users/erman-gurses/eliminate-code-duplication branch from 40a9dad to b2dcb3e Compare March 10, 2025 06:34
@ScottTodd
Copy link
Member

Got a question. Can this be pkgci rather than CI?

So that we only build the packages once per PR?

Yes. All of my suggestions in #1031 (comment) involve moving into pkgci.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants