Add callback function as parameter in the feature usage functions

[chinmaytredence]

Background: The current functions in feathr client which are used for getting the online or offline features, do not give the capability to extend the function to register the cosumption of the features by any notebook.

This PR includes changes for following functions:

• get_online_features
• multi_get_online_features
• get_offline_features
• monitor_features
• materialize_features

In these above functions, two optional parameters: a callback function, params for the callback function, are added.

[xiaoyongzhu]

Thanks @chinmaytredence for the contribution! The code looks good to me. @hangfei can also chime in.

A few comments:

1. Can we add simple tests for those? To make sure others' changes won't break those code.
2. Maybe a small doc describing this, maybe under https://github.com/linkedin/feathr/tree/main/docs/how-to-guides

[hangfei]

Thanks for contributing this work!

1. Please add a user guide so we know how the design will look in the POV of users
2. asyncio is a new lib so please follow the contribution guide to add its license and add to requirements/setup.py.

[chinmaytredence]

Thanks @xiaoyongzhu and @hangfei for your valuable comments. I am adding the tests and updating this PR

[chinmaytredence]

@xiaoyongzhu @hangfei we have added the how-to-doc as well as the test case.
The asyncio and mock are part of the python 3. So, we did not add any licensing for those.

Please review once.

[windoze]

Could you please explain more about the purpose of these callback functions? What's their use cases?

[chinmaytredence]

@windoze the primary aim to add this callback as parameter was to register the details about which notebook is using these features. This helps in generating end to end lineage of a feature.

the callback function could be used to call one rest end point. The param is used as payload to the rest end point which would hold the metadata of the feature usage. In this way we will be able to extend the feature consumption layer.

This will solve the issue #344

[windoze]

@windoze the primary aim to add this callback as parameter was to register the details about which notebook is using these features. This helps in generating end to end lineage of a feature.

the callback function could be used to call one rest end point. The param is used as payload to the rest end point which would hold the metadata of the feature usage. In this way we will be able to extend the feature consumption layer.

This will solve the issue #344

Took a look at the issue, if my understanding is correct, the final usage should look likes this:

def update_lineage(url, ...):
    atlas_client.do_something_with(url, ...)

client.get_online_features(..., callback=update_lineage, params=[some_purview_url, something_else])

My point is, what's the different between above snippet to the following one?

def update_lineage(url, ...):
    atlas_client.do_something_with(url, ...)

client.get_online_features(...)
update_lineage(url, something_else)

Why do we want to "callback" a "call-after" function?

[chinmaytredence]

@windoze what if someone needs to store the feature usage in a separate DB to monitor it, or some other events we want to trigger based on the feature usage.
This callback would be a generic place for extending the capability for the feathr client functions, not only the lineage Further, the function is implicitly providing the capability to call the callback whereas we have to write a separate line for updating the lineage with usage of update_lineage(url, something_else)

Please suggest.

[windoze]

@windoze what if someone needs to store the feature usage in a separate DB to monitor it, or some other events we want to trigger based on the feature usage. This callback would be a generic place for extending the capability for the feathr client functions, not only the lineage Further, the function is implicitly providing the capability to call the callback whereas we have to write a separate line for updating the lineage with usage of update_lineage(url, something_else)

Please suggest.

Thanks, @chinmaytredence, I understand the motivation now.

From my POV this callback is too generic and doesn't look very clear how it should be used, my suggestion is to add a more specific function like following:

1. Add the "consuming callback" in FeathrClient instead of individual consuming points.
2. FeathrClient calls the "consuming callbacks" after every "consuming" methods implicitly after the consuming points such as get_online_features.

So the user needs to register the consuming callback only once in the FeathrClient instead of adding them ad-hoc, and the consumption can be recorded automatically.

[chinmaytredence]

@windoze thanks for suggesting the class level callback approach. Wouldn't it be too specific and what if someone wants to call different callbacks in different apis of the Fethrclient.

Suppose i would be sending an email to the feature creator as well as call another api to store the feature in a DB

[windoze]

We can add something like a reason parameter to tell the callback why it got called, so it can do specific things for specific reason.

For ad-hoc usage, I still think they can just call another function right after the get_online_features call, it doesn't make any difference.

[windoze]

@windoze the params would be sent in function level. Or else we cannot send the variable payload.

Then I'll suggest to thoroughly review the requirement and the use case before this PR.

I cannot imagine a case that requires totally different parameter sets in different places but for the same purpose, or you can add multiple callbacks if you do have multiple different purposes.

If the variable is environmental, you can always wrap them into a callable Class and get needed info from the environment.

It's easy to add a function, but it's hard to change or remove it later even if we find that the function is not what we expected.

[chinmaytredence]

@windoze Please see the below code snippet

res = client.get_online_features('nycTaxiDemoFeature', '265', ['f_location_avg_fare', 'f_location_max_fare'])

and

feature_query = FeatureQuery( feature_list=["f_location_avg_fare", "f_trip_distance_rounded", "f_is_long_trip_distance"], key=location_id) settings = ObservationSettings( observation_path="wasbs://[email protected]/sample_data/green_tripdata_2020-04_with_index.csv", event_timestamp_column="lpep_dropoff_datetime", timestamp_format="yyyy-MM-dd HH:mm:ss")

client.get_offline_features(observation_settings=settings, feature_query=feature_query, output_path=output_path )

There are various features getting pulled in two different methods. If we have to register the features w.r.t online or offline feature store, then we need to send variable parameters.

Please suggest.

[windoze]

@windoze Please see the below code snippet

res = client.get_online_features('nycTaxiDemoFeature', '265', ['f_location_avg_fare', 'f_location_max_fare'])

and

feature_query = FeatureQuery( feature_list=["f_location_avg_fare", "f_trip_distance_rounded", "f_is_long_trip_distance"], key=location_id) settings = ObservationSettings( observation_path="wasbs://[email protected]/sample_data/green_tripdata_2020-04_with_index.csv", event_timestamp_column="lpep_dropoff_datetime", timestamp_format="yyyy-MM-dd HH:mm:ss")

client.get_offline_features(observation_settings=settings, feature_query=feature_query, output_path=output_path )

There are various features getting pulled in two different methods. If we have to register the features w.r.t online or offline feature store, then we need to send variable parameters.

Please suggest.

We could pass the consumption input/output information to the callback as the callback context. Given the main use cases, I think it's reasonable.

Also it can be less error-prone, e.g., if you add the callback in an ad-hoc fashion:

def record_consumption(feature_list):
    # Record list of features consumption to somewhere
    ...
res = client.get_online_features('nycTaxiDemoFeature', '265', ['f_location_avg_fare', 'f_location_max_fare'],
    callback = record_consumption, params=['f_location_avg_fare', 'f_location_max_fare'])

You need to provide feature list twice and they could be inconsistent then you record everything wrong.

I strongly recommend an accurate design for the clear scenarios, over of a overgeneric solution that can do anything. The latter will always be abused in an unexpected way and you can do nothing about it.

[chinmaytredence]

@windoze Please see the below code snippet
res = client.get_online_features('nycTaxiDemoFeature', '265', ['f_location_avg_fare', 'f_location_max_fare'])
and
feature_query = FeatureQuery( feature_list=["f_location_avg_fare", "f_trip_distance_rounded", "f_is_long_trip_distance"], key=location_id) settings = ObservationSettings( observation_path="wasbs://[email protected]/sample_data/green_tripdata_2020-04_with_index.csv", event_timestamp_column="lpep_dropoff_datetime", timestamp_format="yyyy-MM-dd HH:mm:ss")
client.get_offline_features(observation_settings=settings, feature_query=feature_query, output_path=output_path )
There are various features getting pulled in two different methods. If we have to register the features w.r.t online or offline feature store, then we need to send variable parameters.
Please suggest.

We could pass the consumption input/output information to the callback as the callback context. Given the main use cases, I think it's reasonable.

Also it can be less error-prone, e.g., if you add the callback in an ad-hoc fashion:

def record_consumption(feature_list):
    # Record list of features consumption to somewhere
    ...
res = client.get_online_features('nycTaxiDemoFeature', '265', ['f_location_avg_fare', 'f_location_max_fare'],
    callback = record_consumption, params=['f_location_avg_fare', 'f_location_max_fare'])

You need to provide feature list twice and they could be inconsistent then you record everything wrong.

I strongly recommend an accurate design for the clear scenarios, over of a overgeneric solution that can do anything. The latter will always be abused in an unexpected way and you can do nothing about it.

I understand that the generic solution could do anything. But, the intention of this callback was to make it generic enough so that the capability of the handlers could be enhanced by calling other rest end points without any specific usecase.

i am not able to visualize what could be breaking with this callback function in the Feathr implementations. Would you like to help me with some scenarios on this please.

[windoze]

@windoze Please see the below code snippet
res = client.get_online_features('nycTaxiDemoFeature', '265', ['f_location_avg_fare', 'f_location_max_fare'])
and
feature_query = FeatureQuery( feature_list=["f_location_avg_fare", "f_trip_distance_rounded", "f_is_long_trip_distance"], key=location_id) settings = ObservationSettings( observation_path="wasbs://[email protected]/sample_data/green_tripdata_2020-04_with_index.csv", event_timestamp_column="lpep_dropoff_datetime", timestamp_format="yyyy-MM-dd HH:mm:ss")
client.get_offline_features(observation_settings=settings, feature_query=feature_query, output_path=output_path )
There are various features getting pulled in two different methods. If we have to register the features w.r.t online or offline feature store, then we need to send variable parameters.
Please suggest.

We could pass the consumption input/output information to the callback as the callback context. Given the main use cases, I think it's reasonable.
Also it can be less error-prone, e.g., if you add the callback in an ad-hoc fashion:

def record_consumption(feature_list):
    # Record list of features consumption to somewhere
    ...
res = client.get_online_features('nycTaxiDemoFeature', '265', ['f_location_avg_fare', 'f_location_max_fare'],
    callback = record_consumption, params=['f_location_avg_fare', 'f_location_max_fare'])

You need to provide feature list twice and they could be inconsistent then you record everything wrong.
I strongly recommend an accurate design for the clear scenarios, over of a overgeneric solution that can do anything. The latter will always be abused in an unexpected way and you can do nothing about it.

I understand that the generic solution could do anything. But, the intention of this callback was to make it generic enough so that the capability of the handlers could be enhanced by calling other rest end points without any specific usecase.

i am not able to visualize what could be breaking with this callback function in the Feathr implementations. Would you like to help me with some scenarios on this please.

So far nothing will be breaking by this change because we're talking about adding instead of changing or removing functions, I was talking about the design solely.

As to generic solution, I think nothing can be more generic than following:

client.get_online_features(...)
do_something(some_param)

And it doesn't require any change.

With this PR, the code doesn't look so different:

client.get_online_features(..., do_something, some_param)

Pretty much like we merge 2 lines into 1.

So I'm really confused by the purpose of this PR.

[chinmaytredence]

@windoze Please see the below code snippet
res = client.get_online_features('nycTaxiDemoFeature', '265', ['f_location_avg_fare', 'f_location_max_fare'])
and
feature_query = FeatureQuery( feature_list=["f_location_avg_fare", "f_trip_distance_rounded", "f_is_long_trip_distance"], key=location_id) settings = ObservationSettings( observation_path="wasbs://[email protected]/sample_data/green_tripdata_2020-04_with_index.csv", event_timestamp_column="lpep_dropoff_datetime", timestamp_format="yyyy-MM-dd HH:mm:ss")
client.get_offline_features(observation_settings=settings, feature_query=feature_query, output_path=output_path )
There are various features getting pulled in two different methods. If we have to register the features w.r.t online or offline feature store, then we need to send variable parameters.
Please suggest.

We could pass the consumption input/output information to the callback as the callback context. Given the main use cases, I think it's reasonable.
Also it can be less error-prone, e.g., if you add the callback in an ad-hoc fashion:

def record_consumption(feature_list):
    # Record list of features consumption to somewhere
    ...
res = client.get_online_features('nycTaxiDemoFeature', '265', ['f_location_avg_fare', 'f_location_max_fare'],
    callback = record_consumption, params=['f_location_avg_fare', 'f_location_max_fare'])

You need to provide feature list twice and they could be inconsistent then you record everything wrong.
I strongly recommend an accurate design for the clear scenarios, over of a overgeneric solution that can do anything. The latter will always be abused in an unexpected way and you can do nothing about it.

I understand that the generic solution could do anything. But, the intention of this callback was to make it generic enough so that the capability of the handlers could be enhanced by calling other rest end points without any specific usecase.
i am not able to visualize what could be breaking with this callback function in the Feathr implementations. Would you like to help me with some scenarios on this please.

So far nothing will be breaking by this change because we're talking about adding instead of changing or removing functions, I was talking about the design solely.

As to generic solution, I think nothing can be more generic than following:

client.get_online_features(...)
do_something(some_param)

And it doesn't require any change.

With this PR, the code doesn't look so different:

client.get_online_features(..., do_something, some_param)

Pretty much like we merge 2 lines into 1.

So I'm really confused by the purpose of this PR.

When we write some other piece of code apart from the client handler as below:
client.get_online_features(...) do_something(some_param)
we cannot justify that the notebook is using feathr. As the do_something is just a function which is called by the notebook. But not from feathr client. When we use FeathrClient, we can justify that the consumer is really from feathr client not from anywhere else.

[windoze]

I'm even more confused, isn't the code in the notebook written by the user?

Even if the callback is added, how can you prevent user from calling the callback function directly?

[chinmaytredence]

I'm even more confused, isn't the code in the notebook written by the user?

Even if the callback is added, how can you prevent user from calling the callback function directly?

Good point.

I think we could use the

client.get_online_features('nycTaxiDemoFeature', '265', ['f_location_avg_fare', 'f_location_max_fare'], callback = record_consumption)

and record_cosumption will take the features from the third parameter itself instead of we passing the features in the params.

But what if we would like to send the path of notebook or the url of the notebook ? That's where the use case coming up where we can package all the details in one place instead of writing the callback function as a separate line as below:

client.get_online_features(...)
record_consumption(...)

[windoze]

I'm even more confused, isn't the code in the notebook written by the user?
Even if the callback is added, how can you prevent user from calling the callback function directly?

Good point.

I think we could use the

client.get_online_features('nycTaxiDemoFeature', '265', ['f_location_avg_fare', 'f_location_max_fare'], callback = record_consumption)

and record_cosumption will take the features from the third parameter itself instead of we passing the features in the params.

But what if we would like to send the path of notebook or the url of the notebook ? That's where the use case coming up where we can package all the details in one place instead of writing the callback function as a separate line as below:

client.get_online_features(...) record_consumption(...)

Before the PR, we write:

client.get_online_features(...)
record_consumption(params)

After the PR, we write:

client.get_online_features(..., record_consumption, params)

Whatever the info you need to provided in the former, you'll need the same in the latter, the only difference is "one line" or "two lines", right?

If you want to use implicit parameters, you won't need to set callback in every get_online_feature call, because you won't be able to send the variable payload anyway. Why don't make it class level for less error-prone and more convenient?

[chinmaytredence]

I'm even more confused, isn't the code in the notebook written by the user?
Even if the callback is added, how can you prevent user from calling the callback function directly?

Good point.
I think we could use the
client.get_online_features('nycTaxiDemoFeature', '265', ['f_location_avg_fare', 'f_location_max_fare'], callback = record_consumption)
and record_cosumption will take the features from the third parameter itself instead of we passing the features in the params.
But what if we would like to send the path of notebook or the url of the notebook ? That's where the use case coming up where we can package all the details in one place instead of writing the callback function as a separate line as below:
client.get_online_features(...) record_consumption(...)

Before the PR, we write:

client.get_online_features(...)
record_consumption(params)

After the PR, we write:

client.get_online_features(..., record_consumption, params)

Whatever the info you need to provided in the former, you'll need the same in the latter, the only difference is "one line" or "two lines", right?

Not necessarily on two lines. It is about the origin of your call stack. who is calling it. That makes it feathr client compliant.

[windoze]

I'm even more confused, isn't the code in the notebook written by the user?
Even if the callback is added, how can you prevent user from calling the callback function directly?

Good point.
I think we could use the
client.get_online_features('nycTaxiDemoFeature', '265', ['f_location_avg_fare', 'f_location_max_fare'], callback = record_consumption)
and record_cosumption will take the features from the third parameter itself instead of we passing the features in the params.
But what if we would like to send the path of notebook or the url of the notebook ? That's where the use case coming up where we can package all the details in one place instead of writing the callback function as a separate line as below:
client.get_online_features(...) record_consumption(...)

Before the PR, we write:

client.get_online_features(...)
record_consumption(params)

After the PR, we write:

client.get_online_features(..., record_consumption, params)

Whatever the info you need to provided in the former, you'll need the same in the latter, the only difference is "one line" or "two lines", right?

Not necessarily on two lines. It is about the origin of your call stack. who is calling it. That makes it feathr client compliant.

Could you please make a working sample? I'm not sure if I fully understand your point.

[chinmaytredence]

The philosophy is, we are using Feathr client for all our operations. Be it register feature, or build feature, or get features. Instead user could directly use the lower level functions by passing the environment variables.

Adhering to this, when we are going for any callbacks, I think feathr client is the best place to attach the callback and params. And I am confused where is the design issue in this ?

[windoze]

The design issue is we don't need to add this callback, because we can always simply call whatever right after the Feathr call.

This PR simply chains 2 function calls into 1 without any additional logic, so I think it's pointless.

If you're talking about missing consumption statistics, this PR doesn't help as user may still forget to add the callback into the get_online_features parameters; on the other hand, if you make these 2 parameters non-optional, that brings burden to anyone who doesn't use them.

If you're talking about consumption forgery, this PR also doesn't help as user can still call the function to record fake consumptions directly.

I don't see any issue resolved by this PR, that's why I put all these comments.

[chinmaytredence]

The design issue is we don't need to add this callback, because we can always simply call whatever right after the Feathr call.

This PR simply chains 2 function calls into 1 without any additional logic, so I think it's pointless.

If you're talking about missing consumption statistics, this PR doesn't help as user may still forget to add the callback into the get_online_features parameters; on the other hand, if you make these 2 parameters non-optional, that brings burden to anyone who doesn't use them.

If you're talking about consumption forgery, this PR also doesn't help as user can still call the function to record fake consumptions directly.

I don't see any issue resolved by this PR, that's why I put all these comments.

This PR simply chains 2 function calls into 1 without any additional logic, so I think it's pointless.

Don't you think this gives us the capability to extend the capability of client by enabling it with rest api handlers ?

We have separate set of code which does the registration of features in the purview. But this PR is mostly for extending the capability to call a rest api with params.

[windoze]

The design issue is we don't need to add this callback, because we can always simply call whatever right after the Feathr call.
This PR simply chains 2 function calls into 1 without any additional logic, so I think it's pointless.
If you're talking about missing consumption statistics, this PR doesn't help as user may still forget to add the callback into the get_online_features parameters; on the other hand, if you make these 2 parameters non-optional, that brings burden to anyone who doesn't use them.
If you're talking about consumption forgery, this PR also doesn't help as user can still call the function to record fake consumptions directly.
I don't see any issue resolved by this PR, that's why I put all these comments.

This PR simply chains 2 function calls into 1 without any additional logic, so I think it's pointless. Don't you think this gives us the capability to extend the capability of client by enabling it with rest api handlers ?

it doesn't extend any capability, because logically
get_online_features(..., cb, params)
is exactly equivalent to

get_online_features(...)
cb(params)

[chinmaytredence]

The design issue is we don't need to add this callback, because we can always simply call whatever right after the Feathr call.
This PR simply chains 2 function calls into 1 without any additional logic, so I think it's pointless.
If you're talking about missing consumption statistics, this PR doesn't help as user may still forget to add the callback into the get_online_features parameters; on the other hand, if you make these 2 parameters non-optional, that brings burden to anyone who doesn't use them.
If you're talking about consumption forgery, this PR also doesn't help as user can still call the function to record fake consumptions directly.
I don't see any issue resolved by this PR, that's why I put all these comments.

This PR simply chains 2 function calls into 1 without any additional logic, so I think it's pointless. Don't you think this gives us the capability to extend the capability of client by enabling it with rest api handlers ?

it doesn't extend any capability, because logically get_online_features(..., cb, params) is exactly equivalent to

get_online_features(...)
cb(params)

We can similarly say that instead of client.register_features(...)

we can just call the purview .

My whole point is atleast some how we make sure that the usage of Feathrclient is helping us to regsiter the consumption layer as well till we tightly couple the consumption layer code.

[windoze]

We can similarly say that instead of client.register_features(...)

we can just call the purview .

My whole point is atleast some how we make sure that the usage of Feathrclient is helping us to regsiter the consumption layer as well till we tightly couple the consumption layer code.

Theoretically yes, as long as you're willing to reimplement all the logic to make sure your output is still compatible with Feathr.

I can understand your point, but this PR doesn't do any help to your point, it just saved 1 keystroke by replacing a pair of braces with a comma.

[chinmaytredence]

How this PR helps me ?

I have one Fast API layer which has used feather as library and implemented the consumer_file url registration to purview registry with a new typedef.

When the FeathrClient gets the ability to register a callback function, my notebook users would be able to use client.get_onlinefeatures(...,cb,params) which will enable me to call the fast api layer, which will do mulitple stuff in the background. Be it registering to purview, be it putting who is using it in my local db as well as sending one email etc.

One more thing, while we are provisioning the cluster we will configure feathr client with the callback so the users will not be going through setting up feathr. Rather they will be more focused on the feature usage and model evaluations

[windoze]

How about you try to use your Fast API layer and call it by using this?

client.get_online_features(...)
cb(params)

Does it still do the same help to you?

[chinmaytredence]

How about you try to use your Fast API layer and call it by using this?

client.get_online_features(...)
cb(params)

Does it still do the same help to you?

yes it does the same thing. But, the philosophy is to use FeathrClient always whenever user wants to connect for feathr related stuff. This makes the usage easier and user will not be going to forget as well

[windoze]

How about you try to use your Fast API layer and call it by using this?

client.get_online_features(...)
cb(params)

Does it still do the same help to you?

yes it does the same thing. But, the philosophy is to use FeathrClient always whenever use want to connect for feathr related stuff.

That's why I used the word 'pointless' because logically this PR does nothing.

My suggestions is, either we define a clear interface to systematically collect all consumptions, or, we let our users to record consumption by themselves, I put the suggestions about the former in previous comments, we can talk more if you're interested, but adding a half-baked function may not be a good idea.

[chinmaytredence]

How about you try to use your Fast API layer and call it by using this?

client.get_online_features(...)
cb(params)

Does it still do the same help to you?

yes it does the same thing. But, the philosophy is to use FeathrClient always whenever use want to connect for feathr related stuff.

That's why I used the word 'pointless' because logically this PR does nothing.

My suggestions is, either we define a clear interface to systematically collect all consumptions, or, we let our users to record consumption by themselves, I put the suggestions about the former in previous comments, we can talk more if you're interested, but adding a half-baked function may not be a good idea.

Then could you help me understand how the Feathrclient would be extendable with a rest api handler. Currently it is not able to give us any handler. Just think in the direction of Webhooks.

[windoze]

How about you try to use your Fast API layer and call it by using this?

client.get_online_features(...)
cb(params)

Does it still do the same help to you?

yes it does the same thing. But, the philosophy is to use FeathrClient always whenever use want to connect for feathr related stuff.

That's why I used the word 'pointless' because logically this PR does nothing.
My suggestions is, either we define a clear interface to systematically collect all consumptions, or, we let our users to record consumption by themselves, I put the suggestions about the former in previous comments, we can talk more if you're interested, but adding a half-baked function may not be a good idea.

Then could you help me understand how the Feathrclient would be extendable with a rest api handler. Currently it is not able to give us any handler.

Are you going to extend a REST API service or a client with Feathr? Feathr itself has many REST clients inside and there shouldn't be any trouble to add another one.

As to the REST service, it's totally another topic, and I can hardly understand why do we want to do it.

[chinmaytredence]

How about you try to use your Fast API layer and call it by using this?

client.get_online_features(...)
cb(params)

Does it still do the same help to you?

yes it does the same thing. But, the philosophy is to use FeathrClient always whenever use want to connect for feathr related stuff.

That's why I used the word 'pointless' because logically this PR does nothing.
My suggestions is, either we define a clear interface to systematically collect all consumptions, or, we let our users to record consumption by themselves, I put the suggestions about the former in previous comments, we can talk more if you're interested, but adding a half-baked function may not be a good idea.

Then could you help me understand how the Feathrclient would be extendable with a rest api handler. Currently it is not able to give us any handler.

Are you going to extend a REST API service or a client with Feathr? Feathr itself has many REST clients inside and there shouldn't be any trouble to add another one.

As to the REST service, it's totally another topic, and I can hardly understand why do we want to do it.

May be think in the direction of Webhook concept.