Time the rig-side stack, and bind telemetry on a real run - #722
Time the rig-side stack, and bind telemetry on a real run#722v-positronic wants to merge 10 commits into
Conversation
6553fdb to
ee8114f
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6553fdb1a1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The eval CLI binds telemetry for a simulated sweep. An attended rollout builds its own World around the harness, so nothing there binds, and a real run records no spans at all. `telemetry.bind_from_env` binds a named process from the two env vars the eval CLI already sets for a launched env server. The harness calls it, so any binary that runs a harness records the same sidecar with no change of its own. It is inert while the vars are unset, and while a provider is already bound — under `eval run --timing` the CLI owns that lifecycle. Six spans split what one round trip costs the rig: - `policy.call` — the whole session call, so every control tick is timed, not only the ticks that infer. `policy.inferred` says which a tick was. Recorded rather than entered: entering it would make `policy.infer` a grandchild of the episode, and the pass report reads that span off the episode's own children. - `policy.stack` — the history window a `TemporalStack` assembles. It sits outside the scheduling layer, so every tick pays for it. - `policy.encode` — one codec's rig-side encode, named by the codec. `RestrictImageSize` over a 25-frame two-camera stack is 50 resizes on the thread that drives the arm. - `policy.prepare` — the JPEG encode, which already runs on the executor worker. - `wire.send` / `wire.recv` — the upload and the wait, split. `wire.bytes` carries the payload size, so a slow uplink shows as a send that outlasts its own bytes. `bind` keeps its signature; its body moves to `_bind_to`, which takes the spans file, because the two callers name it differently — one from a run's output dir, one from the telemetry dir the env var carries. Ticket: Positronic-Robotics/internal#1168 #refs
ee8114f to
cf6d90f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf6d90f76f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
`wire.send` covered the msgpack serialisation as well as the socket write, so a span named for the transfer carried CPU work that is not transfer. Serialising happens before the span; `wire.bytes` stays on it, so the pair still answers whether the uplink is too slow for the payload. Ticket: Positronic-Robotics/internal#1168 #refs
2d3e81a to
f97b4cb
Compare
The directory is what turns recording on, so requiring a run id beside it only pushed the operator into inventing one. A fixed string in a shell profile then merges every run's spans into one file under one name, which nothing afterwards can separate. An unset id is minted per process instead. Ticket: Positronic-Robotics/internal#1168 #refs
c647260 to
fd59687
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd596875e8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
`wire.recv` covered the msgpack decode as well as the socket read, the mirror of what the send span carried. Both spans now hold the socket and nothing else, so the pair reads as network plus the server's own time, which is what a reader subtracts a server-reported duration from. Ticket: Positronic-Robotics/internal#1168 #refs
6c690a5 to
f593bbd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f593bbdbbf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Minting a run id separated two runs in the resource block and not on disk: both still opened `harness.spans.jsonl` and the exporter appended, so one file held two runs and the reduce could not tell them apart. The run now names the file. The reduce globs the suffix and reads the process from each file's resource block, so it finds them either way. Ticket: Positronic-Robotics/internal#1168 #refs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3673caf251
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The run id reaches the process from an operator's environment, and naming the sidecar after it made that string a path component. One naming a parent directory would write outside the directory that turned recording on. The filename now carries a reduced token; the resource block holds the run id verbatim, so the reduce still reports what the operator set. Ticket: Positronic-Robotics/internal#1168 #refs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96d5a4b169
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
`_bind_to` sat after `bind`, and `Harness._guarded` after `run`, so a reader met each delegation before the body it delegates to and had to search forward. Both move up. `bind` and `_bind_to` exchange docstrings with the order, so the one a reader meets first carries the substance. Ticket: Positronic-Robotics/internal#1168 #refs
The run names the sidecar so two runs against one telemetry directory each get their own file. The reduce to filename characters is many-to-one, so `a/b` and `a_b` name one file between them and two runs append to it — the state naming the file after the run was meant to end. A reduced token now carries a digest of the id it reduced; an id a filename may carry is still the name on the file. Ticket: Positronic-Robotics/internal#1168 #refs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58b7abbb43
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
`_filename_token` returned the whole reduced run id. A run id longer than the filesystem's component limit therefore failed the export with `ENAMETOOLONG`, before the harness recorded a span. The token is capped now. A capped token differs from the id it reduced, so the digest already there keeps two ids that share a long prefix in their own sidecars. Naming a sidecar per run separated the files and nothing more. `read_spans` dropped the `run.id` resource attribute, so `_episode_windows` grouped every root episode under the `None` parent. An attended rollout opens no `eval.pass` span, so its episodes are roots. Two such runs appended to one telemetry directory reduced as one window, with the idle wall between them inside it. `SpanRec` carries the run id now, and a wall window is keyed by run and parent. Ticket: Positronic-Robotics/internal#1168 #refs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3703995d00
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Capping the filename token made two long ids share it, so the digest alone separated them and it carried 32 bits. `'r' * 65 + '18966'` and `'r' * 65 + '155513'` collide on those bits and named one sidecar between them. The digest carries 64 bits now, and the test that covers a capped prefix uses that pair. Four comments in this change described the change rather than the code. `bind_from_env` said that naming the file by run "costs the reduce nothing"; three test docstrings said what a run id "now" does, what two ids that reduce alike would name, and what a long one "would" have failed. Each states its invariant instead. `_resource_attrs` moves above the first test that calls it. That is every instance of both classes in this branch: the comments were found by reading each line the diff adds, and `_resource_attrs` is the only definition it adds that sits away from its first caller. Ticket: Positronic-Robotics/internal#1168 #refs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c6d92c0ed
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if token and token == run_id: | ||
| return token | ||
| # The reduction is many-to-one — `a/b` and `a_b` reduce alike, as do two ids sharing a capped prefix — | ||
| # so a reduced id carries a digest of the id it reduced and two runs never share one sidecar. | ||
| return f'{token}.{hashlib.sha256(run_id.encode()).hexdigest()[:_DIGEST_CHARS]}'.lstrip('.') |
There was a problem hiding this comment.
Rule hidden-dependency violated:
_filename_token assumes that a valid raw run ID cannot equal another ID's encoded token. For example, . and cdb4ee2aea69cc6a both map to cdb4ee2aea69cc6a, so the two runs append to one sidecar despite the per-run-file contract. Fresh evidence beyond the previously flagged digest-width collision is this deterministic collision between the raw and encoded namespaces; prefix every encoded token with a reserved marker or encode every ID uniformly.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
| """The run id reaches this from an operator's environment, and one longer than the filesystem's component | ||
| limit fails the export with `ENAMETOOLONG` before the harness records a span.""" |
There was a problem hiding this comment.
Rule diff-comments violated:
The docstring for test_a_run_id_too_long_for_a_filename_still_names_a_sidecar says the long ID fails with ENAMETOOLONG, while the test now verifies that encoding it succeeds. This documents the pre-fix failure and falsely describes current behavior; state the current invariant that oversized IDs are shortened into filesystem-safe sidecar names while remaining verbatim in the resource attributes.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
1 participant
What
telemetry.bind_from_env(process)binds one process's span sidecar from the two telemetry env vars the eval CLI already sets for a launched env server. The harness calls it, so any binary running a harness records the same sidecar with no change of its own: an attended rollout needs two environment variables and thetelemetryextra, and nothing else.Six spans split what one inference round trip costs the rig:
policy.callpolicy.stackTemporalStackassemblespolicy.encodecodecpolicy.preparewire.send/wire.recvwire.bytesis the payload sizepolicy.inferredsays whether a tick reached the server or was answered from the trajectory still playing.The run names its own sidecar and
SpanReccarries the run id, so two runs against one telemetry directory stay apart in the files and in the reduce.Why
A real rollout records no timing at all today. The eval CLI binds telemetry for a simulated sweep; an attended rollout builds its own World around the harness, so nothing there calls
bind. Existing client-side instrumentation does not cover the cost either:policy.inferopens after the observation is prepared, and a rig-side codec runs in a layer outside it. So everything between "the chunk finished playing" and "the next chunk's first setpoint" is invisible from both ends of the wire — the interval a partner is asking us to account for.policy.callis recorded rather than entered. Entering it would makepolicy.infera grandchild of the episode span, and the pass report reads that span off the episode's own children. Pairing a call with its round trip offline is a comparison of two timestamps.bind_from_envis inert while a provider is already bound. Undereval run --timingthe CLI owns the lifecycle, and a second bind would shut its provider down on leaving.ENAMETOOLONG, so the token naming the file is reduced, capped and digested — two ids still name two files, and the resource block holds the id verbatim.SpanReccarries the id itself because an attended rollout opens noeval.passspan: every episode is a root, and a window keyed by parent alone would hold two runs with the dead wall between them counted as theirs.policy.callandpolicy.stackopen on every control tick, so a long attended run writes a large sidecar — worth measuring before this is left on by default.Verification
uv run --extra telemetry pytest positronic --ignore=positronic/simulator— 1259 passed, 8 skipped.ruff,ruff formatandbasedpyrightclean, the baseline unchanged.Three tests were watched failing before their fix and passing after:
test_seal_exports_when_the_harness_owns_the_provider(zero episode spans exported), a 500-character run id failing withENAMETOOLONG, and two attended runs in one directory reducing as one 1040 s window rather than two 40 s ones.Driven against a live server's declared layer stack —
stop_on_fault | temporal_stack | chunked_schedule | restrict_image_size— over 220 control ticks of real 1280x720 frames, the spans separate the per-tick cost from the per-chunk cost and report the payload size.Refs
Positronic-Robotics/internal#1168.