Skip to content

fix: Encode non-finite floats in feature server JSON responses - #6930

Open
Zhuoxi2000 wants to merge 1 commit into
feast-dev:masterfrom
Zhuoxi2000:fix-rest-nonfinite-500
Open

Zhuoxi2000 wants to merge 1 commit into
feast-dev:masterfrom
Zhuoxi2000:fix-rest-nonfinite-500

Conversation

@Zhuoxi2000

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

A stored NaN or +/-inf float value makes POST /get-online-features fail with HTTP 500 (ValueError: Out of range float values are not JSON compliant). _value_to_native returns raw Python floats, and JSONResponse serializes with allow_nan=False, so one non-finite value fails the whole multi-feature response. The SDK get_online_features().to_dict() path serves the same value fine. /search and /retrieve-online-documents share convert_response_to_dict and are affected too.

This PR encodes non-finite values as the protobuf JSON mapping strings "NaN", "Infinity" and "-Infinity". This applies to:

  • scalar double_val / float_val
  • double_list_val, float_list_val, double_set_val, float_set_val (element-wise)

Finite values are unchanged, and lists without non-finite values still go through a plain list(...). Nested list_val / map_val / struct_val values get the fix through the existing recursion. The convert_response_to_dict docstring documents the encoding.

If maintainers would rather emit null for non-finite values (the behaviour while ORJSONResponse was in use), this is a one-line change in _float_to_native.

Not covered here: RemoteOnlineStore._build_online_response_from_json infers value types from the JSON, so it would read the encoded strings as strings and rejects a list that mixes numbers and strings. This path already failed before (the server returned 500), and null would not round-trip through it either. I can follow up on the client side once the encoding is settled.

Which issue(s) this PR fixes:

No separate issue; the bug is described above.

Checks

  • I've made sure the tests are passing.
  • My commits are signed off (git commit -s)
  • My PR title follows conventional commits format

Testing Strategy

  • Unit tests
  • Integration tests
  • Manual tests
  • Testing is not required for this change

New tests:

  • tests/unit/test_feature_server.py::test_get_online_features_serves_non_finite_float[inf|-inf]: writes +/-inf to the online store, checks that the SDK serves it, then checks that /get-online-features returns 200 with "Infinity" / "-Infinity" and status PRESENT.
  • tests/unit/test_feature_server_utils.py: covers NaN/inf/-inf for double_val and float_val, the four float/double list and set types, and a json.dumps(..., allow_nan=False) round trip of convert_response_to_dict.

From sdk/python:

python -m pytest tests/unit/test_feature_server_utils.py tests/unit/test_feature_server.py -q
  • Without the fix (the tests only): 13 failed, 97 passed. Every new test fails, and the endpoint test fails with assert 500 == 200 / Out of range float values are not JSON compliant.
  • With the fix: 110 passed.

tests/unit/test_proto_json.py, tests/unit/test_feature_server_async.py and tests/unit/infra/feature_servers/test_mcp_server.py also pass (28 passed). ruff check and ruff format --check (0.16.9) are clean on the changed files.

Misc

A stored NaN or +/-inf float made /get-online-features (and the other
endpoints that use convert_response_to_dict) fail with HTTP 500, because
JSONResponse serializes with allow_nan=False. Encode non-finite values in
float/double scalars, lists and sets as the protobuf JSON strings "NaN",
"Infinity" and "-Infinity".

Signed-off-by: Edson <zhuoxi2000@gmail.com>
@Zhuoxi2000
Zhuoxi2000 requested a review from a team as a code owner October 2, 2026 11:23
@codecov-commenter

codecov-commenter commented Oct 2, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.66%. Comparing base (bc5aeef) to head (95890f7).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6930      +/-   ##
==========================================
+ Coverage   48.64%   48.66%   +0.01%     
==========================================
  Files         427      427              
  Lines       53864    53880      +16     
  Branches     7849     7854       +5     
==========================================
+ Hits        26204    26220      +16     
  Misses      25792    25792              
  Partials     1868     1868              
Flag Coverage Δ
go-feature-server 30.58% <ø> (ø)
python-unit 50.03% <100.00%> (+0.01%) ⬆️
Files with missing lines Coverage Δ
sdk/python/feast/feature_server_utils.py 90.80% <100.00%> (+2.07%) ⬆️

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc5aeef...95890f7. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Zhuoxi2000

Copy link
Copy Markdown
Contributor Author

The one failing job, unit-test-python (3.11, macos-14), errored during the setup of test_push_and_get. The feast apply subprocess in cli_repo_creator.local_repo timed out twice ("Command timed out after 120s (attempt 2/2): ['apply']"). That step isn't touched by this change.

The other four unit-test jobs passed, including macos-14 on 3.12. In that job, 2985 tests passed with 1 setup error. Could someone re-run it when convenient?

This branch has not been deployed

No deployments
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.

2 participants