Skip to content

Share Skippy stage wire byte accounting - #818

Merged
i386 merged 2 commits into
mainfrom
codex/skippy-stage-wire-byte-accounting
Jun 13, 2026
Merged

Share Skippy stage wire byte accounting#818
i386 merged 2 commits into
mainfrom
codex/skippy-stage-wire-byte-accounting

Conversation

@i386

@i386 i386 commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

Share one stage-message wire byte estimator between Skippy telemetry and conditioned-wire simulation.

This PR is only about transfer accounting. It does not add activation striping, change message framing, or alter request/cache epoch telemetry.

Before

Skippy estimated stage message size in two places:

  • binary_transport.rs for llama_stage.message_wire_bytes
  • binary_transport/wire.rs for bandwidth/delay conditioning

Those estimates could drift. The conditioned-wire estimate also counted only a partial header plus tokens, activation, and raw bytes. It missed position sidebands, sampling metadata, chat metadata, and the full fixed header.

How It Works

StageWireMessage::estimated_wire_bytes() now accounts for:

  • fixed stage wire header
  • sampling config bytes
  • logit bias bytes
  • chat sampling metadata length prefix and bytes
  • token sideband bytes
  • position sideband bytes
  • state import raw bytes
  • activation payload bytes

Both telemetry and wire conditioning call the same helper.

Representative telemetry:

llama_stage.message_wire_bytes=52429612
skippy.activation_bytes=52428800

Why This Is Good

Large prefill activation frames are one of the places where pipeline transport cost becomes visible. Before adding any striped-frame or multi-stream transfer path, Skippy needs reliable accounting:

  • telemetry should report the same byte count the simulator uses
  • bandwidth simulation should include sidebands and metadata
  • future striping thresholds can use one shared estimate
  • benchmark reports become easier to compare because byte accounting is centralized

This is the low-risk prerequisite for later large activation-frame transfer work.

Compatibility

No wire-format change.

The helper estimates existing bytes; it does not encode or decode anything new.

Validation

  • cargo fmt --all -- --check
  • cargo test -p skippy-protocol --lib
  • cargo test -p skippy-server --lib
  • cargo clippy -p skippy-protocol --all-targets -- -D warnings
  • cargo clippy -p skippy-server --all-targets -- -D warnings
  • cargo check -p mesh-llm
  • cargo clippy -p mesh-llm --all-targets -- -D warnings

Summary by CodeRabbit

  • Tests

    • Added a unit test validating wire message size estimation across sampling, chat metadata, tokens, and payload scenarios.
  • New Features

    • Exposed a message size estimation API to compute approximate serialized wire sizes for messages.
  • Refactor

    • Centralized message size estimation for bandwidth-delay and write scheduling, improving consistency and reliability of transfer timing.

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e529cd1c-1d45-4f79-82e8-d2f60d0b8b29

📥 Commits

Reviewing files that changed from the base of the PR and between 0ce4f0c and 424780b.

📒 Files selected for processing (3)
  • crates/skippy-protocol/src/binary/mod.rs
  • crates/skippy-protocol/src/binary/types.rs
  • crates/skippy-server/src/binary_transport.rs
🚧 Files skipped from review as they are similar to previous changes (3)
  • crates/skippy-server/src/binary_transport.rs
  • crates/skippy-protocol/src/binary/types.rs
  • crates/skippy-protocol/src/binary/mod.rs

📝 Walkthrough

Walkthrough

This PR centralizes wire-size estimation into StageWireMessage::estimated_wire_bytes(), adds a unit test validating the calculation for a representative message, and updates server callsites to use the new method while removing local estimation helpers.

Changes

Centralize wire-size estimation in StageWireMessage

Layer / File(s) Summary
Wire-size estimation implementation and test
crates/skippy-protocol/src/binary/types.rs, crates/skippy-protocol/src/binary/mod.rs
StageWireMessage::estimated_wire_bytes() and private payload_wire_bytes() estimate on-wire size (fixed header, optional sampling/logit-bias, chat metadata length prefix + bytes, and kind-dependent payload sizing). A unit test verifies the computed estimate for a representative prefill message with sampling, two logit_bias items, chat metadata, tokens/positions, and activation bytes.
Update callsites to use centralized estimation
crates/skippy-server/src/binary_transport.rs, crates/skippy-server/src/binary_transport/wire.rs
Replaces local helpers with message.estimated_wire_bytes() in telemetry (llama_stage.message_wire_bytes) and bandwidth-delay calculation (WireCondition::sleep_for); removes the deleted helper(s) and reformats an import list.

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • michaelneale
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly matches the PR's main objective: centralizing stage wire byte estimation by introducing a shared StageWireMessage::estimated_wire_bytes() method.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/skippy-stage-wire-byte-accounting

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

@i386 i386 changed the title [codex] Share Skippy stage wire byte accounting Share Skippy stage wire byte accounting Jun 10, 2026
@i386
i386 marked this pull request as ready for review June 10, 2026 10:43
@i386
i386 force-pushed the codex/skippy-stage-wire-byte-accounting branch from 58e1cf6 to 0ce4f0c Compare June 10, 2026 10:46
@i386

i386 commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator Author

@michaelneale could you review this one when you get a chance?

@i386
i386 merged commit a49302c into main Jun 13, 2026
3 checks passed
@i386
i386 deleted the codex/skippy-stage-wire-byte-accounting branch June 13, 2026 02:53
michaelneale added a commit that referenced this pull request Jun 13, 2026
* origin/main:
  Add transport-aware Skippy stage ordering (#814)
  Share Skippy stage wire byte accounting (#818)
  Report Skippy artifact cold-start costs (#815)
  fix: debug output capturing for TUI / panics (#827)
  fix(hero): visual corrections for iPhone SE size devices (#838)
  Add Skippy stage role metadata (#816)
  Add Skippy request cache epoch telemetry (#817)
  Consolidate agent skills and fix stale docs (Windows deploy, repo map, design docs) (#836)
  feature(version): normalize version markers for different build types (#831)
  fix(website): fix visual regressions (#835)
  fix(gh): change micn to michaelneale in auto_assign.yml
  Revert "fix(gh): replace micn with IvGolovach in auto_assign.yml (not a collaborator)"
  fix(gh): replace micn with IvGolovach in auto_assign.yml (not a collaborator)
michaelneale added a commit that referenced this pull request Jun 14, 2026
* origin/main: (29 commits)
  MoA: don't let small-model consensus pre-empt a still-running large model (#837)
  fix(console): render thinking traces as markdown
  Add bounded direct path repair (#846)
  Fix skippy smoke PR gate (#850)
  Stabilize skippy smoke chain startup (#849)
  fix(ci): switch back to auto-assign workflow
  fix(website): polish longform visual explainer (#843)
  fix: gemma thinking
  Carry GLM llama MTP patches (#840)
  Refresh llama.cpp canary patch queue (#839)
  Add transport-aware Skippy stage ordering (#814)
  Share Skippy stage wire byte accounting (#818)
  Report Skippy artifact cold-start costs (#815)
  fix: debug output capturing for TUI / panics (#827)
  fix(hero): visual corrections for iPhone SE size devices (#838)
  Add Skippy stage role metadata (#816)
  Add Skippy request cache epoch telemetry (#817)
  Consolidate agent skills and fix stale docs (Windows deploy, repo map, design docs) (#836)
  feature(version): normalize version markers for different build types (#831)
  fix(website): fix visual regressions (#835)
  ...

# Conflicts:
#	AGENTS.md
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