-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OnDemandFeatureViews & online async feature retrieval & passing unknown entity values --> raises TypeError when it should not #4473
Comments
Hey @tokoko, I see that you were looking into this. Did you already manage to learn something about this issue? No worries if you haven't had the time of course! Wouldn't want to pressure you :) |
I've updated the description with an alternative re-producable method, let me know whether this helps! |
Yeah, I get why this could happen. I'll try to make a patch for it. Should be reasonable. |
We'll maybe log skipping the odfv |
Nice, good to hear! Let me know if I can assist with anything.
What do you mean with this comment? I'm not sure I understand. @franciscojavierarceo |
@franciscojavierarceo Any update on this? 🙏 |
Ah yes sorry I spent a bit of time implementing something else and now that I'm mostly done there I can take this on. |
Okay, let me give an update here. I actually added a test for this in this PR:
I'm not entirely sure what's the best solution here because the exception alerts that the data is not available and the client can choose to handle the exception as they please (i..e., this is very explicit). The alternative is that we return None. If we do that, I would want to adde metadata outlining that this was "Entity Not Found" or something in the database so that the behavior was explicit about it (though people would potentially not care). @tokoko any thoughts here? My concern is that existing users may have logic handling this already and the decision here favors resiliency over being explicit. I personally find handling the exception to be a good thing but I also don't want to necessarily make that decision for other users. We can have a configuration in the |
imho this is clearly a bug that's happening in an untested corner case. feast really has no business calling |
@tokoko I can take this one on since I'm already deeply in this codebase and plan on refactoring stuff anyways. Unless you want to take it on of course. |
@franciscojavierarceo sure, feel free to take it |
Fixed here: #4667 |
Context
Factors to Consider
get_online_features_async
method on theFeatureStore
.driver_id
1
and2
are materialized in the online store. However, I’m passing99999
.OnDemandFeatureView
.Expected Behavior
When only points 1 and 2 are true, I get a response that has
None
values for all the features. This functionality works as expected.When all three points are true, I would also expect to get a response that has
None
values for all the features. However, this is not the case.Current Behavior
When only points 1 and 2 are true, I get a response that has
None
values for all the features. This functionality works as expected.I would expect this to be true as well when using
OnDemandFeatureViews
(ODFVs). However, when all three points are true, the following error is raised:TypeError: Couldn't infer value type from empty value
.Steps to Reproduce
This branch contains an additional integration test named
test_async_online_retrieval_with_event_timestamps_null_only
, which acts as a minimal failing example.The error is raised when the
python_values_to_proto_values
method is called in thesdk/python/feast/utils.py
file, which is invoked by theget_online_features_async
method in thesdk/python/feast/infra/online_stores/online_store.py
file (these locations are marked with# breakpoint()
in the linked branch).Furthermore, this error is only happening when we are "only" passing unknown entity values. For example, if we are only passing the unknown entity value 99999, it will fail. If we pass the known entity value 1 and the unknown value 99999, it will be successful.
You can run this test by creating a virtual environment, and run this command in the shell:
PYTHONPATH='.' \ FULL_REPO_CONFIGS_MODULE=sdk.python.feast.infra.online_stores.contrib.postgres_repo_configuration \ PYTEST_PLUGINS=sdk.python.tests.integration.feature_repos.universal.online_store.postgres \ python -m pytest --integration sdk/python/tests/integration/online_store/test_universal_online.py::test_async_online_retrieval_with_event_timestamps_null_only
Another method to re-produce:
docker-compose.yml
feature_store.yml
Insert into offline store (postgres)
postgres_init/create-offline-store-database.sql
bootstrap.py
materialize
inference
Specifications
Possible Solution
I’m not entirely sure why
ValueType.UNKNOWN
is passed to thefeature_type
argument of thepython_values_to_proto_values
method. If we were to pass another value, I believe the method would succeed, as theif
statement that raises the error would not be triggered.The text was updated successfully, but these errors were encountered: