| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
@patelchaitany would you mind reviewing this when you have a chance? It is the valkey-glide half of #6856 on its own; the HGETALL threshold change is deliberately left out. (Posting this as a comment because the review-request API requires write access to this repo, which a fork author does not have.) |
Sorry, something went wrong.
|
@ntkathole tagging you as the right reviewer for this one: it extends the Redis online store read path from #6337, and you have merged most of the recent Redis online store changes (#6446, #6656). Scope: an opt-in client: glide on RedisOnlineStoreConfig that runs the batched HMGET as a single non-atomic valkey-glide batch, so the fetch runs off the GIL. redis-py stays the default; writes and async reads are unchanged; the HGETALL threshold change is deliberately not included. |
Sorry, something went wrong.
|
⚠️ Please install the Codecov Report❌ Patch coverage is 58.90411% with 30 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## master #6870 +/- ##
==========================================
+ Coverage 48.58% 48.62% +0.03%
==========================================
Files 427 427
Lines 53787 53845 +58
Branches 7834 7844 +10
==========================================
+ Hits 26132 26181 +49
- Misses 25788 25797 +9
Partials 1867 1867
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for the well-structured PR! Left a couple of suggestions below.
Sorry, something went wrong.
| # Only set when `client: glide` is configured, so the native extension stays | ||
| # untouched for users who never opt in. | ||
| _glide_client: Optional[Any] = None | ||
|
|
There was a problem hiding this comment.
Nit: _glide_client is cached here but never cleaned up. The existing teardown() closes the redis-py client — the GLIDE client should get the same treatment to release native (Rust-side) resources, file descriptors, and the connection pool.
Something like:
def teardown(self, config, tables, entities):
...
if self._glide_client:
self._glide_client.close()
self._glide_client = None
Sorry, something went wrong.
There was a problem hiding this comment.
Done in d12d035. teardown() now closes self._glide_client and sets it back to None, so the native client is released as you suggested.
One note: the redis-py client was not actually being closed in teardown() before this change either (the method only deletes keys), so I scoped this to the GLIDE client rather than also changing the redis-py lifecycle in this PR. Happy to close that one too if you'd prefer it here.
Sorry, something went wrong.
|
|
||
| if online_store_config.client == RedisClient.glide: | ||
| glide_sync = _load_glide_sync() | ||
| return _glide_hmget_batch( |
There was a problem hiding this comment.
_load_glide_sync() is called on every read when client == RedisClient.glide. While Python caches imported modules in sys.modules, this still incurs a function-call + dict-lookup overhead per read that is easy to avoid.
Consider caching the module reference on the instance alongside _glide_client, e.g.:
_glide_sync: Optional[Any] = None
def _get_glide_sync(self):
if self._glide_sync is None:
self._glide_sync = _load_glide_sync()
return self._glide_syncThen _read_hash_fields and _get_glide_client can both use self._get_glide_sync() instead of calling _load_glide_sync() each time.
Sorry, something went wrong.
There was a problem hiding this comment.
Done in d12d035. Added _get_glide_sync(), which imports the module once and caches it on the instance; both _read_hash_fields and _get_glide_client now go through it instead of calling _load_glide_sync() each read.
Sorry, something went wrong.
|
Also, please fix the linting error in CI |
Sorry, something went wrong.
|
Updated in d12d035. Both review suggestions are addressed and the lint failure is fixed:
Testing (local, against a real Valkey 9.1.2 on 127.0.0.1:6379)The Docker daemon here can't create containers (permission denied on the socket), so the testcontainers-based integration test was run against the local Valkey instead:
The checks on this fork PR show action_required, so they need a maintainer to approve before the workflows actually run. |
Sorry, something went wrong.
|
Correction: the head is now 93c88d2 (same tree as d12d035). That commit only changes the author/committer metadata — the first push reset the author name to chandlerok while the sign-off line said Chandler King, which tripped DCO. Re-authored so the sign-off matches, DCO is green again. No code change. |
Sorry, something went wrong.
Adds a `client: glide` option to RedisOnlineStoreConfig. When set, the synchronous read paths (online_read and the batched read behind get_online_features) issue their HMGET commands as one non-atomic GLIDE batch, so the fetch runs off the GIL instead of in Python per command. Default behavior is unchanged: redis-py remains the default client, all writes and async reads keep using redis-py, and GLIDE reads the same entity keys and hashed feature fields Feast already writes. valkey-glide-sync is an optional dependency behind the new `feast[glide]` extra. Configuring `client: glide` without it installed raises a Feast extras import error naming the extra to install. This does not include the HGETALL threshold change from the same issue. Related to feast-dev#6856. Signed-off-by: Chandler King <chandleroking@gmail.com>
Addresses review feedback on the GLIDE read path: - teardown() now closes the native GLIDE client and clears the cached reference, so its Rust-side resources and file descriptors are released. - The glide_sync module is imported once per store and cached on the instance instead of being looked up on every read. - Allowlist the test password literals so detect-secrets stops failing lint-python. Signed-off-by: Chandler King <chandleroking@gmail.com>
|
@ntkathole would you mind taking another look when you have a chance? Both of your suggestions from Sep 26 are addressed:
This is still the valkey-glide half of #6856 on its own; the HGETALL threshold change is deliberately not in here. Happy to split or drop anything if you would rather it landed differently. |
Sorry, something went wrong.
|
oh nvm I see you already reviewed! ty! |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
What this PR does / why we need it:
Adds an opt-in GLIDE-backed Redis online read path for Python get_online_features.
The existing redis-py path does command packing, socket parsing, and response handling in Python. Under thread contention, that work repeatedly reacquires the GIL and can make online reads much slower than single-threaded benchmarks suggest.
This PR adds a client: glide option to the Redis online store config. When enabled, Feast uses GLIDE for the batched Redis read while preserving the existing Redis storage layout and default redis-py behavior.
This PR intentionally does not include the HGETALL threshold optimization. That can be evaluated separately.
Scope
Implementation notes
The read client is selected in one place, RedisOnlineStore._read_hash_fields, which takes a list of (key, fields) HMGET commands and returns one reply per command. Both clients produce the same ordering, so the existing response conversion (_convert_redis_values_to_protobuf) and timestamp handling are untouched.
_glide_hmget_batch builds a single Batch(is_atomic=False) (or ClusterBatch(is_atomic=False) for cluster) and calls exec(batch, raise_on_error=True) once, so the whole fetch is one FFI call.
Three type annotations were corrected because the GLIDE path made the existing inaccuracy visible: _generate_hset_keys_for_features returns hashed bytes keys, not str, and the value sequences accepted by _convert_redis_values_to_protobuf / _get_features_for_entity legitimately contain None for missing fields.
Which issue(s) this PR fixes:
Related to #6856.
Checks
Testing Strategy
Commands run:
# Unit tests uv run python -m pytest sdk/python/tests/unit/infra/online_store/test_redis.py -q # 35 passed # Redis-related unit tests together uv run python -m pytest sdk/python/tests/unit/infra/online_store/test_redis.py \ sdk/python/tests/unit/infra/online_store/test_redis_versioning.py \ sdk/python/tests/unit/infra/online_store/test_helpers.py -q # 65 passed # Integration tests uv run python -m pytest --integration \ sdk/python/tests/integration/online_store/test_redis_glide_client.py -q # 2 passed # Lint and format uv run ruff check <changed files> # All checks passed! uv run ruff format --check <changed files> # 3 files already formatted # Types uv run bash -c "cd sdk/python && mypy feast/infra/online_stores/redis.py" # Success: no issues found in 1 source fileNotes on the test runs
Misc
The GLIDE batch is non-atomic by design, matching the existing pipeline(transaction=False) behavior. valkey-glide-sync 2.5.0 and 2.5.3 were both checked to confirm the Batch/ClusterBatch/config signatures used here exist across the 2.5 line.
I am happy to add glide to the ci lock files in this PR if you would rather have CI coverage of the GLIDE path immediately.