UN-3636 [PERF] Stop the test rig wasting CPU and hashing time - #2195
Merged
Conversation
load_groups() now merges extra group manifests listed in the env var (os.pathsep-separated, REPO_ROOT-relative) onto the base tests/groups.yaml before validation, so cross-manifest depends_on and the platform-gate invariant are checked over the union. Name collisions are an error. Lets a downstream repo (the cloud build) contribute its own test groups by copying a groups.cloud.yaml into the merged tree, without editing the OSS manifest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz
UserContext.get_organization() ran Organization.objects.get() even with no org id in StateStore (import time, or management commands with no request), catching only DoesNotExist/ProgrammingError — a DB-less/unmigrated setup hit an uncaught OperationalError. Short-circuit when there's no org id: no query, so serializers/managers that reference org-scoped querysets at class-def can be imported during DB-free test collection.
Address review feedback on the UNSTRACT_RIG_EXTRA_MANIFESTS overlay: - Overlays now apply only when loading the default manifest, so an explicit `load_groups(path)` (test fixture, ad-hoc manifest) can no longer absorb a downstream repo's ambient overlay. - `_merge_manifest` returns the merged defaults so an overlay can rename `platform_gate_group` instead of having it silently ignored. - A bad path in the env var raises a ValueError naming the variable rather than a bare FileNotFoundError/IsADirectoryError. - Extract `_load_manifest_dict` to single-source manifest parsing and its error message. - Tests: drive the real default-manifest path; cover overlay isolation, overlay defaults, malformed overlay, and a missing overlay path. - Pin the truthy branch of UserContext.get_organization so inverting the guard can't pass unnoticed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz
`--tier X` expanded each selected group's `depends_on` transitively, with no tier bound. `integration-workflow-execution` and `e2e-smoke` both declare `depends_on: [unit-sdk1, unit-workers]`, so those two unit groups ran again in the integration leg and a third time in the e2e leg. Tiers run as separate CI legs and the unit leg already covers them, so dep expansion is now bounded to the requested tier. Explicitly named groups are never dropped, and intra-tier deps (e2e-smoke -> e2e-login) still expand and order as before. Unrun deps do not weaken gating: `blocked_by` intersects with groups that failed in the same run. Measured on the last main run: ~88s of unit-workers and ~48s of unit-sdk1 re-executed per run. On the cloud CI runner the same duplication costs ~350s. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz
* UN-3636 [FIX] Drop the ENVIRONMENT gate on the LLM mock It did not defend the case it was added for. The threat was a worker env block copied out of the test overlay into a real deployment, but the gate was written into that same block, so a copy carries it. Base compose also sets ENVIRONMENT=development on both workers that run the injection, so any deployment derived from it satisfied the gate regardless. That left one real case -- the mock var set alone somewhere that sets no ENVIRONMENT at all -- which holds by accident rather than design, in exchange for depending on a variable nothing else in the codebase reads. What actually guards the hatch is unchanged: it is off unless someone sets UNSTRACT_LLM_MOCK_RESPONSE, and it warns once per process while active. Making mocked spend distinguishable downstream is the defence worth having, and it belongs on the usage record rather than in a config check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3636 [MISC] Drop the unread ENVIRONMENT variable from compose Nothing reads it: no service, worker, frontend or plugin looks the variable up, and the one consumer it ever had — the LLM mock gate — was removed in the previous commit. Dropping it everywhere keeps a dead knob from looking load-bearing to the next reader. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ryg9chVDJQggCybpq3YoY3 --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Drop lines that restate the code, trim session-specific detail, and merge comments that duplicated each other across a module and its test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz
`load_groups(DEFAULT_MANIFEST)` merged overlays even though the caller named a manifest explicitly, because the check compared path values. Path equality is also spelling-sensitive, so the same file relative and absolute behaved differently. Key on `path is None` instead: an explicit path loads exactly what it names. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ryg9chVDJQggCybpq3YoY3
TestIdePromptComplete drives the full success path, which reaches
client_plugin_registry.get_client_plugin("subscription_usage"). In OSS no
such plugin is installed, so the lookup returns None instantly. In a tree
with the cloud plugins copied in, it resolves to a real plugin that POSTs
to the backend; with no backend running the call only fails after a
multi-second connect timeout, and _track_subscription_usage swallows the
error so the tests still pass. That accounted for ~190s of the cloud
unit-workers run.
The file already declared _PATCH_GET_PLUGIN but never applied it outside
the dedicated subscription-usage classes. Apply it as a class-scoped
fixture so the lookup is pinned to the OSS answer.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz
Three independent wins measured on the cloud-merged tree: - `-n auto` collapsed to a single worker on any group shipping psutil, because xdist prefers physical cores there. Resolve the count in the rig instead, capped at 8 to avoid contending on the test database. - `--no-migrations` builds the schema from the models rather than replaying the full migration history once per xdist worker. - Test fixtures were paying 600k-iteration PBKDF2 per seeded user. integration-backend fell from 163s to ~52s with an identical result set. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
Summary by CodeRabbit
WalkthroughTest execution settings, CI caching, rig manifest loading, worker defaults, and per-group ChangesTest infrastructure
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Environment
participant RigCLI
participant load_groups
participant Pytest
Environment->>RigCLI: provide worker and group environment settings
RigCLI->>load_groups: load base and optional manifests
load_groups-->>RigCLI: return validated groups
RigCLI->>Pytest: execute groups with merged PYTHONPATH and worker count
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
chandrasekharan-zipstack
marked this pull request as ready for review
July 23, 2026 08:11
Contributor
|
| Filename | Overview |
|---|---|
| tests/rig/cli.py | Resolves a capped worker count and preserves group-specific import paths during test execution. |
| backend/pyproject.toml | Configures pytest-django to construct test schemas without replaying migrations. |
| backend/backend/settings/test.py | Uses Django's fast MD5 password hasher exclusively in test settings. |
| tests/rig/groups.py | Rejects manifests whose top-level groups value is not a mapping. |
| backend/dashboard_metrics/tests/test_tasks.py | Runs cleanup-task tests using Django TestCase instead of TransactionTestCase. |
| unstract/sdk1/tests/utils/test_retry_utils.py | Replaces real retry sleeps with an autouse test fixture. |
| .github/workflows/ci-test.yaml | Enables uv dependency caching across CI test and report jobs. |
Reviews (4): Last reviewed commit: "UN-3636 [MISC] Drop stale-prone CI comme..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/rig/groups.py`:
- Around line 171-178: Update _load_manifest_dict to validate that raw["groups"]
is a mapping before returning the manifest. Raise the existing intended
ValueError for missing or non-mapping groups values, while preserving valid
manifest handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 03694465-9bfb-4e49-9956-f296f71d424b
📒 Files selected for processing (15)
backend/backend/settings/test.pybackend/pyproject.tomlbackend/utils/tests/test_user_context.pybackend/utils/user_context.pydocker/docker-compose.yamltests/README.mdtests/compose/docker-compose.test.yamltests/rig/cli.pytests/rig/groups.pytests/rig/selection.pytests/rig/tests/test_groups.pytests/rig/tests/test_selection.pyunstract/sdk1/src/unstract/sdk1/llm.pyunstract/sdk1/tests/test_mock_response.pyworkers/tests/test_ide_callback.py
💤 Files with no reviewable changes (3)
- docker/docker-compose.yaml
- unstract/sdk1/src/unstract/sdk1/llm.py
- unstract/sdk1/tests/test_mock_response.py
Second performance pass on the test rig: - unit-workers-cloud ran zero tests: its group `PYTHONPATH` overwrote the rig-injected plugin dir, so `-p rig_critical_path` failed to import. Merge the two instead of letting env.update clobber it. - Drop `-s` from backend addopts — it disabled capture and flooded the log. - Persist uv's cache across runs via setup-uv enable-cache, so per-group `uv sync` links from cache instead of refetching. - No-op the real backoff sleeps in the sdk retry tests (~7s -> ~1s); no test asserts on elapsed time. - TestCleanupTasks needs no transaction semantics; TestCase over TransactionTestCase drops the per-test truncate-and-reseed. - Reject a non-mapping `groups:` manifest instead of crashing later. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz
# Conflicts: # tests/rig/groups.py
|
Contributor
Unstract test resultsPer-group results
Critical paths
|
athul-rs
self-requested a review
July 23, 2026 12:10
athul-rs
approved these changes
Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Three independent, test-only changes to how the rig runs the backend suites. No product code touched.
-n autotests/rig/cli.py--no-migrationsbackend/pyproject.tomlbackend/backend/settings/test.pyWhy
Worker count.
-n autocalls xdist'spytest_xdist_auto_num_workers, which preferspsutil.cpu_count(logical=False)and only falls back tosched_getaffinitywhen psutil is absent. psutil is present in thebackendandworkersvenvs, so on a hyperthreaded runner those groups see the physical core count — often 1 — and collapse to a single worker, while groups without psutil get the full thread count. The three largest suites (unit-workers,unit-backend,integration-backend) are exactly the ones that lose their parallelism. Onlygw0/gw1ever appear in the logs.-n logicalwould fix that but oversubscribe developer machines. Measured on a 12-core box,integration-backend: 2w=95.1s, 4w=79.4s, 8w=89.4s, 12w=121.4s — past ~4 workers the suites contend on the test DB more than they parallelise. So the rig counts usable CPUs itself (sched_getaffinity, honouring pinning) and caps at 8.Migrations. pytest-django replays the full migration history into a fresh database per xdist worker.
--no-migrationsbuilds the schema straight from the current models instead. The final schema is identical to the post-migration schema (Django'smakemigrations --checkkeeps models and migrations in sync), so tests see the same tables and columns — they just skip the historical replay. The one thing it drops is data migrations (RunPythonseed rows); nothing in these suites depends on migration-seeded data (verified: identical pass set), and tests that need rows create them via fixtures/factories. Payoff scales with the number of migrations, so it grows on larger app trees.Hasher. No
PASSWORD_HASHERSoverride existed anywhere underbackend/settings/, so everycreate_user()in a fixture paid Django's default 600k-iteration PBKDF2 — ~120ms a call, and the permissions/owner-management suites seed several users per test.Measured
integration-backend, 4 workers,104 passedidentical in every arm:+ --no-migrations+ MD5 hasherThe hasher dominates on this tree because the integration suites seed many users;
--no-migrationscontributes more as the migration count grows. The xdist change is orthogonal — it recovers the lost workers on CI runners that report a single physical core.Verification
On this branch:
tox -e groups -- integration-backend→104 passed, 26 skipped, 1 xfailedtox -e groups -- unit-backend→163 passedtox -e groups -- unit-rig→86 passed🤖 Generated with Claude Code
https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz