Skip to content

feat(xtest): shared benchmark preparation for local and CI runs - #626

Draft
dmihalcik-virtru wants to merge 1 commit into
DSPX-4372-s1-stat-fixesfrom
DSPX-4372-s2-local-prep
Draft

dmihalcik-virtru wants to merge 1 commit into
DSPX-4372-s1-stat-fixesfrom
DSPX-4372-s2-local-prep

Conversation

@dmihalcik-virtru

@dmihalcik-virtru dmihalcik-virtru commented Sep 28, 2026 •

Copy link
Copy Markdown
Member

Stage 2 of the six-stage split described in spec/DSPX-4372-t1.md (added in #625). Replaces the corresponding part of #621.

What this delivers on its own

A benchmark can be prepared and run locally by the same code path CI will use in stage 5, and the setup rules that govern it are encoded once.

  • xtest/perf/config.py — a pytest-free module that owns BenchConfig (moved out of perf/runner.py) and the request rules: SDK-name validation, the arm ceiling, duplicate rejection, finite positive budgets and timeouts, max_rounds >= min_rounds, and the 1500 * K / 2 budget default. Previously these lived in three disagreeing places — runner.BenchConfig.__post_init__, a lazy per-SDK select_arms that turned a typo into a silent skip, and a block of shell in xtest.yml.
  • Eager option validation — conftest.pytest_configure now checks every --bench* option before session setup. A bad --bench-threshold used to surface from the bench_config fixture, after payloads were built and CLIs installed.
  • otdf-sdk-mgr install benchmark — resolves an ordered list of refs (first is the reference), expands pr:N, rejects installed-path collisions before any build, installs releases and source refs by the right method, and writes a versioned manifest to xtest/sdk/benchmark.installed.json.
  • otdf-local benchmark parity — the platform-only service shape the bench job wants actually works: ec_tdf_enabled/hybrid_tdf_enabled are set on the platform config rather than only on the km instances, a missing services.kas.root_key is generated once where the whole fleet reads it, otdf-local env exports the manifest pointer plus OTDFCTL_HEADS, and platform discovery finds the xtest/platform/src/<ref>/ worktrees otdf-sdk-mgr actually creates (see below).

Defects fixed while porting

  • The provisioning CLI was a measured arm. conftest.load_otdfctl falls back through sdk/go/dist/main/otdfctl.sh to a bare system binary, so "otdfctl" meant "whatever the go arm happens to be" — the build under measurement provisioned the fixtures it was measured against. The manifest now records a separate otdfctl pin and that alone is exported.
  • OTDFCTL_HEADS is a JSON array, not a comma-separated list. conftest.py reads it with json.loads and xtest.yml with fromJson. A comma-joined value is silently discarded and the run drops back onto the fallback above. A test now performs the exact json.loads conftest does.
  • A release-pinned otdfctl was silently ignored by an installation_method == "source" filter, reinstating the fallback for exactly the runs that asked not to have it. installed_tag is the dist/ directory name for both methods.
  • kas.py accepted an empty root key. The platform now generates one, and a KAS started without one fails at start() with a message naming root_key instead of surfacing as cipher: message authentication failed at decrypt time, several minutes later.
  • Dist-path collision. cmd_tip strips sdk--/otdfctl--, normalize_version only v-prefixes, and refs.ref_slug only flattens slashes — so otdfctl/v0.24.0 and v0.24.0 both land in dist/v0.24.0, undetected. dist_slug is now the single slug function for dist paths and check_collision buckets on it.
  • A dist-path collision was defined too broadly, and the provisioning pin escaped it entirely. check_collision now flags a shared directory only when the artifacts sit at different commits — two aliases at one commit produce one build, so sharing is fine and for measured arms it is the neutral outcome. The --otdfctl pin is resolved alongside the arms and joins the check when it shares their dist/ tree, so a pin at a different commit can no longer overwrite a build the run is measuring. An unresolvable pin is also now reported before the arms are compiled rather than after.
  • A neutral preparation claimed an installed_path it had not installed. It recorded the directory if one happened to exist — routinely true from an earlier run — asserting a provenance the preparation never checked. It is empty now, unconditionally.
  • otdf-local up discarded Docker's reason for failing. DockerService.start captured compose's output and returned a bare False, so a run died with "✗ Failed to start Docker services" and nothing else; the only way to learn why was to rerun compose by hand. It now populates the start_error the base class already defines, and up prints it. Found by hitting it: the real message was error while creating mount source path ... chown ...: permission denied.
  • A manifest is written on every exit path, including partial and failed ones, because the failing paths are the ones where "what did it actually install" matters. status is one of success/neutral/partial/failed and status_message explains whichever it is — neutral is not an error, so the field is not named for one.

Dependencies and compatibility

  • Based on fix(xtest): test benchmark improvements on the faster tail #625 (stage 1). Merge that first; this PR's diff against it is 21 files.
  • No existing command changes behavior. install stable, install tip, and install release are untouched; install benchmark is new. otdf-local env gains variables and drops none. --bench-baseline/--bench-candidate keep their current meaning — they now fail fast on invalid input rather than late.
  • BenchConfig moved from perf.runner to perf.config and is re-imported in runner under the same name, so from perf.runner import BenchConfig still resolves.
  • No live service is contacted by anything added here.

Validation

Re-run after rebasing onto #625 at e4c7c8b0, its tip including the review corrections. The rebase was clean — this branch's diff against stage 1 is unchanged at 21 files, and its additions to perf/README.md are a pure append to the section stage 1 rewrote.

package ruff pyright tests
otdf-sdk-mgr clean 0 errors 175 passed
otdf-local clean 0 errors 59 passed, 1 skipped, 3 failed — all three @pytest.mark.integration, see below
xtest clean 0 errors 167 passed (bench suites)

test_ls_no_services, test_ls_json and test_clean_command require a platform checkout to exist somewhere discoverable: without one _find_platform_dir raises and otdf-local ls exits 1. They pass in a workspace where a platform is installed — which is the earlier green run, and is what the discovery fix below is for — and they fail in one where none is. Checking out main in this same workspace fails the same three identically, so nothing here regressed them; they are simply not runnable without a platform present.

Eager validation checked against real pytest invocations:

--bench --bench-threshold 0.9
  ERROR: invalid benchmark options: threshold is a ratio above 1.0, e.g. 1.15 for 15%
--bench-baseline go@main --bench-candidate java@main
  ERROR: invalid benchmark options: baseline and candidate must use the same SDK; got ['go', 'java']
--bench-baseline go@main
  ERROR: invalid benchmark options: both --bench-baseline and --bench-candidate are required, or neither; got only go@main
(no --bench)
  404/406 tests collected (2 deselected)  -- unaffected

Live preparation, actually run

install benchmark was run for real against this branch, not stubbed:

uv run otdf-sdk-mgr install benchmark go v0.38.0 main --platform main --otdfctl v0.38.1

It resolved and installed both arms (v0.38.0 as a release at 308002c2, main from source at e8a2f7f5), built the platform service from main, installed the provisioning CLI separately at v0.38.1 (99b51145), and wrote xtest/sdk/benchmark.installed.json with "status": "success". The source arm's dist/main/.version holds ref=main / sha=e8a2f7f5..., which is what the commit-reuse check reads.

That run also exposed two of the defects listed above — the discarded Docker error, and a unit test that passed only because nothing had ever been installed in the workspace.

Platform discovery, fixed here

otdf-sdk-mgr installs the platform source to xtest/platform/src/<ref>/, but
otdf_local.config.settings._find_platform_dir looked only for a platform/ directory
sibling to an ancestor of xtest — i.e. the hand-cloned tests/platform/. The two layouts
had drifted apart on main, so install benchmark --platform main put a platform somewhere
otdf-local up could not see and the sequence in perf/README.md worked only with
OTDF_LOCAL_PLATFORM_DIR set by hand. Discovery now knows both: the sibling checkout still
wins where it exists, several installed refs are an error naming OTDF_LOCAL_PLATFORM_DIR
rather than an arbitrary pick, and the bare platform.git is excluded by shape rather than
by name.

Two further gaps surfaced on the way to a live run and are fixed with it:

  • The installed worktrees have no opentdf.yaml. It is gitignored in the platform repo
    and made by hand in the manual setup (cp opentdf-dev.yaml opentdf.yaml); nothing in
    install platform creates it, so up died with a bare FileNotFoundError from
    load_yaml. It is now seeded once from the committed opentdf-dev.yaml — and the seeding
    has to happen before the first generation, because opentdf-dev.yaml is also where
    generation writes; without a separate pristine copy each run's output becomes the next
    run's input and the golden keyring entries accumulate a copy per run.
  • The platform's keys/ are not generated by any documented step, so Keycloak's bind
    mounts resolve to empty directories Docker then fails to chown. Run the platform's own
    .github/scripts/init-temp-keys.sh once. This one is left as an environment step, not
    code: it belongs to the platform checkout, not to otdf-local.

Live measured run

The full sequence from perf/README.md was executed end to end against real services — no
stubs, no OTDF_LOCAL_PLATFORM_DIR override:

otdf-local up --services docker,platform     # keycloak, postgres, platform all ready
eval $(otdf-local env)
pytest --bench --sdks go --bench-baseline go@v0.38.0 --bench-candidate go@main \
       --bench-min-rounds 5 --bench-max-rounds 10 --bench-warmup 2 -v test_benchmarks.py

7 cells executed, 0 skipped ("skipped": {} in the JSON), 10 rounds and 2 warmup rounds
each, every one stopping on max_rounds rather than the budget:

cell rounds outcome
go-encrypt-1MiB-control 10 A/A control, noise floor ±12.0%, not tripped
go-encrypt-1KiB 10 IMPROVED (wall, cpu, rss)
go-encrypt-1MiB 10 IMPROVED (wall, cpu)
go-encrypt-32MiB 10 IMPROVED (wall, cpu)
go-decrypt-1KiB 10 IMPROVED (wall, cpu)
go-decrypt-1MiB 10 IMPROVED (wall, cpu)
go-decrypt-32MiB 10 IMPROVED (wall, cpu)

13 improvements, 0 regressions, trustworthy: true, noise_floor.tripped: false. The
environment exported by otdf-local env carried XT_TMP_DIR, BENCH_INSTALLATION_MANIFEST
and OTDFCTL_HEADS='["v0.38.1"]' — the JSON-array form conftest parses — so the run
provisioned with the pinned CLI rather than with a build it was measuring.

The run also exercises #625's new code path. go-encrypt-1MiB/wall came back
ratio 0.493, ci_high 0.578, p_value 1.0, p_adjusted 1.0, p_value_faster 0.00098, p_adjusted_faster 0.00107 → IMPROVED, so both tails are computed and BH-adjusted
independently on a real measurement rather than only in the unit tests.

It does not, though, discriminate the old rule from the new one, and an earlier
version of this paragraph claimed it did. main's IMPROVED clause was
p_adjusted > 1 - alpha on the slower tail; p_adjusted is 1.0 here and
1.0 > 0.95 holds, so main would have called this cell IMPROVED too. That is
what the fix predicts. BH never lowers a p-value, so reading an adjusted
upper-tail p as evidence of improvement gets easier as a run grows: the defect
shows up as false IMPROVED verdicts on null data under multiplicity, not as
changed verdicts on a genuine 2x speedup. test_adjusted_slower_tail_is_not_read_as_evidence_of_improvement
in #625 is what covers the discriminating case.

@coderabbitai

coderabbitai Bot commented Sep 28, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown

X-Test Failure Report

✅ java@main-v0.27.0
✅ js@v0.4.0-v0.27.0
✅ js@v0.4.0-main

@github-actions

Copy link
Copy Markdown

One encoding of the benchmark's setup rules, reachable from a pytest run,
a local shell and (in stage 5) the workflow.

perf/config.py is a pure module -- no pytest import -- that owns BenchConfig
and the request rules that were previously split between runner.BenchConfig,
a lazy per-SDK select_arms that answered a typo with a skip, and a block of
shell in xtest.yml. conftest validates every --bench option in
pytest_configure, before payloads are built, so a bad threshold costs a
second rather than several minutes of setup.

otdf-sdk-mgr gains 'install benchmark': ordered request resolution that
keeps alias->commit multiplicity, pr:N expansion, installed-path collision
detection before any build, and a versioned manifest written on every exit
path including the failing ones. The platform pin and the provisioning
otdfctl pin are recorded apart from the measured arms, so the build under
measurement no longer provisions the fixtures it is measured against.

otdf-local starts the benchmark's platform-only shape correctly: ec and
hybrid wrapping are set on the platform config rather than only on the km
instances, a missing root key is generated once where every KAS reads it,
and a KAS with no key fails loudly at start instead of reporting 'cipher:
message authentication failed' at decrypt time. 'otdf-local env' exports the
manifest pointer and OTDFCTL_HEADS as the JSON array conftest parses.

It also finds and starts what otdf-sdk-mgr actually installs. Discovery
looked only for a 'platform/' checkout beside xtest/, while 'install
platform' creates xtest/platform/src/<ref>/ worktrees; several installed
refs are an error naming OTDF_LOCAL_PLATFORM_DIR rather than an arbitrary
pick. Those worktrees also lack the gitignored opentdf.yaml the manual setup
makes by hand, so it is seeded once from the committed opentdf-dev.yaml --
before generation overwrites that file, since otherwise each run's output
becomes the next run's input. Failed 'docker compose up' now reports what
compose said instead of only that it failed.

Refs DSPX-4372. Stage 2 of the split described in spec/DSPX-4372-t1.md.
@sonarqubecloud

Copy link
Copy Markdown

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.

1 participant