perf: Replace MessageToDict with optimized custom dict builder#6015
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
6a38f3c to
2b81774
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
2b81774 to
8fbda9f
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
8fbda9f to
f3ea953
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
06877fd to
063eeb2
Compare
d3c512d to
c06a75d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
c06a75d to
53a5264
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
2616fb8 to
300be07
Compare
300be07 to
a8c7653
Compare
| } | ||
|
|
||
|
|
||
| def response_to_dict_fast(response: GetOnlineFeaturesResponse) -> Dict[str, Any]: |
There was a problem hiding this comment.
why not response_to_dict? better yet, convert_response_to_dict is a better description
There was a problem hiding this comment.
Good call - renamed to convert_response_to_dict. The _fast suffix was leaking implementation details ✅
a8c7653 to
9859bad
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
bd8f618 to
eb7547c
Compare
|
@abhijeet-dhumal please resolve conflicts and update PR |
Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>
…nt FieldValues Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>
… and precision, add base64 encoding for bytes Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>
Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>
Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>
eb7547c to
0c0637b
Compare
| which = v.WhichOneof("val") | ||
| if which is None or which == "null_val": | ||
| return None | ||
| elif "_list_" in which: | ||
| return list(getattr(v, which).val) | ||
| else: | ||
| return getattr(v, which) |
There was a problem hiding this comment.
🔴 bytes_val values cause ORJSONResponse to raise TypeError
_value_to_native at feature_server_utils.py:68 returns raw bytes for bytes_val fields. Even if orjson were installed, orjson.dumps() cannot serialize Python bytes objects and raises TypeError: Type is not JSON serializable: bytes. The old code path returned the dict to FastAPI which processed it through jsonable_encoder (capable of handling bytes). The switch to ORJSONResponse bypasses that encoder, causing a regression for any response containing bytes-typed feature values.
Prompt for agents
In _value_to_native (feature_server_utils.py), when the value type is bytes_val, the raw bytes object is returned. orjson cannot serialize bytes. You need to either: (1) base64-encode bytes values before returning them (e.g. using base64.b64encode(val).decode('ascii')), or (2) not use ORJSONResponse and instead use a serializer that handles bytes. Note that bytes_list_val (handled by the _list_ branch) would also produce a list of bytes objects with the same issue.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
@abhijeet-dhumal bytes_val is still problem since you are using JSONResponse
There was a problem hiding this comment.
Thanks for catching this, Nikhil! Now bytes_val and bytes_list_val now base64-encode the output before returning, so JSONResponse / json.dumps can serialize them without TypeError.
There was a problem hiding this comment.
Please review!
|
@ntkathole Apologies for the late reply here .. I have fixed and addressed the open reviews! |
| from fastapi.concurrency import run_in_threadpool | ||
| from fastapi.logger import logger | ||
| from fastapi.responses import JSONResponse | ||
| from fastapi.responses import JSONResponse, ORJSONResponse |
There was a problem hiding this comment.
ORJSONResponse has been deprecated in FastAPI.
https://fastapi.tiangolo.com/reference/responses/?h=ORJSON#fastapi.responses.ORJSONResponse
Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>
Previous implementation used "_list_" substring check which silently returned protobuf objects (not JSON-serializable) for: - *_set_val types (bytes_set_val, string_set_val, int64_set_val, etc.) - list_val / set_val (RepeatedValue with nested Values) - map_val / struct_val (Map<string, Value>) - map_list_val / struct_list_val (MapList) Add tests for all previously uncovered types and a float32 precision consistency test against MessageToDict. Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>
…nt set-type behavior Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>
8d76583 to
b2f2909
Compare
…p test Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>
Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>
bytes_val returned raw bytes which cause json.dumps TypeError when JSONResponse serializes the response. Base64-encode these values to match standard protobuf JSON encoding for bytes fields. Fixes Nikhil's review comment: bytes_val is still problem since you are using JSONResponse. Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>
Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>
Summary
Fixes #6013
The feature server uses
google.protobuf.json_format.MessageToDictfor convertingGetOnlineFeaturesResponseto JSON, but this generic converter is slow due toreflection-based field introspection.
Changes
feature_server_utils.pywithconvert_response_to_dict()functionGetOnlineFeaturesResponsestructurestatusbool field (omitted when false, matching proto3 default behavior)Performance Impact
~4x faster serialization, saving 5-15ms per request for typical feature responses
with 50-200 features.
Test Plan
test_feature_server_utils.pyMessageToDict+proto_json.patch()formatstatusfieldmake lint-python