feast-dev / feast

The Open Source Feature Store for Machine Learning
https://feast.dev
Apache License 2.0
5.57k stars 996 forks source link

Re-materialize feature view when underlying data source has changed #2795

Closed felixwang9817 closed 1 year ago

felixwang9817 commented 2 years ago

Is your feature request related to a problem? Please describe. Right now Redis prevents overwrites for entity keys when the new entry has an older timestamp than the existing entry in Redis (see here). This means that if someone changes the underlying data source and wishes to rematerialize data to the online store, rematerialization will not work.

Describe the solution you'd like Not sure. Maybe the ability to pass in a flag during materialization for whether to overwrite?

Describe alternatives you've considered N/A

Additional context Originally raised here.

achals commented 2 years ago

What about asking users to delete the feature view and then recreate the feature view?

felixwang9817 commented 2 years ago

What about asking users to delete the feature view and then recreate the feature view?

yeah this is the temporary solution (either this or directly tearing down the feature view) - I think this is a bit clunky, so this issue is more to track a longer-term solution

roelschr commented 2 years ago

Thanks for creating the issue @felixwang9817

What about asking users to delete the feature view and then recreate the feature view?

That's also an option, thanks! Since our FE pipelines are decoupled from Feast, this would require that users change the pipelines, then open two additional PRs to delete then recreate, and only then they can rerun the full/backfill job which consists of a full recreation of the DataSource table and a full materialization. It will be easier for the user to just rerun the full pipeline, trusting that a full materialization means recreating the keys on Redis, just like the full ETL recreates the DataSource table.

felixwang9817 commented 2 years ago

That's also an option, thanks! Since our FE pipelines are decoupled from Feast, this would require that users change the pipelines, then open two additional PRs to delete then recreate, and only then they can rerun the full/backfill job which consists of a full recreation of the DataSource table and a full materialization. It will be easier for the user to just rerun the full pipeline, trusting that a full materialization means recreating the keys on Redis, just like the full ETL recreates the DataSource table.

@roelschr I might be misunderstanding your setup, but I think @achals's suggestion would still work. the point is to leave your data sources intact (so there's no need to rerun or restart any ETL jobs or FE pipelines). if you destroy a FV and then recreate it, all previous materialization metadata will be gone, so when you run materialize again, the most recent data will be materialized. hope this makes sense!

klesouza commented 2 years ago

Do you see any potential issues if we solve this by changing this part https://github.com/feast-dev/feast/blob/19aedc2ed4893eab60af57e48af8b2602755e43e/sdk/python/feast/infra/online_stores/redis.py#L218 to be like:

if prev_ts.seconds and event_time_seconds < prev_ts.seconds:

The only issue I see is that existing clients that rely on the logic to avoid over loading Redis with already materialize items. But for the of Re-materializing it would work nicely, since the event_timestamp wouldn't have changed (meaning prev_ts.seconds == event_time_seconds), just the underlying calculated feature values.

And one issue I see with destroying the FV is that you would need to re-materialize all FV that share the same entity keys. As far as I got from the code, tearing down one single FV only takes into account which entity keys it has to lookup in Redis. https://github.com/feast-dev/feast/blob/19aedc2ed4893eab60af57e48af8b2602755e43e/sdk/python/feast/infra/online_stores/redis.py#L128

roelschr commented 2 years ago

@felixwang9817 if you want, I could walk you guys through our current setup, not sure if this would be of value for Feast/Tecton, but let me know.

But as @klesouza pointed out, it seems that tearing down Redis involves deleting all the keys for the whole entity. This means we cannot delete a single FV from the Online Store with the current implementation of Redis. So @achals' suggestion doesn't work.

https://github.com/feast-dev/feast/blob/master/sdk/python/feast/infra/online_stores/redis.py#L106

https://github.com/feast-dev/feast/blob/master/sdk/python/feast/infra/online_stores/redis.py#L79

To be fair, this behaves in a very unexpected way. Could you confirm this or have we misunderstood the code somehow?

achals commented 2 years ago

@roelschr It'd be extremely valuable - lets set some time up.

And I think you and @klesouza are right - the current feast data model doesn't allow for deletions. We've been kicking around the idea of changing the data model to namespace features by their feature view + entity id instead of just the entity id. We don't have a formal doc written up but happy to discuss more over slack if you'd like.

felixwang9817 commented 2 years ago

just noting that this issue is very closely related to #2254

adchia commented 2 years ago

@roelschr The above approach (changing <= to <) also seems to be fair to me. It was also partially there so that batch materialization doesn't override streaming feature values that are pushed in. And if the event_timestamps are the same, then presumably it isn't a big deal to overwrite

roelschr commented 2 years ago

@adchia it doesn't fully solve the issue. It's common to see that the underlying logic of a feature changed, but the base timestamp of the feature view didn't, so it would still not replace the old feature value in case of a full materialization. On our side, we are patching online_write_batch with a version without the timestamp check when we run full materialization jobs (keeping it the same for the incremental jobs).

To be fair the check itself makes a lot of sense. It would be perfectly fine if deleting a feature view actually deleted the values from redis.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.