Skip to content

fix: RemoteRegistry.apply_saved_dataset() calls the wrong RPC - #6740

Open
xKomil wants to merge 1 commit into
feast-dev:masterfrom
xKomil:fix/remote-registry-apply-saved-dataset-wrong-rpc
Open

fix: RemoteRegistry.apply_saved_dataset() calls the wrong RPC#6740
xKomil wants to merge 1 commit into
feast-dev:masterfrom
xKomil:fix/remote-registry-apply-saved-dataset-wrong-rpc

Conversation

@xKomil

@xKomil xKomil commented Aug 14, 2026

Copy link
Copy Markdown

What this PR does / why we need it:

RemoteRegistry.apply_saved_dataset() (sdk/python/feast/infra/registry/remote.py) builds an ApplySavedDatasetRequest, but sends it through the ApplyFeatureService RPC instead of the dedicated ApplySavedDataset RPC:

def apply_saved_dataset(
    self,
    saved_dataset: SavedDataset,
    project: str,
    commit: bool = True,
):
    request = RegistryServer_pb2.ApplySavedDatasetRequest(
        saved_dataset=saved_dataset.to_proto(), project=project, commit=commit
    )
    self.stub.ApplyFeatureService(request)  # <-- wrong RPC

The server (sdk/python/feast/registry_server.py) already implements a correct ApplySavedDataset handler that expects exactly this request shape and forwards it to proxied_registry.apply_saved_dataset(...). Because the client calls ApplyFeatureService instead, the server tries to handle an ApplySavedDatasetRequest payload through the ApplyFeatureService RPC/ApplyFeatureServiceRequest shape, so registering a SavedDataset against a remote registry (registry_type: remote) never reaches the intended handler and the saved dataset is not correctly persisted.

This was found while working with a Feast deployment using a remote registry (Feast Operator / OpenShift), trying to register a SavedDataset via store.registry.apply_saved_dataset(...) after using store.create_saved_dataset()'s .persist() step wasn't an option for the offline store in use.

The fix is a one-line change: call self.stub.ApplySavedDataset(request) instead of self.stub.ApplyFeatureService(request).

Which issue(s) this PR fixes:

None filed yet — I searched open/closed issues and PRs for ApplySavedDataset / apply_saved_dataset and didn't find an existing report.

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

Added test_apply_saved_dataset_calls_apply_saved_dataset_rpc to sdk/python/tests/unit/infra/registry/test_remote_registry.py, following the existing remote_registry fixture pattern in that file. Verified locally:

  • Test fails against the pre-fix code (ApplyFeatureService.assert_not_called() fails because it was called).
  • Test passes with the fix applied, and confirms the request payload (ApplySavedDatasetRequest) is unchanged, only the RPC method changes.
  • Full test_remote_registry.py suite (5 tests) passes.

Misc

Small, isolated fix — one production line changed, plus a regression test. No API changes.

…oteRegistry.apply_saved_dataset()

RemoteRegistry.apply_saved_dataset() builds an ApplySavedDatasetRequest
but sends it via the ApplyFeatureService RPC. The server has a dedicated
ApplySavedDataset RPC that expects exactly this request type; calling
ApplyFeatureService instead means the server tries to interpret an
ApplySavedDatasetRequest payload as an ApplyFeatureServiceRequest, so
registering a saved dataset through a remote registry never actually
persists it as intended.

Adds a regression test asserting the correct RPC is invoked.

Signed-off-by: xKomil <plociu1104@gmail.com>
@xKomil
xKomil requested a review from a team as a code owner August 14, 2026 11:49
@codecov-commenter

codecov-commenter commented Aug 14, 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 46.81%. Comparing base (b8dfcb0) to head (f90e3c1).
❗ 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    #6740   +/-   ##
=======================================
  Coverage   46.81%   46.81%           
=======================================
  Files         415      415           
  Lines       50399    50399           
  Branches     7214     7214           
=======================================
+ Hits        23592    23595    +3     
+ Misses      25157    25155    -2     
+ Partials     1650     1649    -1     
Flag Coverage Δ
go-feature-server 30.58% <ø> (ø)
python-unit 48.14% <100.00%> (+<0.01%) ⬆️
Files with missing lines Coverage Δ
sdk/python/feast/infra/registry/remote.py 44.96% <100.00%> (+0.62%) ⬆️

... and 1 file with indirect coverage changes


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 b8dfcb0...f90e3c1. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants