Skip to content

Forward the controller preview size, and validate --bind in the paired CLI - #9

Closed
Omair-Asdaq wants to merge 2 commits into
LedFx:mainfrom
Omair-Asdaq:fix/perf-tools
Closed

Omair-Asdaq wants to merge 2 commits into
LedFx:mainfrom
Omair-Asdaq:fix/perf-tools

Conversation

@Omair-Asdaq

Copy link
Copy Markdown

Two small fixes, plus one finding I deliberately did not fix.

--vis-pixels was ignored for none/default streams, but recorded as applied

worker_command always forwarded the full-stream pixel count, so a --vis-pixels value had no effect on the preview streams — while <output>.metadata.json recorded it as applied, meaning the report claimed a preview size the run never used. It now forwards args.pixels for full and args.vis_pixels otherwise, and the 81-pixel default is preserved.

Repro: Options(vis_pixels=1024) with a default stream forwarded --vis-pixels 81 before and 1024 now. full still uses args.pixels, and the argv for every other case I compared is byte-identical.

The paired CLI never validated --bind

matrix.py accepted any bind string, unlike options.py. A non-loopback bind therefore created the output directory and manifest and then failed every trial — the pixel runner rejects the same value anyway — leaving a half-written output directory behind. matrix.parse_args now applies the same --bind must be an IPv4 loopback address check, so it exits 2 before creating anything.

Repro: matrix ... --bind 192.0.2.1 --plan-only wrote a manifest and returned 0 before; it now exits 2 with no output directory. 127.0.0.1 and 127.0.0.2 are still accepted.

Not fixed: --baseline compares unlike measurement windows

scenario_key omits the measurement window, so --baseline will happily match rows measured over different --seconds values — contrary to the README, which promises that missing matches fail instead of silently comparing unlike workloads. The obvious fix (adding the row's seconds to the key) is wrong, and I'm flagging it rather than shipping it:

  • the row's seconds is the measured window (end - begin, a perf_counter delta), so two runs of the same command differ by about a millisecond;
  • with it in the key, matrix.paired_summary raises ValueError: Paired workloads differ on the first real pair — both rows come from one command line, but they record slightly different windows — so ledfx-paired-bench dies mid-campaign;
  • --baseline then fails even on identical workloads, and group_rows splits the repeats of one scenario into separate groups, turning the reported medians into single-sample values.

The identity should key the requested window instead — e.g. record args.seconds / args.warmup as their own row fields and key on those. That changes the row schema, so it needs a call on whether existing baseline files should be invalidated.

Tests

One regression test per fix; both fail on the current main and pass here. The suite is 26 passed, 3 failed, and the 3 failures are pre-existing and Windows-only — path separators in test_migration.py / test_integrity.py, plus one Path.as_uri() on a drive-relative path in test_integrity.py — untouched by these commits.

ruff and pyrefly aren't installed in my environment, so I couldn't run the lint/type gates; the changed lines follow the surrounding style (88 columns, one argument per line).

worker_command passed a literal 81 pixels for the none/default streams, so a controller-level --vis-pixels had no effect there while <output>.metadata.json still recorded it as an applied argument. Forward args.vis_pixels instead: the none/default preview stays 81 pixels by default and the full stream still uses args.pixels.
The paired driver validated pixels, repeats, durations, effects, streams and interpreters but accepted any --bind, so a bad address created the output directory and a manifest whose commands can never succeed and then failed every trial. Mirror the pixel runner's check so the invalid argument is reported once, with the same parser error and exit status.
@Omair-Asdaq
Omair-Asdaq marked this pull request as ready for review October 17, 2026 18:47
Copilot AI balanced review requested due to automatic review settings October 17, 2026 18:47

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Omair-Asdaq

Copy link
Copy Markdown
Author

no reply from mantainer I am closing this Pr

@Omair-Asdaq Omair-Asdaq closed this Nov 3, 2026
@Omair-Asdaq
Omair-Asdaq deleted the fix/perf-tools branch November 3, 2026 15:37
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