8000 fix(ci,tests): propagate dynamic workunits count to verification scripts by janzak-dev · Pull Request #987 · google/xmc · GitHub
[go: up one dir, main page]

Skip to content

fix(ci,tests): propagate dynamic workunits count to verification scripts - #987

Merged
janzak-dev merged 2 commits into
mainfrom
fix/dynamic-workunits-propagation
Sep 28, 2026
Merged

janzak-dev merged 2 commits into
mainfrom
fix/dynamic-workunits-propagation

Conversation

@janzak-dev
@janzak-dev janzak-dev commented Sep 25, 2026 •
Copy link
Copy Markdown
Collaborator

Summary

Propagates dynamic num_work_units configuration from GitHub Actions workflow_dispatch manual triggers to downstream verification scripts (simple_job_verify and simple_job_ui_verify), preventing assertion timeouts when non-default WorkUnit counts (e.g., 2) are launched.

It used to fail when you specified a number of workunits different than the default one https://github.com/google/xmc/actions/runs/36124267073/job/108036661702

Motivation & Context

In manual dispatch runs of nightly-test-suite.yml, users can specify custom WorkUnit counts (e.g., num_work_units: 2). While the workload launcher honored this setting, the SDK and UI verification steps lacked --expected_work_units flags and environment variable fallbacks, causing them to fall back to hardcoded defaults (3 WorkUnits) and time out waiting for non-existent units.

Key Changes

  • CI Workflow (.github/workflows/nightly-test-suite.yml): Explicitly passes --expected_work_units="${NUM_WORK_UNITS}" to both tools.nightly_tests.simple_job_verify and tools.nightly_tests.simple_job_ui_verify.
  • Common Verification Utilities (tools/nightly_tests/common.py): Added resolve_expected_work_units(default) which checks FLAGS.expected_work_units, falls back to os.environ.get("NUM_WORK_UNITS"), validates that values are strictly positive (> 0), and defaults to 3.
  • Verification Scripts (tools/nightly_tests/simple_job_verify.py & simple_job_ui_verify.py): Parametrized expected WorkUnit counts and expected worker names (tuple(str(i) for i in range(1, expected_work_units + 1))).
  • Unit Tests (tools/nightly_tests/common_test.py): Added test coverage for resolve_expected_work_units covering flag precedence, environment fallback, non-positive integer filtering, and default values.

Assumptions

None — requirements were fully specified.

Verification & Testing

  • Commands Executed:
    • bazel test //tools/nightly_tests:all (4/4 tests passed)
    • ./.githooks/pre-commit (all linters passed)
    • ./tools/analyze_diff_size.py --base-ref origin/main... (0 threshold exceedances)
  • Live E2E Verification with Overridden WorkUnits:
    • Successful GitHub Actions Run: Run #36125990744 (Job #108042119449)
    • Configuration: Triggered via workflow_dispatch on branch fix/dynamic-workunits-propagation with num_work_units: 2.
    • Results:
      • Launch: Created 2 WorkUnits (demo-worker-1, demo-worker-2) on XID 24066.
      • SDK Assertions: Successfully asserted Expected WorkUnits Count: 2 and verified 2/2 WorkUnits completed on cpu-queue.
      • UI Assertions: Playwright traversal successfully asserted header title Demo Workload (2 units, 60s execution) and verified 2 WorkUnits rendered in the DOM.

Related Issues / Bugs

N/A

TAG=agy
CONV=df3c1c34-924a-42d3-81dc-9755d79b93f5

Ensure custom num_work_units specified in workflow_dispatch triggers is
propagated to simple_job_verify and simple_job_ui_verify. Add environment
fallback in common.resolve_expected_work_units and pass explicit CLI flags
in nightly-test-suite.yml.

TAG=agy
CONV=df3c1c34-924a-42d3-81dc-9755d79b93f5
@gemini-code-assist gemini-code-assist Bot left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

8000

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

Code Review

This pull request introduces a helper function resolve_expected_work_units to dynamically resolve the expected work units count from a command-line flag, the NUM_WORK_UNITS environment variable, or a default fallback value. This helper is integrated into both the UI and backend verification scripts, and corresponding unit tests are added. The feedback suggests validating that the resolved work units count is strictly positive to prevent downstream issues, along with adding test cases to cover non-positive input scenarios.

Comment thread tools/nightly_tests/common.py Resolved
Comment thread tools/nightly_tests/common_test.py Resolved
…cted_work_units

Address reviewer feedback on PR #987 by ensuring non-positive values
(<= 0) passed via flag or NUM_WORK_UNITS env var are ignored and fall back
to default. Also add corresponding unit test assertions.

TAG=agy
CONV=df3c1c34-924a-42d3-81dc-9755d79b93f5
@janzak-dev
janzak-dev marked this pull request as ready for review September 25, 2026 10:57
@janzak-dev janzak-dev added the priority::3-medium needs review. label Sep 25, 2026
@github-actions
github-actions Bot requested a review from AntonTyb September 25, 2026 10:57
@janzak-dev
janzak-dev requested review from andrzejfudro and removed request for AntonTyb September 25, 2026 10:58
@janzak-dev
janzak-dev added this pull request to the merge queue Sep 28, 2026
Merged via the queue into main with commit 28260a0 Sep 28, 2026
12 checks passed
@janzak-dev
janzak-dev deleted the fix/dynamic-workunits-propagation branch September 28, 2026 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority::3-medium needs review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

0