Delete entity key from Redis only when all attached feature views are gone #2240

pyalex · 2022-01-26T09:29:27Z

What this PR does / why we need it:

This PR adds the test for the use case described in #2150 as well as the fix for it.

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:

NONE

Signed-off-by: pyalex <moskalenko.alexey@gmail.com>

… gone Signed-off-by: pyalex <moskalenko.alexey@gmail.com>

Signed-off-by: pyalex <moskalenko.alexey@gmail.com>

codecov-commenter · 2022-01-26T09:54:49Z

Codecov Report

Merging #2240 (5145f5e) into master (396f729) will decrease coverage by 24.95%.
The diff coverage is 14.28%.

@@             Coverage Diff             @@
##           master    #2240       +/-   ##
===========================================
- Coverage   85.24%   60.28%   -24.96%     
===========================================
  Files         107      107               
  Lines        8605     8631       +26     
===========================================
- Hits         7335     5203     -2132     
- Misses       1270     3428     +2158

Flag	Coverage Δ
integrationtests	`?`
unittests	`60.28% <14.28%> (-0.15%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
sdk/python/feast/infra/online_stores/redis.py	`26.45% <10.00%> (-67.63%)`	⬇️
.../integration/online_store/test_universal_online.py	`15.00% <13.04%> (-82.24%)`	⬇️
...ts/integration/feature_repos/repo_configuration.py	`45.71% <50.00%> (-48.58%)`	⬇️
.../integration/online_store/test_online_retrieval.py	`17.39% <0.00%> (-82.61%)`	⬇️
...fline_store/test_universal_historical_retrieval.py	`19.18% <0.00%> (-80.82%)`	⬇️
sdk/python/tests/utils/online_read_write_test.py	`20.68% <0.00%> (-79.32%)`	⬇️
...gration/registration/test_feature_service_apply.py	`31.25% <0.00%> (-68.75%)`	⬇️
sdk/python/tests/data/data_creator.py	`34.78% <0.00%> (-65.22%)`	⬇️
sdk/python/tests/integration/e2e/test_usage_e2e.py	`35.00% <0.00%> (-65.00%)`	⬇️
... and 53 more

Continue to review full report at Codecov.

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

judahrand

What if, for example, you have two different FeatureViews both with the same join_key. Now you want to delete one of those FeatureViews but not the other.

In the Redis data model the features from both feature views are stored as keys under the same HASH. With these changes the extra features won't be deleted because the 'Entity' is still used.

I think there needs to be a method which is able to delete certain Features (from a FeatureView) when that FeatureView is changed or deleted. This method would have to use HDEL.

I guess this does fix #2150 but it introduces a new bug (which is admittedly less system breaking).

pyalex · 2022-01-26T14:28:30Z

@judahrand
Truth be told the current storage model used by Redis integration was not designed to delete features at all and generally speaking it's not possible (at least together with the current Provider API). Since we do not store feature names (only hashes) in Redis maps we need to know those names in advance. But what if you deleted some feature or renamed it. At the moment the feature view is deleted there's no guarantee that we can correctly determine all features (Redis map fields) that must be deleted. And Feast APIs are designed in such way that when the feature is renamed or deleted - it's not being clearly communicated to an online store implementation.

judahrand · 2022-01-26T15:28:25Z

Truth be told the current storage model used by Redis integration was not designed to delete features at all and generally speaking it's not possible (at least together with the current Provider API).

Why not? The signature of the method you've renamed to delete_entity_values used to take a FeatureView. From that you can determine the hash key prefix _redis_key_prefix(table.entities).

Then once you have that you can also determine the fields for that FeatureView by doing

fields = [_mmh3(f"{table.name}:{feature.name}") for feature in table.features ]

Once you've done that you just need to loop over the HASHs which match your prefix and call HDEL for each field.

I was thinking that delete_table_values would just need to be changed to look like this:

def delete_table_values(self, config: RepoConfig, table: FeatureView):
        client = self._get_client(config.online_store)
        deleted_count = 0
        pipeline = client.pipeline()
        prefix = _redis_key_prefix(table.entities)
        fields = [_mmh3(f"{table.name}:{feature.name}") for feature in table.features]

        for _k in client.scan_iter(
            b"".join([prefix, b"*", config.project.encode("utf8")])
        ):
            pipeline.hdel(_k, *fields)
            deleted_count += 1
        pipeline.execute()

        logger.debug(f"Deleted {deleted_count} keys for {table.name}")

This would at least delete the correct features when an entire FeatureView was deleted.

Since we do not store feature names (only hashes) in Redis maps we need to know those names in advance.

And Feast APIs are designed in such way that when the feature is renamed or deleted - it's not being clearly communicated to an online store implementation.

Yeah, this is true. And arguably is a bigger problem. Letting the OnlineStore just get less and less efficient/bloated etc should not be acceptable. 🤔

I can see, however, that my proposed solution does still have issues. If the FeatureView which has been deleted has been edited a lot in the past (eg. Feature renames, partial removals, etc.) then it will leave some garbage behind. But that feels better to me than what sort of amounts to a reference counter on the whole key - and the key is only released when there are 0 references (even though there could be thousands of unused fields under that key).

sdk/python/tests/integration/online_store/test_universal_online.py

sdk/python/feast/infra/online_stores/redis.py

felixwang9817 · 2022-01-27T03:03:29Z

@pyalex

Truth be told the current storage model used by Redis integration was not designed to delete features at all and generally speaking it's not possible (at least together with the current Provider API). Since we do not store feature names (only hashes) in Redis maps we need to know those names in advance. But what if you deleted some feature or renamed it. At the moment the feature view is deleted there's no guarantee that we can correctly determine all features (Redis map fields) that must be deleted. And Feast APIs are designed in such way that when the feature is renamed or deleted - it's not being clearly communicated to an online store implementation.

do you think feast plan can resolve this? the goal here is that each online store should report exactly what tables should exist, and the registry contains info on what tables currently exist - then the diff can be applied. so even if you rename a FV, feast plan should be able to tear down the old table and create a new one

pyalex · 2022-01-27T11:13:02Z

@felixwang9817 yes, I hope that with new design and APIs objects like online store will have more control and will receive all information, like about renaming a view or renaming a feature, whereas right now options are very limited.

felixwang9817

/lgtm

feast-ci-bot · 2022-01-28T07:25:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: felixwang9817, pyalex

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [felixwang9817]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* Add backticks to left_table_query_string (#2250) Signed-off-by: david <davidmiller252@gmail.com> Signed-off-by: sfc-gh-madkins <miles.adkins@snowflake.com> * Delete entity key from Redis only when all attached feature views are gone (#2240) * Delete entity from redis when the last attached feature view is deleted Signed-off-by: pyalex <moskalenko.alexey@gmail.com> * Delete entity key from Redis only when all attached feature views are gone Signed-off-by: pyalex <moskalenko.alexey@gmail.com> * make lint happy Signed-off-by: pyalex <moskalenko.alexey@gmail.com> * make lint happy Signed-off-by: pyalex <moskalenko.alexey@gmail.com> * one more try with mypy Signed-off-by: pyalex <moskalenko.alexey@gmail.com> Signed-off-by: sfc-gh-madkins <miles.adkins@snowflake.com> * historical_field_mappings2 merge for one sign off commit (#2252) Signed-off-by: Michelle Rascati <michelle.rascati@sailpoint.com> Signed-off-by: sfc-gh-madkins <miles.adkins@snowflake.com> * Correct inconsistent dependency (#2255) Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com> Signed-off-by: sfc-gh-madkins <miles.adkins@snowflake.com> * Add snowflake environment variables to allow testing on snowflake infra (#2258) * add snowflake environment vars to test framework Signed-off-by: sfc-gh-madkins <miles.adkins@snowflake.com> * add snowflake environment vars to test framework Signed-off-by: sfc-gh-madkins <miles.adkins@snowflake.com> * Return `UNIX_TIMESTAMP` as Python `datetime` (#2244) * Refactor `UNIX_TIMESTAMP` conversion Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com> * Return `UNIX_TIMESTAMP` types as `datetime` to user Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com> * Fix linting errors Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com> * Rename variable to something more sensible Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com> Signed-off-by: sfc-gh-madkins <miles.adkins@snowflake.com> * Feast plan clean up (#2256) * Run validation and inference on views and entities during plan Signed-off-by: Felix Wang <wangfelix98@gmail.com> * Do not log objects that are unchanged Signed-off-by: Felix Wang <wangfelix98@gmail.com> * Rename Fco to FeastObject Signed-off-by: Felix Wang <wangfelix98@gmail.com> * Remove useless method Signed-off-by: Felix Wang <wangfelix98@gmail.com> * Lint Signed-off-by: Felix Wang <wangfelix98@gmail.com> * Always initialize registry during feature store initialization Signed-off-by: Felix Wang <wangfelix98@gmail.com> * Fix usage test Signed-off-by: Felix Wang <wangfelix98@gmail.com> * Remove print statements Signed-off-by: Felix Wang <wangfelix98@gmail.com> Signed-off-by: sfc-gh-madkins <miles.adkins@snowflake.com> * Squash commits Signed-off-by: Danny Chiao <danny@tecton.ai> Signed-off-by: sfc-gh-madkins <miles.adkins@snowflake.com> * Add error type and refactor query execution to have retries Signed-off-by: Danny Chiao <danny@tecton.ai> Signed-off-by: sfc-gh-madkins <miles.adkins@snowflake.com> * Handle more snowflake errors Signed-off-by: Danny Chiao <danny@tecton.ai> Signed-off-by: sfc-gh-madkins <miles.adkins@snowflake.com> * Fix lint errors Signed-off-by: Danny Chiao <danny@tecton.ai> Signed-off-by: sfc-gh-madkins <miles.adkins@snowflake.com> * Fix lint errors Signed-off-by: Danny Chiao <danny@tecton.ai> Signed-off-by: sfc-gh-madkins <miles.adkins@snowflake.com> * Fix lint errors Signed-off-by: Danny Chiao <danny@tecton.ai> Signed-off-by: sfc-gh-madkins <miles.adkins@snowflake.com> * Fix wrong import Signed-off-by: Danny Chiao <danny@tecton.ai> Signed-off-by: sfc-gh-madkins <miles.adkins@snowflake.com> * modify registry.db s3 object initialization to work in S3 subdirectory with Java Feast Server (#2259) Signed-off-by: NalinGHub <nalinm01@gmail.com> Signed-off-by: sfc-gh-madkins <miles.adkins@snowflake.com> * clean up docs Signed-off-by: sfc-gh-madkins <miles.adkins@snowflake.com> * lint-python Signed-off-by: sfc-gh-madkins <miles.adkins@snowflake.com> * fixed historical test Signed-off-by: sfc-gh-madkins <miles.adkins@snowflake.com> * fixed historical test Signed-off-by: sfc-gh-madkins <miles.adkins@snowflake.com> Co-authored-by: David Miller <david@patagona.ca> Co-authored-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com> Co-authored-by: Michelle Rascati <44408275+michelle-rascati-sp@users.noreply.github.com> Co-authored-by: Judah Rand <17158624+judahrand@users.noreply.github.com> Co-authored-by: Felix Wang <wangfelix98@gmail.com> Co-authored-by: Danny Chiao <danny@tecton.ai> Co-authored-by: Nalin Mehra <37969183+NalinGHub@users.noreply.github.com>

pyalex requested a review from a team as a code owner January 26, 2022 09:29

pyalex requested review from adchia and removed request for a team January 26, 2022 09:29

feast-ci-bot added do-not-merge/release-note-label-needed needs-kind labels Jan 26, 2022

github-actions bot assigned achals Jan 26, 2022

feast-ci-bot added the size/M label Jan 26, 2022

pyalex added the kind/bug label Jan 26, 2022

feast-ci-bot added release-note-none and removed do-not-merge/release-note-label-needed labels Jan 26, 2022

pyalex added 3 commits January 26, 2022 16:31

Delete entity from redis when the last attached feature view is deleted

af0e9b6

Signed-off-by: pyalex <moskalenko.alexey@gmail.com>

Delete entity key from Redis only when all attached feature views are…

e444b1e

… gone Signed-off-by: pyalex <moskalenko.alexey@gmail.com>

make lint happy

9124605

Signed-off-by: pyalex <moskalenko.alexey@gmail.com>

pyalex force-pushed the redis-delete-by-entity branch from c59dd35 to 9124605 Compare January 26, 2022 09:36

feast-ci-bot added size/L and removed size/M labels Jan 26, 2022

pyalex assigned judahrand and unassigned achals Jan 26, 2022

pyalex added 2 commits January 26, 2022 16:48

make lint happy

cb3f162

Signed-off-by: pyalex <moskalenko.alexey@gmail.com>

one more try with mypy

5145f5e

Signed-off-by: pyalex <moskalenko.alexey@gmail.com>

pyalex added ok-to-test and removed needs-kind labels Jan 26, 2022

judahrand reviewed Jan 26, 2022

View reviewed changes

felixwang9817 reviewed Jan 27, 2022

View reviewed changes

sdk/python/tests/integration/online_store/test_universal_online.py Show resolved Hide resolved

sdk/python/feast/infra/online_stores/redis.py Show resolved Hide resolved

felixwang9817 self-assigned this Jan 27, 2022

felixwang9817 approved these changes Jan 28, 2022

View reviewed changes

feast-ci-bot added the lgtm label Jan 28, 2022

feast-ci-bot added the approved label Jan 28, 2022

feast-ci-bot merged commit 5fc0b52 into feast-dev:master Jan 28, 2022

This was referenced Jan 28, 2022

Awkward Deletion logic for Redis #2254

Closed

Merge feast-snowflake plugin into main repo with documentation #2193

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Delete entity key from Redis only when all attached feature views are gone #2240

Delete entity key from Redis only when all attached feature views are gone #2240

Uh oh!

pyalex commented Jan 26, 2022 •

edited

Loading

Uh oh!

codecov-commenter commented Jan 26, 2022 •

edited

Loading

Uh oh!

judahrand left a comment •

edited

Loading

Uh oh!

pyalex commented Jan 26, 2022

Uh oh!

judahrand commented Jan 26, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

felixwang9817 commented Jan 27, 2022

Uh oh!

pyalex commented Jan 27, 2022

Uh oh!

felixwang9817 left a comment

Uh oh!

feast-ci-bot commented Jan 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Delete entity key from Redis only when all attached feature views are gone #2240

Delete entity key from Redis only when all attached feature views are gone #2240

Uh oh!

Conversation

pyalex commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

judahrand left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pyalex commented Jan 26, 2022

Uh oh!

judahrand commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixwang9817 commented Jan 27, 2022

Uh oh!

pyalex commented Jan 27, 2022

Uh oh!

felixwang9817 left a comment

Choose a reason for hiding this comment

Uh oh!

feast-ci-bot commented Jan 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pyalex commented Jan 26, 2022 •

edited

Loading

codecov-commenter commented Jan 26, 2022 •

edited

Loading

judahrand left a comment •

edited

Loading

judahrand commented Jan 26, 2022 •

edited

Loading