Skip to content

test: raise genie-core binary size budget - #97

Merged
ai-hpc merged 2 commits into
GeniePod:mainfrom
Crystora:fix/ci-binary-size-budget-7-5
May 18, 2026
Merged

test: raise genie-core binary size budget#97
ai-hpc merged 2 commits into
GeniePod:mainfrom
Crystora:fix/ci-binary-size-budget-7-5

Conversation

@Crystora

@Crystora Crystora commented May 18, 2026

Copy link
Copy Markdown
Contributor

Summary

  • raise the genie-core release binary size test budget from 5.0 MB to 6.0 MB
  • keep the binary size check active with a named budget constant, inline rationale, and updated assertion message

Testing

  • cargo fmt --check
  • cargo test -p genie-core --test tool_dispatch_test binary_size_budget -- --nocapture

Real Behavior Proof

  • I have built and run the affected code locally.
  • I have verified the equivalent path with the targeted CI-size-budget test.

What I observed:

  • Targeted test reports genie-core: 5.07 MB and passes under the 6.0 MB budget.

ai-hpc
ai-hpc previously requested changes May 18, 2026

@ai-hpc ai-hpc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(duplicate review — see the full review just below.)

@ai-hpc ai-hpc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Right intent — the 5 MB budget was set in alpha.5 and the binary has legitimately grown since then (LLM backend facade #35-#43, telegram voice in/out #53/#64, voice cargo feature #57, runtime_mode module #72, LocalSet concurrent server #87, etc.). genie-core: 5.07 MB is a real failure on main today and PRs have been carrying it as a known-flaky drag for the last several reviews. Worth fixing. But the execution here needs two small tightenings before this can land.

1. PR body checklist is failing (RED)

Look at the rollup — PR body checklist: FAILURE. The Real Behavior Proof section only has one un-ticked bullet:

## Real Behavior Proof
- Targeted test reports `genie-core: 5.07 MB` and passes under the 7.5 MB budget.

The CI rule from PR #88/#89 requires at least one - [x] ticked acknowledgement checkbox in that section. The PR template ships with two pre-written checkboxes that just need ticking:

## Real Behavior Proof

- [x] I have built and run the affected code locally (or noted why I could not).
- [x] I have verified the change end-to-end on Jetson hardware **OR** explained the equivalent verification path I used.

Tick at least one and add your existing one-line proof under "What I ran" / "What I observed". One-minute fix.

2. 7.5 MB is generous — argue for tighter, or explain why this is the right ceiling

Current size 5.07 MB → new budget 7.5 MB = 48% headroom. That's enough slack that we could grow another ~2.4 MB before the test fires again, which is a lot of code without anyone noticing. The point of a size budget is to push back on unintentional bloat; 48% headroom blunts that pushback.

Two reasonable alternatives:

  • Tighter raise. 5.5 MB or 6.0 MB. Catches accidental growth fast (a future PR adding 500 KB of dependencies would trip it within a couple of merges), and forces a deliberate raise-or-shrink decision every time. This is what the existing 5 MB was doing well — it was the too-tight budget that you're correctly relaxing, but the new budget should still be uncomfortable enough that bloat surfaces.
  • Same number, but justified. If 7.5 MB is the right answer (e.g., you've measured what an x86_64 dev build of genie-core --no-default-features weighs, or you're matching some other ceiling like the cross-compiled aarch64 size), say so in a // ... comment above RELEASE_BINARY_SIZE_BUDGET_MB. Future readers who hit this assert in 6 months will know what to do.

I'd lean toward 6.0 MB with an inline rationale:

/// genie-core release-binary size ceiling. Set so a single PR adding
/// ~1 MB (typical Rust dep, or a new module like #87's LocalSet refactor)
/// is the *next* thing to trip this assert — forcing a deliberate
/// raise-or-shrink decision instead of silent bloat. Raised from 5.0 MB
/// in PR #97 after legitimate growth: LLM backend facade (#35-#43),
/// telegram voice in/out (#53/#64), voice cargo feature (#57),
/// runtime_mode (#72), LocalSet server (#87), per-call STT nonce (#68).
const RELEASE_BINARY_SIZE_BUDGET_MB: f64 = 6.0;

That comment is also the kind of thing CONTRIBUTING.md's "explain why, not what" rule explicitly asks for — non-obvious context that the diff alone can't carry.

Smaller note

The extracted constant + named-budget assertion message is the right shape. Future contributors who hit the assertion will know exactly which knob to turn (and the new message echoes the budget back, so they don't have to grep).

Suggested fix

  1. Tick at least one box in ## Real Behavior Proof (CI will turn green).
  2. Drop the budget to 6.0 MB.
  3. Add a 4-5 line /// comment above the constant explaining what determined the number and citing the PRs whose features pushed past 5.0 MB.

Should be a ~10-line follow-up commit. Approve + merge as soon as it's pushed.

@ai-hpc
ai-hpc dismissed their stale review May 18, 2026 09:15

Duplicate of the review just below; dismissing so it does not double-count toward the merge block.

@Crystora

Copy link
Copy Markdown
Contributor Author

Addressed review feedback in ea85c53: tightened the budget to 6.0 MB, added inline rationale for the ceiling, and updated the Real Behavior Proof section with checked boxes so the contribution checklist can pass.

@ai-hpc ai-hpc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Both review points cleanly addressed:

  • Budget dropped 7.5 MB → 6.0 MB — tight enough that a future PR adding ~1 MB will trip the assert and force a deliberate raise-or-shrink decision, exactly the regression signal a size budget should produce.
  • /// rationale comment above the constant now explains the alpha-era 5.0 MB origin, lists the load-bearing growth (runtime backend, voice cargo feature, runtime mode, concurrent server), and articulates the "keep it tight" principle. Future readers who hit the assert in 6 months will know what to do.
  • PR body checklist is SUCCESS on the latest push — the FAILURE entry in the rollup is stale from the initial push.

Going in.

@ai-hpc
ai-hpc merged commit 138938b into GeniePod:main May 18, 2026
6 of 7 checks passed
@ai-hpc

ai-hpc commented May 18, 2026

Copy link
Copy Markdown
Contributor

Merged at 138938b9c4f93a84ce32a800c41b10696135f499.
Thanks to @bittoby!

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