Skip to content
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

Support convertion from []interface to list type. #226

Merged
merged 3 commits into from
Mar 4, 2022

Conversation

ariefrahmansyah
Copy link
Contributor

What this PR does / why we need it:

Given a first successful standard transformer request that has feast feature retrieval with list types:

➜ curl -X POST http://localhost:8081/v1/models/echo:predict -d '{"driver_id":1}'
{"merlin":[{"merlin_test_driver_features:test_double":3.14,"merlin_test_driver_features:test_double_list":[1.23,2.34,3.45],"merlin_test_driver_features:test_int64":null,"merlin_test_driver_features:test_int64_list":null,"merlin_test_driver_features:test_string":null,"merlin_test_driver_features:test_string_list":["AAABBBCCC"],"merlin_test_driver_id":"1"}]}

And feast cache is enabled, and we send the same request, it will return error:

➜ curl -X POST http://localhost:8081/v1/models/echo:predict -d '{"driver_id":1}'
{"code":500,"message":"preprocessing error: error executing preprocessing operation: *pipeline.FeastOp: unsupported conversion from []interface {} to []float64"}

This is because, on the second request, standard transformer uses cached feature value which stored as []interface instead of the correct Feast's list type.

Checklist

  • Added unit test, integration, and/or e2e tests
  • Tested locally

This feature is needed when we uses cached feast list value as we stored it in []interface instead of the correct Feast's list type.
@pradithya
Copy link
Member

Can you add test that reproduce the issue, those new unit tests seems didn't cover the bug.

@pradithya
Copy link
Member

pradithya commented Mar 3, 2022

Can you also handle it in feature cache (https://github.com/gojek/merlin/blob/a382020bfa5f660467103a21d68b12b78c8f47ef/api/pkg/transformer/feast/feature_cache.go#L159) so that anything return by cache is already correct.

It's currently called castIntValue since it's only for handling int, but it seems necessary to handle the list type conversion there too.

@pradithya
Copy link
Member

Awesome! LGTM!

@tiopramayudi
Copy link
Collaborator

LGTM

@ariefrahmansyah ariefrahmansyah merged commit 82a95fe into main Mar 4, 2022
@ariefrahmansyah ariefrahmansyah deleted the convert-list-interfaces branch March 4, 2022 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants