-
Notifications
You must be signed in to change notification settings - Fork 35
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 both pinned and unpinned versions of IREE dependencies #760
Comments
Brainstorming a few strategies for this... Possible strategiesA) Test with only pinned, use dependabot to send PRs that try new versionsThis would involve switching all workflows that install packages to use requirements files with pinned versions in them. Then we would have some automation (likely dependabot, but there are other options too) send pull requests at some regular frequency attempting to bump to the latest versions. References:
B) Add a
|
strategy: | |
matrix: | |
python-version: ["3.11", "3.12"] | |
torch-version: ["2.3.0", "2.4.1", "2.5.1"] | |
os: [ubuntu-24.04] | |
include: | |
- os: windows-2022 | |
python-version: "3.11" | |
torch-version: "2.3.0" | |
- os: windows-2022 | |
python-version: "3.12" | |
torch-version: "2.4.1" | |
exclude: | |
- python-version: "3.12" | |
# `torch.compile` requires torch>=2.4.0 for Python 3.12+ | |
torch-version: "2.3.0" | |
fail-fast: false |
We could add new variables for versions like
strategy:
matrix:
iree-requirements: ["requirements-iree-pinned.txt", "requirements-iree-nightly.txt"]
That would result in many more workflow jobs on each commit but would give us a complete view of job status for every event.
C) Fork workflows to run pinned/unpinned
Similar to how we have splits like .github/workflows/ci_eval.yaml
and .github/workflows/ci_eval_short.yaml
, we could have ci_sharktank_pinned.yml
and ci_sharktank_unpinned.yml
. That way, we would have separate run history pages like https://github.com/nod-ai/shark-ai/actions/workflows/ci_eval.yaml and https://github.com/nod-ai/shark-ai/actions/workflows/ci_eval_short.yaml.
D) Use reusable workflows to run pinned/unpinned
Reusable workflows (docs here) would be similar to (C), but without as much copy/paste. We could still set different triggers for each variant and track run histories separately.
Thoughts
I like the simplicity of (A), as this would let us bring up new workflows with minimal changes and keep all workflows predictable, with source code always being the source of truth for versions. Options (C) and (D) would give us independent workflow run history for each variant. I think we could simulate that with event/branch/actor filters and (A) though.
Note that for dependabot updates (A), jobs that only run nightly and not on individual pull requests would not pick up those changes by default. We could find a way to opt those PRs in to running those jobs, or go with one of the other options for those jobs.
Chatted with @marbre a bit. Leaning towards option (A) for workflows that run on The actual mechanism for (A) is TBD. I'm testing dependabot but having a hard time getting it to understand a single requirements.txt file that uses
Might instead write a workflow explicitly, like https://github.com/iree-org/iree/blob/main/.github/workflows/bump_torch_mlir.yml. Or, if it works, use Renovate (https://docs.renovatebot.com/). cc @stbaione @archana-ramalingam (I saw a separate discussion at #757 (comment)) |
We had discussed earlier about which workflows require pinned vs latest versions in this PR. If that works we can stick with it. The overarching idea is pre-submits use pinned versions and nightly use latest/nightly versions. |
I'm planning to have:
|
This simplification will help with #760. Pros: * Now there are fewer places that use a ref pin * Workflows are now simpler Cons: * ~~Workflows will be several seconds slower since FetchContent always fetches all submodules~~ * The `SHORTFIN_IREE_SOURCE_DIR` option is no longer tested
This is prep work for #760. I also considered putting the files under `build_tools/` or `shark-ai/`, but we already have a few requirements files in the repository root. Still not as many as https://github.com/vllm-project/vllm though 😛.
Progress on #760. The idea here is that we will test with only pinned versions in all workflows that run on `pull_request` and `push` triggers, then we will create pull requests (ideally via automation like dependabot) that attempt to update the pinned versions. This will give us confidence that test regressions are _only_ due to the code changes in the pull request and not due to a dependency changing. Workflows will also be more reproducible as the versions they fetch will come from source code and not an external, time-dependent source.
This simplification will help with #760. Pros: * Now there are fewer places that use a ref pin * Workflows are now simpler Cons: * ~~Workflows will be several seconds slower since FetchContent always fetches all submodules~~ * The `SHORTFIN_IREE_SOURCE_DIR` option is no longer tested
This is prep work for #760. I also considered putting the files under `build_tools/` or `shark-ai/`, but we already have a few requirements files in the repository root. Still not as many as https://github.com/vllm-project/vllm though 😛.
Progress on #760. The idea here is that we will test with only pinned versions in all workflows that run on `pull_request` and `push` triggers, then we will create pull requests (ideally via automation like dependabot) that attempt to update the pinned versions. This will give us confidence that test regressions are _only_ due to the code changes in the pull request and not due to a dependency changing. Workflows will also be more reproducible as the versions they fetch will come from source code and not an external, time-dependent source.
Made good progress on this. Remaining tasks:
|
See #760 for context. We want to stay close to the latest versions while still pinning versions for predictability. Updating version pins is currently a manual process but we plan on automating it in the future. We can decide how noisy we want these dependency updates to be: * new PRs daily or less frequently * do or don't reuse existing PRs * merge ASAP or let them sit for multiple days
Progress on #760. We could make the scheduled jobs test both pinned and unpinned versions like on #767. Cleanup included here: * Dropped the "Installing the PyTorch CPU wheels saves multiple minutes and a lot of bandwidth on runner setup." comments since they are repetitive. Could add them back if people find them useful. * Stopped installing from the root `requirements.txt` in some workflows, instead opting to just install from the more specific `sharktank/requirements-tests.txt` I did not test the changes to scheduled workflows. Could do that on request, or just revert if we see issues.
Progress on #760, built off of the work in iree-org/iree-turbine#388. This adds a new workflow that runs once a day to update all pinned IREE versions. I also looked into using Dependabot but found that it struggles with `--find-links`, `--index-url`, and with there being multiple `requirements.txt` files in a repository. While I would love to not need to reinvent this wheel, I do like keeping full control over the process. This PR includes: * A new `build_tools/update_iree_requirement_pins.py` script handles updating the pins in `requirements-iree-pinned.txt` and `shortfin/CMakeLists.txt`. The script also sets some variables in `GITHUB_ENV`. * A new `.github/workflows/update_iree_requirement_pins.yml` workflow runs that script then calls https://github.com/peter-evans/create-pull-request to create or update a pull request if there are local changes after running that script. The commit message and pull request body are constructed using the variables set by the script. Test action run: https://github.com/ScottTodd/shark-ai/actions/runs/12777789320 Test pull request: ScottTodd#1
We have many workflows testing only unpinned versions of the IREE packages:, such as:
shark-ai/.github/workflows/ci-sharktank.yml
Lines 79 to 84 in 5f08cb2
Some workflows explicitly test pinned versions like
shark-ai/.github/workflows/ci-sglang-benchmark.yml
Lines 71 to 75 in 5f08cb2
Having inconsistent version pinning results in fragmented PRs updating pins like #757, #746, and #721.
We should further consolidate where version pins are defined and also refactor some workflows to test both pinned and unpinned versions. Workflows that pin the versions can be marked as "required checks" and give us confidence that workflow failures are a result of the changes in a PR/commit and not due to changes in dependencies. We do still want early and regular signal for upcoming API breaks, regressions, and other issues coming from dependencies though, so these versions of workflows could still run on pull requests or at least on schedules (e.g. nightly).
The text was updated successfully, but these errors were encountered: