Let a run end on an interrupt without naming another cause - #691
Let a run end on an interrupt without naming another cause#691DarksaCY wants to merge 11 commits into
Conversation
![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: 9a1f80ff03
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2edc42a68
ℹ️ 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 not isinstance(data, SMCompliant): | ||
| raise TypeError('Shared memory transport selected; data must implement SMCompliant') | ||
| self._emit_shared_memory(data, ts) | ||
| if _interrupted: |
There was a problem hiding this comment.
Rule hidden-dependency violated:
MultiprocessEmitter.emit() depends on the module-global _interrupted, so an interrupt involving one World's manager silently disables every other, independently managed World in the process, including Worlds created later with fresh manager connections. The fresh evidence after c2edc42 is that _interrupted remains module-global and is never reset or attached to World._manager; pass manager-scoped interruption state into each transport instead.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
| source: ControlSystemEmitter[T], | ||
| target: ControlSystemReceiver[T], | ||
| *, | ||
| emitter_wrapper: Callable[[SignalEmitter[T]], SignalEmitter[T]] = ..., |
There was a problem hiding this comment.
Rule overspecific violated:
connect() hard-codes the physical channel to the source type by annotating emitter_wrapper as SignalEmitter[T] to SignalEmitter[T] and states that only the receiver wrapper may change the payload type, although SignalMapWrapper.__call__ explicitly supports emitter-side transformations too. Introduce an intermediate transport type shared by the emitter-wrapper input and receiver-wrapper input so either side can transform while wrapper-less connections remain same-typed.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not this change's to make. The overloads added here restate the annotation connect's implementation already carries -- emitter_wrapper: Callable[[SignalEmitter[T]], SignalEmitter[T]], untouched by this PR -- and they exist only so wire() type-checks once the interrupt guard moved onto the public methods. Letting an emitter wrapper change the payload type widens what connect promises, and belongs in a change about connect rather than in one about ending a run on an interrupt.
An interrupt that lands inside a manager call leaves that connection holding half a message, and the next read comes back as something the queue never carried. The reader took it for a message and raised `AttributeError` over the real reason the run ended.
…ssages An interrupt lands in every process of a run at once, and it can land inside a call to the manager. That connection then holds half a message, and the next call over it returns what another one asked for: the recorder read a grip value off the channel that carries its commands and raised `AttributeError` over the real reason the run ended. A process that has taken an interrupt now neither sends nor reads. What it would have carried is going nowhere anyway: every other process is stopping.
`_optional_import` caught only a missing package. A vendor that is installed but whose own native library is not — `pyzed` without the ZED SDK — raises plain `ImportError`, and that ended the collection of the whole module. A station without the component is in the same place either way.
`pimm.map` maps a `SignalReceiver[T]` to a `SignalReceiver[U]` — its docstring says so, and data collection uses it to hand the headset the array inside a camera frame. `World.connect` typed the wrapper as type-preserving, so the call read as an error as soon as the camera port carried a type of its own.
… installed Four things the review of this branch found. The guard sat inside `_emit_queue` and `_read_queue`, which the public `emit` and `read` reach only after `_ensure_mode` and `transport_mode` have already called the manager -- and the shared memory path, which holds a manager lock for every frame, never reached it at all. Both public methods check first now. Tracking the interrupt from a handler `_bg_wrapper` installs left every main-process control system out: those reach the same transports, and their interrupt went unrecorded. A connection is torn only by an interrupt that lands inside a call over it, so the transport records its own -- `emit` and `read` re-raise what they catch, and no process depends on a handler being there. The handler and the `signal` import go. `World.connect` typed the receiver with a free `U`, which let an emitter of one type feed a receiver of another whenever no wrapper was passed. Overloads now ask for the emitter's own type unless a `receiver_wrapper` is given. That made `wire` say what it has always required -- `connect` asserts a `ControlSystemEmitter`, where the signature promised any `SignalEmitter` -- and six errors leave the basedpyright baseline. `_optional_import` caught every `ImportError`, which turns a broken project import into a skipped test. A failure inside `positronic` or `pimm` is raised now; a vendor's is logged at ERROR before the component is given up.
`_noting_interrupt` is a `contextmanager`, so it is a `ContextDecorator` too: `emit` and `read` wear it instead of indenting their whole bodies under it. Both it and `_interrupted` were defined between the emitter that first uses them and the receiver that uses them next; they sit above the emitter now.
`World.connect` gained overloads on this branch, and none of them takes a wrapper beside a caller and a handler. The test that asks for one and expects the assertion now says the type refuses it too.
The overloads on `World.connect` end five errors the baseline grandfathered. The ratchet only refuses growth, so CI would prune them on its own; the branch carries the pruned file instead.
6e0a043 to
df49ba8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df49ba8d83
ℹ️ 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".
Three errors the ratchet cannot grandfather, because the baseline holds a fixed count for each file and this branch adds a use to two of them. `_Tracker.update` reads `_operator_position` through the `umi_mode` property, which no checker can see through; it asks the field directly. `connect` types a receiver wrapper as carrying the emitter's own type, and `pimm.map` is what changes it. The two calls `main` already makes are grandfathered and a third is not, so this one carries the narrow suppression and names Positronic-Robotics#691, which gives `connect` the overload that says so. `where` in the tracker test was bound only inside its loop.
The guard asked whether the queue answered with `None`. A connection torn by an interrupt answers with what another call asked for, of whatever type that call wanted -- the incident this came from produced a float, and `.data` off it raises `AttributeError` and hides the interrupt. Anything that is not a `Message` says the connection is torn, and the answer is in the error.
`mp_pipes` answers with a `SignalReceiver`, which carries no queue; the test says which receiver it has.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee97552f20
ℹ️ 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".
`transport_mode` reads the manager when the mode is still undecided, and that read sits outside `emit` and `read`. An interrupt landing in it left the latch false, and the teardown after it used the torn connection. The test that names a torn connection also told the incident it came from rather than what has to hold.
1 participant
Four things a run needs to survive its own ending, and one an environment needs to survive a vendor it
does not have. Found while running a teleoperation station: every
Ctrl+Cended with a traceback thatnamed something other than the interrupt.
An interrupt lands in every process at once
It can land inside a call to the manager, and that connection then holds half a message. Every call over
it afterwards returns what another one asked for.
Two of them were seen on a station with an arm, two cameras and a recorder:
fail_queuedread a grip value where a request envelope belongs, and raisedAttributeError.AttributeError: 'float' object has no attribute 'type'— over the real reason the run ended.A process that has taken an interrupt now neither sends nor reads: whatever it would carry is going
nowhere, because every other process is stopping too. Where a torn connection still answers with
something that is not a message, the reader says so by name rather than dereferencing it.
A vendor that is installed but cannot load
_optional_importcaught a missing package. A package that is present while its own native library is not—
pyzedon a host without the ZED SDK — raises plainImportError, and that ended the collection ofthe whole test module. A station without the component is in the same place either way.
A wrapper that changes what a signal carries
pimm.mapmaps aSignalReceiver[T]to aSignalReceiver[U]; its docstring says so, and data collectionuses it to hand a headset the array inside a camera frame.
World.connecttyped the wrapper astype-preserving, so that call read as an error as soon as the camera port carried a type of its own.
Testing
uv run --locked pytestis green. The interrupt guard and the torn-connection reader each carry a testthat fails without them.