Fix two proto conformance bugs in GetInfo and the implicit server - #69
Merged
Conversation
GetInfo is declared as a unary RPC in the proto, but the server implemented it as a generator and the client indexed the response as if it were a stream. gRPC could not serialize the generator, so every call failed with "Failed to serialize response!". The server now returns a DisciplineProperties message and the client reads the fields directly. The server also populates the name and version fields, which were previously left empty; disciplines carry these via new _name / _version attributes. Adds an end-to-end integration test that exercises GetInfo against a real gRPC server, so the two sides cannot drift again. The existing unit tests asserted against mocks shaped like the buggy implementation and have been corrected. Fixes #67
Array.end is inclusive per the standard, and the explicit server and both clients encode and decode it that way. The implicit server instead wrote the exclusive chunk bound from get_chunk_indices, so the client computed a destination slice one element longer than the payload. Any implicit variable spanning more than one chunk failed to decode with "could not broadcast input array from shape (n,) into shape (n+1,)". Single-chunk arrays survived by accident: the over-long slice was clipped by NumPy to the array length, which happened to match the payload. Every implicit variable in the examples and tests was a scalar, so nothing exercised the multi-chunk path. Adds an integration test with a 5-element implicit variable at num_double=2 (3 chunks) covering solve_residuals, compute_residuals, and residual_gradients. Existing implicit server unit tests asserted the exclusive encoding and have been corrected to the inclusive convention. Fixes #66
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes two spec-conformance bugs found while implementing
Philote-Rust against the same proto.
Closes #67
Closes #66
GetInfowas a generator on a unary RPC (#67)The proto declares
GetInfoas unary, but the serveryielded and the clientindexed
response[0]. gRPC cannot serialize a generator into aDisciplineProperties, so every call failed withStatusCode.INTERNAL | Failed to serialize response!— including Python clientto Python server.
Both sides had to change together: with the server alone fixed,
response[0]raises
TypeError.The server now also populates
DisciplineProperties.nameand.version, whichwere previously always empty.
Disciplinegained_name/_versionattributes (default
"") to back them.ImplicitServeremitted an exclusiveArray.end(#66)Array.endis inclusive per the standard. The explicit server and both clientsencode and decode it that way; the implicit server wrote the exclusive bound
straight from
get_chunk_indices, so the client sized its destination slice oneelement longer than the payload.
Any implicit variable spanning more than one chunk failed with
could not broadcast input array from shape (n,) into shape (n+1,).Single-chunk arrays survived by accident — the over-long slice was clipped by
NumPy to the array length, which happened to match the payload.
Why CI didn't catch either
Both bugs were invisible to the existing suite, so the tests are the substantive
part of this PR:
GetInfowas only ever tested through mocks shaped like the buggyimplementation (the client mock returned a list), and nothing exercised it
against a real server.
examples/andtests/is a scalar, so themulti-chunk path was never run.
Added:
test_get_discipline_info— end-to-endGetInfoagainst a real gRPC server,so the two sides can't drift again.
test_implicit_multi_chunk_transfer— a 5-element implicit variable atnum_double=2(3 chunks per variable, 13 for the 5×5 Jacobian), coveringsolve_residuals,compute_residuals, andresidual_gradients.Both were verified against the pre-fix code and reproduce the exact failures
from the issues.
Corrected the unit tests that encoded the buggy behavior as expected output:
three in
test_implicit_server.pyasserted the exclusive encoding, and tworequest fixtures used the exclusive convention on the wire (passing only via the
same NumPy clipping that masked the bug).
Testing
pytest tests/— 280 passed.