Conformance test for the client/server boundary, and raise UnsupportedReturnType instead of silently stringifying#111
Merged
Conversation
_pack_result previously fell through to a silent str() for any return type its switch did not handle. msgpack.packb then either encoded the stringified value (silently degrading real data) or failed downstream with an opaque "failed to serialize response" — how random.default_rng (a numpy Generator) broke for participants before 0.4.2. The fallback now raises a structured UnsupportedReturnType when the result is not msgpack-native, surfaced as error_type "UnsupportedReturnType" so the client gets an attributable error. _make_serializable still flattens nested numpy structures, so legitimate dict/list/scalar results are unaffected. Adds UnsupportedReturnType to core errors, mirrored into the generated client errors via sync_client.py.
… guard Every op the client registry advertises as proxyable is optimistically forwarded to the server; nothing verified the result could come back. Adds tests/test_registry_conformance.py: - test_op_round_trips drives a representative call for each exercised op (155 category defaults plus ~65 overrides incl random.default_rng) through the real RequestHandler and fails on the serialization-error class. - test_every_proxyable_op_is_classified is the coverage guard: each of the 473 proxyable ops must be exercised or in PENDING_EXAMPLES, so a newly generated op cannot ship unexercised. Pairs with the UnsupportedReturnType hardening: together they would have caught the default_rng divergence before release.
The category-default args (a float matrix) didn't fit 21 ops, which the smoke skipped as arg-domain errors. Give them correct inputs via EXAMPLE_OVERRIDES so they actually round-trip: integer arrays for the bitwise / gcd / lcm / shift / ldexp ufuncs, a q value for percentile / quantile (keyword for the nan- variants), 1-D input for cumulative_sum / cumulative_prod, and binary args for isclose (registry-categorized counted_unary but takes a, b). Conformance is now 227 passed, 0 skipped.
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
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.
The client forwards every op in its registry (
iter_proxyable()— 473 of them) to the server without ever checking that the result can actually come back._pack_resulton the server is a type switch with astr()fallback at the end, so a return type it doesn't recognise just slips through and then dies inmsgpack.packbwith the opaque "failed to serialize response". That's exactly whatrandom.default_rngdid to participants until 0.4.2 (#109) — it returns aGenerator, which hit the fallback. And that fix only patcheddefault_rng; nothing stops the next unhandled return type doing the same thing.So this adds a guard for the whole class of bug, and makes the failure loud instead of silent.
Server side.
_pack_resultnow raisesUnsupportedReturnTypefor results that aren't msgpack-native, instead of stringifying them. It comes back aserror_type: "UnsupportedReturnType"with the offending type in the message._make_serializablestill flattens the nested-numpy cases, so anything that packed fine before is unchanged.The test (
tests/test_registry_conformance.py):test_op_round_tripsruns a real call for each exercised op through an in-processRequestHandlerand fails if the response isUnsupportedReturnType/FlopscopeServerError. Arg errors (ValueError/TypeError) get skipped — that just means my example args were wrong for the op, not that the boundary is broken.test_every_proxyable_op_is_classifiedis the guard: every proxyable op has to be either exercised or parked inPENDING_EXAMPLES. Add an op to the registry and it lands in neither, so this goes red until someone gives it an example.There's also a
test_default_rng_round_tripsas an explicit regression for #109.A few notes:
PENDING_EXAMPLES. Shrinking that set is follow-up work — the point here is that nothing falls off the radar silently.RemoteGeneratormethods (rng.standard_normal()and friends) aren't module-level proxies, so they're outsideiter_proxyable()and this test's scope.str()-ed will now error instead. I ran the full suite and nothing actually hit that path, but flagging it in case.Ran locally: the conformance file is 227 passed / 0 skipped, the full
pytest --cov=flopscoperun is green, andsync_client.py --check, the version-sync check, ruff, and pyright are all clean.