Skip to content

Split Examples test into mulitple jobs - #180

Merged
Yuge Zhang (ultmaster) merged 4 commits into
mainfrom
split-examples-test
Oct 20, 2025
Merged

Split Examples test into mulitple jobs#180
Yuge Zhang (ultmaster) merged 4 commits into
mainfrom
split-examples-test

Conversation

@ultmaster

Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI review requested due to automatic review settings October 20, 2025 06:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR splits the monolithic Examples test workflow into four separate, focused jobs to improve parallelization and reduce total execution time.

Key Changes:

  • Split single 120-minute job into four parallel jobs (calc-x, spider, apo, unsloth) with reduced individual timeouts
  • Consolidated dependency sync steps by combining stable and legacy paths
  • Created dedicated backward-compatibility job for legacy API testing

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

env:
OPENAI_BASE_URL: http://localhost:12306/
OPENAI_API_KEY: dummy
if: matrix.setup-script != 'legacy'

Copilot AI Oct 20, 2025

Copy link

Choose a reason for hiding this comment

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

The conditional check is inconsistent with other jobs. The APO job matrix includes 'legacy' but this step will skip it. Either remove 'legacy' from the matrix (lines 276-277) or update the conditional to match the intended behavior.

Copilot uses AI. Check for mistakes.
- name: Sync dependencies
run: |
uv sync --frozen --no-default-groups --extra apo --extra verl \
--group dev --group experiment --group agents --group torch-gpu-${{ matrix.setup-script }}

Copilot AI Oct 20, 2025

Copy link

Choose a reason for hiding this comment

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

The backward-compatibility job syncs both 'apo' and 'verl' extras but doesn't use the conditional pattern from other jobs to handle 'latest' vs 'stable/legacy'. This will use 'torch-gpu-latest' if a 'latest' matrix entry existed, which doesn't match the pattern established in calc-x and spider jobs where 'latest' uses 'torch-gpu-stable'.

Suggested change
--group dev --group experiment --group agents --group torch-gpu-${{ matrix.setup-script }}
--group dev --group experiment --group agents --group torch-gpu-${{ matrix.setup-script == 'latest' && 'stable' || matrix.setup-script }}

Copilot uses AI. Check for mistakes.
- name: Sync dependencies
run: |
uv sync --frozen --no-default-groups --extra verl \
--group dev --group experiment --group trl --group agents --group torch-gpu-stable

Copilot AI Oct 20, 2025

Copy link

Choose a reason for hiding this comment

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

The unsloth job hardcodes 'torch-gpu-stable' instead of using the matrix variable pattern. This differs from the approach in other jobs and won't respect the 'latest' vs 'stable' distinction if the matrix is expanded. Consider using '--group torch-gpu-${{ matrix.setup-script }}' for consistency, or add conditional steps like the other jobs.

Suggested change
--group dev --group experiment --group trl --group agents --group torch-gpu-stable
--group dev --group experiment --group trl --group agents --group torch-gpu-${{ matrix.setup-script }}

Copilot uses AI. Check for mistakes.
@ultmaster
Yuge Zhang (ultmaster) merged commit 895bffc into main Oct 20, 2025
24 checks passed
@ultmaster
Yuge Zhang (ultmaster) deleted the split-examples-test branch October 28, 2025 01:23
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.

2 participants