fix: Update Feast object metadata in the registry

[msistla96]

What this PR does / why we need it:

This PR is to fix an issue observed when updating a feature view: Thecreated_timestamp and materialization_intervals values already present in the SQL registry for the feature view are overwritten by the respective values created when feature view is updated, which is None. So when one queries the feature view from the registry, it return None for both these fields.

The fix requires updating the apply_object function to have the previously stored values of the metadata fields like created_timestamp etc assigned to the feast objects respective fields, before it gets updated by the SQL registry.
Hence this change has been made for the relevant Feast objects like Entity, SavedDataset, FeatureService, FeatureView,OnDemandFeatureView,StreamFeatureView.
This change has also been applied for all other registries other than SQL.

Which issue(s) this PR fixes:

Fixes

[tokoko]

Hey, thanks for raising this. Just curious, maybe you have taken a look, is this the case only for sql registry? why isn't the same happening for file-based registries?

[msistla96]

@tokoko I noticed this for the SQL registry as our team uses it, but I'm not sure if this is happening for other registry types as well. If other registries do require this change, it would be separate changes to each of their apply methods from what I've seen so far.

[tokoko]

Should you be doing this? If you delete and apply again, of course created_timestamp will be regenerated. Isn't the whole point to assert that it's unchanged even if new object is applied when during apply an object already exists?

[msistla96]

My bad, this was supposed to be present for test_apply_feature_service_success only. I'll remove this.

[tokoko]

If you drop delete above, then I think this should be an equality check to assert that it's not changed. is not None doesn't seem to be checking much, unless I'm missing something

[msistla96]

You're right. I initially wrote the code with the mindset that it shouldn't return a None when you do an apply, but it would be better to check this against the previous feature view's created timestamp as well.

[tokoko]

@msistla96 sure, I quickly took a look and this doesn't seem to be handled anywhere. That's why I find it strange that the newly added tests in test_universal_registry aren't failing for file-based registries.

[msistla96]

@msistla96 sure, I quickly took a look and this doesn't seem to be handled anywhere. That's why I find it strange that the newly added tests in test_universal_registry aren't failing for file-based registries.

So the tests worked because for the other registries, the created_timestamp value does get set technically but it shouldn't for each time a Feast object gets updated. I've updated the code for all the other registries failing the test and also the tests as well.
I've also had to include an additional check to see if materialization_intervals should be updated or not, based on whether the feature view was materialized before or not.

[tokoko]

@msistla96 thanks. this is looking good. Can you add materialization_intervals test in test_universal_registry as well? You don't have to actually run anything, just call apply_materialization on test_registry.
@jeremyary Can you take a look why tests aren't running?

[msistla96]

@tokoko, while adding the tests you mentioned, I noticed two bugs with Remote Registry:

1. During apply_materialization, when Remote Registry converts the start and end dates to Timestamp before making the request to Registry Server here, Timestamp doesn't preserve the timezone information. So when RegistryServer converts them back to dates before calling the proxy apply_materialization here, the resultant date is timezone naive, i.e: it assumes the date relative to the local timezone rather than UTC timezone because of the mentioned reason. I made a change to RegistryServer in this PR to fix this as the integration tests were failing for Remote Registry.
   (FYI, this issue wrt Timestamp was raised long ago but Timestamp is not meant to store timezone info in the first place from what I understood)

To reproduce Timestamp's behavior:

from google.protobuf.timestamp_pb2 import Timestamp
from datetime import datetime
from pytz import utc

current_date = datetime.utcnow().replace(tzinfo=utc)
print("Current date: ",current_date, ", timezone: ",current_date.tzinfo)
sample_timestamp = Timestamp()
sample_timestamp.FromDatetime(current_date)

converted_date = datetime.fromtimestamp(
    sample_timestamp.seconds + sample_timestamp.nanos / 1e9)

print("Converted date: ", converted_date, ", timezone: ",converted_date.tzinfo)

2. apply_materialization fails for Stream Feature Views. ApplyMaterializationRequest for RegistryServer doesn't have Stream Feature View as a property even though ApplyFeatureViewRequest does. Fixing this is out of scope for this PR, so for now I've omitted test cases for materializing stream feature views to prevent the integration tests from failing. I added a TODO in the file, but let me know if you want me to raise this as a bug as well.

[tokoko]

You should put this before test_registry.apply_feature_view(updated_fv1, project) to make sure that an apply operation doesn't mess with materialization intervals.

[msistla96]

Done

[tokoko]

2. apply_materialization fails for Stream Feature Views. ApplyMaterializationRequest for RegistryServer doesn't have Stream Feature View as a property even though ApplyFeatureViewRequest does. Fixing this is out of scope for this PR, so for now I've omitted test cases for materializing stream feature views to prevent the integration tests from failing. I added a TODO in the file, but let me know if you want me to raise this as a bug as well.

I had the same bug in ApplyFeatureViewRequest and fixed it later on, missed ApplyMaterializationRequest apparently. Yup, a separate bug ticket for this would be good. thanks for catching this.

[msistla96]

@tokoko Could I have another review please? I realised that I missed a test case when apply_materialization adds the materialization_intervals which shouldn't call update_materialization_intervals, which I added in the last commit.

[tokoko]

@msistla96 that's an interesting corner case. Why do you think that should be the desired behavior? That actually would sort of address an issue raised here #4222, but I'm still unsure. We would effectively be allowing materialization intervals to be updates in two ways (apply_materialization and apply_feature_view).

[msistla96]

@msistla96 that's an interesting corner case. Why do you think that should be the desired behavior? That actually would sort of address an issue raised here #4222, but I'm still unsure. We would effectively be allowing materialization intervals to be updates in two ways (apply_materialization and apply_feature_view).

apply_materialization takes care of updating materialization_intervals, but apply_feature_view will ensure the materialization_interval stored doesn't get erased( the only time we would erase it is if the feature view was created for the first time and that becomes a separate feature view anyway). I think renaming update_materialization_intervals to copy_materialization_intervals would avoid this confusion. Does that make sense?

[EXPEbdodla]

2. apply_materialization fails for Stream Feature Views. ApplyMaterializationRequest for RegistryServer doesn't have Stream Feature View as a property even though ApplyFeatureViewRequest does. Fixing this is out of scope for this PR, so for now I've omitted test cases for materializing stream feature views to prevent the integration tests from failing. I added a TODO in the file, but let me know if you want me to raise this as a bug as well.

I had the same bug in ApplyFeatureViewRequest and fixed it later on, missed ApplyMaterializationRequest apparently. Yup, a separate bug ticket for this would be good. thanks for catching this.

@tokoko What is the purpose/ use case of calling apply_materialization for StreamFeatureViews? I'm assuming it mostly make sense for Feature Views only.

[tokoko]

@EXPEbdodla yeah, you're right, my bad. I've had a similar use (not with feast) case when I needed to update online store both directly and from offline with different datasets, but until we can practically support something like that, there's no point.

[msistla96]

@tokoko I see Registry support Stream Feature Views for apply_materialization currently, so I'm a bit confused about your comment on not having it. Are there any changes required for this PR?

[tokoko]

@msistla96 Technically it is, but as feast workflow for stream feature views doesn't really contain materialization from offline to online, it's never used. It would be impossible to restrict it in signature with types only as StreamFeatureView extends FeatureView instead of BaseFeatureView (a historical quirk that probably needs to be revisited at some point).

I was still unsure about the update we discussed above, but you're probably right, overwriting with new materialization intervals if provided seems as good as any other option.

[tokoko]

LGTM, thanks for the fix

[msistla96]

@tokoko Let me know when this would be merged.

[HaoXuAI]

LGTM