Skip to content

chore: parallelize fixture generation#330

Open
MegaRedHand wants to merge 2 commits intomainfrom
improve-fixture-generation
Open

chore: parallelize fixture generation#330
MegaRedHand wants to merge 2 commits intomainfrom
improve-fixture-generation

Conversation

@MegaRedHand
Copy link
Copy Markdown
Collaborator

🗒️ Description / Motivation

This PR enables parallel fixture generation in the Makefile, and changes the CI to use auto-parallelization instead of a fixed 2.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 30, 2026

Greptile Summary

This PR parallelizes fixture generation by replacing the hardcoded -n 2 worker count with -n auto in the CI workflow and adding -n auto to the Makefile target. The change lets both environments use all available CPU cores automatically rather than a fixed value.

Confidence Score: 5/5

Safe to merge — the change is a one-line swap per file with no logic risk.

Both changes simply replace a fixed -n 2 worker count (or absent flag) with -n auto, which is a well-supported pytest-xdist option. No logic, caching, or contract changes are involved.

No files require special attention.

Important Files Changed

Filename Overview
.github/workflows/ci.yml Changed -n 2 to -n auto in the fill command for fixture generation; straightforward parallelism improvement.
Makefile Added -n auto flag to the leanSpec/fixtures target's fill invocation; aligns local fixture generation with CI behaviour.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[CI: cache-hit?] -- yes --> B[Skip fixture generation]
    A -- no --> C[Download prod keys]
    C --> D["uv run fill --fork=Devnet --scheme prod -o fixtures -n auto"]
    D --> E[Save fixtures cache]
    E --> F[Run tests]

    G[make leanSpec/fixtures] --> H["uv run fill --fork devnet --scheme=prod -o fixtures -n auto"]
    H --> I[Fixtures ready]
Loading

Reviews (1): Last reviewed commit: "ci: use automatic parallelization level ..." | Re-trigger Greptile

@github-actions
Copy link
Copy Markdown

🤖 Claude Code Review

PR #330 — chore: parallelize fixture generation

Overview: A minimal 2-line change that enables parallel fixture generation. The CI switches from a fixed -n 2 to -n auto, and the local Makefile target (which previously had no parallelism flag at all) gains -n auto as well.


Code Quality

The change is clean and focused. No unnecessary modifications.

Observations

-n auto semantics: This flag is passed to uv run fill, which is likely a pytest-xdist invocation. pytest-xdist -n auto maps to the number of logical CPUs via multiprocessing.cpu_count(). On a standard GitHub Actions runner (2 vCPUs) the CI change is effectively a no-op — output should be identical to -n 2. The meaningful win is on developer machines with more cores.

Pre-existing inconsistency (not introduced here): The two fill invocations still differ in fork flag style:

  • CI: --fork=Devnet (upper-case D, = separator)
  • Makefile: --fork devnet (lower-case d, space separator)

These produce the same result only if the tool treats the value case-insensitively. Worth aligning, but out of scope for this PR.

Determinism: Parallel fixture generation is safe as long as individual fixture files are independent (no shared mutable state between tests). For spec-test generators this is almost always true, but worth keeping in mind if fixture diffs appear non-deterministic in future cache misses.

Cache interaction (CI): The if: steps.cache-fixtures.outputs.cache-hit != 'true' guard means this runs only on cache misses. The faster generation is a good ergonomic improvement for those cases.


Verdict

The change is correct, safe, and a net improvement. The only actionable note is the pre-existing fork-flag casing inconsistency between CI and Makefile — worth a follow-up cleanup to make the two invocations identical except for the intentional differences.


Automated review by Claude (Anthropic) · sonnet · custom prompt

@github-actions
Copy link
Copy Markdown

🤖 Kimi Code Review

The changes are straightforward improvements to test fixture generation parallelism, but carry minor risks:

.github/workflows/ci.yml (line 123)

  • Changing from -n 2 to -n auto improves fixture generation performance by utilizing all available CPU cores.
  • Risk: If fill is memory-intensive, auto-detecting cores on GitHub Actions runners (typically 2-4 cores, 7GB RAM) could cause OOM kills if the per-process memory footprint is high. Monitor CI for "Exit code 137" (OOM) errors after merge.
  • Note: Consider capping at -n 4 or -n $(nproc) with a max limit if memory issues arise.

Makefile (line 35)

  • Adding -n auto is appropriate for local development where developers likely want maximum parallelism.
  • Pre-existing inconsistency: The Makefile uses --fork devnet (lowercase) while the CI uses --fork=Devnet (uppercase). If the fill tool is case-sensitive, this could cause divergent behavior. Consider aligning these (not necessarily in this PR).

General

  • No security implications; this only affects test infrastructure.
  • No consensus-layer logic changes.
  • Ensure the fill tool's output remains deterministic when run with varying parallelism (it should, as parallelism typically affects only execution order, not SSZ hash tree roots or test vectors).

Verdict: Approved with minor observation. Watch CI memory usage.


Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

@github-actions
Copy link
Copy Markdown

🤖 Codex Code Review

Findings

  1. .github/workflows/ci.yml:123 and .github/workflows/ci.yml:128: switching CI from -n 2 to -n auto makes fill failures from memory / FD pressure more likely, but the fixtures cache is still saved under always(). If fill dies after writing part of leanSpec/fixtures, that partial tree can be cached under the pinned key and reused on later runs. Because the spec tests walk whatever fixtures exist on disk (for example crates/blockchain/state_transition/tests/stf_spectests.rs:332), this can silently reduce consensus test coverage instead of failing loudly. Gate the cache save on success(), or generate into a temp dir and rename only after success.

  2. Makefile:35: leanSpec/fixtures is generated in place as a directory target. I reproduced that if the recipe fails after creating that directory, GNU make will treat the partial tree as up to date on the next make test, so developers can rerun against incomplete fixtures without regeneration. -n auto increases the chance of hitting that state on high-core machines. Safer pattern: write to a temp output dir and mv it into place on success, or explicitly clean leanSpec/fixtures on failure.

No Rust runtime / fork-choice / state-transition / SSZ / signature code is changed here, so I don’t see client-side consensus bugs in the diff itself beyond the fixture-generation failure modes above. I couldn’t run fill in this environment because leanSpec is external and network access is restricted.


Automated review by OpenAI Codex · gpt-5.4 · custom prompt

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