Repository navigation
Replace existing TensorSpec bounds before packing new values - #5
sylvesterkaczmarek wants to merge 1 commit into
Conversation
|
@tomwardio The current head is mergeable, all current checks are green, and there are no unresolved review threads. The change replaces existing TensorSpec bounds before packing new values, with focused regression coverage. Could you review it or point me to the right reviewer? |
|
Hi, can you explain what the problem is you're trying to fix (beyond the issue that you submitted)? How is this affecting your downstream usage? This is quite an old package, so it's hard to know what the downstream effects might be. |
|
The downstream pattern I am validating is an environment adapter that reuses a TensorSpec protobuf and refreshes its bounds in place as the environment/spec changes, rather than constructing a brand-new proto every time. On current master, the second set_bounds() call appends into the repeated numeric field, so a scalar bound becomes two elements; the serialized spec then carries both stale and new values and bounds(spec) raises because the scalar Tensor contains multiple elements. Vector updates and scalar↔vector replacement have the same stale-state problem. I found this during compatibility/integration testing rather than a production outage, but it is deterministic and affects any caller that treats set_bounds as a setter on an existing spec. The patch deliberately validates first, then clears/replaces, so failed validation leaves the existing message unchanged. |
|
Thanks, I think I got that from the issue. Why are you re-using a TensorSpec proto? You'd want to create one per spec rather than re-use an existing spec proto. You can cache the tree of specs once it's been created, and you shouldn't be changing bounds within episode... Can you give any more specifics over the environment you're implementing? This seems like an incorrect usage pattern. |
|
Thanks for clarifying the intended usage. I do not have a deployed downstream environment to point to: this came from a compatibility/idempotence test that reused a TensorSpec protobuf while updating bounds. The repeated-field behavior is reproducible, but I cannot establish that reusing the same proto is a supported or necessary integration pattern. Given your guidance to create a fresh TensorSpec for each spec and cache the resulting spec tree, I will close this PR rather than add an unsupported API behavior. |
Fixes #4.
Change
Clear each existing bound after validation and before packing its replacement. The numeric repeated-field packers append values, so repeated
set_boundscalls previously retained stale values and could make scalar or vector bounds unreadable. The int8/uint8 byte packers already replaced their contents.The setter now replaces bounds consistently across numeric types.
Nonestill unsets the corresponding bound, invalid updates leave the message unchanged, and the low-level packers' behavior remains intact. No signature or protocol changes are included.Verification
All 32 new regression/control cases pass using real protobuf messages and the public bounds reader. They cover scalar/vector transitions, idempotence, eight numeric types, independently clearing bounds, variable-length specs, exact serialized messages and unchanged state after invalid updates. Original code fails 19 cases and passes thirteen controls.
The complete repository suite passes 417 tests and fourteen subtests locally on macOS/Python 3.12 and in exact-commit hosted validation on Ubuntu with Python 3.10/NumPy 1.26.4 and Python 3.12/NumPy 2.5.3.
Both jobs test
519950a576e9ce0e02092f79ed1d941ead9090a2, reproduce the original failures, pass the original existing suite, restore the submitted bytes and rerun the regressions. Direct wheel builds pass, and all 32 cases pass against installed wheels outside the checkout. Formatting, lint, dependency and patch checks pass. Logs, JUnit, coverage and packages are workflow artifacts; validation configuration remains on a separate fork branch.Scope
An existing packaging issue prevents building a wheel from the source distribution because it omits the required api-common-protos submodule. This reproduces with original production source in both hosted environments. Direct wheel builds from the initialized checkout pass. Existing NumPy warnings remain.
Only the bounds setter and new regression module change. No production dependencies, upstream workflows or existing test expectations are modified. Tests include local client/server examples, without live external environments.
Run
python -m pytest dm_env_rpc/v1/tensor_spec_bounds_replacement_test.pyfor the regressions.