Skip to content

Replace existing TensorSpec bounds before packing new values - #5

Closed
sylvesterkaczmarek wants to merge 1 commit into
google-deepmind:masterfrom
sylvesterkaczmarek:fix/replace-tensor-spec-bounds-20260929
Closed

sylvesterkaczmarek wants to merge 1 commit into
google-deepmind:masterfrom
sylvesterkaczmarek:fix/replace-tensor-spec-bounds-20260929

Conversation

@sylvesterkaczmarek

Copy link
Copy Markdown

Fixes #4.

Change

Clear each existing bound after validation and before packing its replacement. The numeric repeated-field packers append values, so repeated set_bounds calls 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. None still 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.py for the regressions.

@sylvesterkaczmarek

Copy link
Copy Markdown
Author

@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?

@tomwardio

Copy link
Copy Markdown
Member

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.

@sylvesterkaczmarek

Copy link
Copy Markdown
Author

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.

@tomwardio

Copy link
Copy Markdown
Member

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.

@sylvesterkaczmarek

Copy link
Copy Markdown
Author

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.

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.

set_bounds appends to existing numeric bounds instead of replacing them

2 participants