Repository navigation
Replace string references used in non user facing components in Feast #674
Description
Background
Currently Feast uses string references in 3 areas:
- Feature Set References
- Feature References
- Subscriptions
As a result many implementations of string reference parsing and formatting has cropped up all over Feast's code:
- Feast Serving's
RefUtil - Feast Core's
Store - Feast Ingestion's
SpecUtil - Feast Storage Writers:
RedisRedisCluster. - Feast Storage Retrievers
RedisRedisCluster - etc...
Problem
Changes to the feature/feature set references/subscriptions are difficult as we have to track down all the different string parsing/formatting implementations. Changes to these areas should not require a comb over the entire codebase to rectify all these parsing and formatting implementations.
Examples of currently WIP and potential changes that is currently affected by this and why the problem is relevant:
- Feature Set version removal Feature set versioning #386
- Changes to the Feast concept & hierarchy and data model Feast API: Feature references, concept hierarchy, and data model #479
- Removal of Projects from Feature References when in the string form Validate feature reference and project functionality #631
Scope
String references that should still be kept around:
- those exposed by the user facing API (ie the SDKs, config files) as they need to be humanly readable.
- those used by interface with components outside of feast (ie PostgreSQL, BigQuery, etc.), although non string references should be preferred if possible.
Possible Solution
Replace string references in non user facing components with their Protobuf equivalents.
Examples of this include:
- Using message types directly in protobuf definitions:
message FeatureRow {
repeated Field fields = 2;
google.protobuf.Timestamp event_timestamp = 3;
FeatureSetReference feature_set_ref = 8;
string ingestion_id = 7;
}- Using references as keys in maps directly.
HashMap<FeatureSetReference, FeatureSetSpec> featureSetMap = new HashMap<>();
⚠️ There might be possible caveats to this
- Serialize references without having to extract its internals:
String refStr = Base64.getEncoder().encodeToString(featureSetReference.toByteArray());
Activity
[-]Remove string references used in non user facing components in Feast[/-][+]Replace string references used in non user facing components in Feast[/+]woop commentedon May 3, 2020
Some comments:
I agree on the Proto message type, but in the code itself FeatureReferences/FeatureSetReferences should be a non-Proto model class ideally, with
toProto()andfromProto()methods. This would allow us to also have a singletoStringRef()orfromStringRef().FeatureSetRef and FeatureRef seem very similar. I think we could possibly have the following after version removal
Although this would increase nesting, which is also frowned upon by some.
ches commentedon May 4, 2020
I mostly agree with the problem, but I'm not convinced about the solution. Swapping strings with encoded protobuf is merely trading one compatibility issue for another (less readable) one, in my mind. I think we're missing clear stated goals to answer whether the solution will meet them.
Definitely agree with @woop in general on avoiding the generated classes in domain logic if this is part of a solution—we've been moving away from that and although there is overhead (CPU + allocation/GC, and code maintenance), I believe it is a net win. The noted hash key behavior is one of many example pitfalls.
There are perhaps two related problems conflated, it could help to distinguish goals for solving them:
For the first, I believe some goals for improving could be met sheerly through code factoring, encapsulation of the domain concepts of references as first-class types instead of
Strings pervading.The second one is harder, I'm not sure there is an infinitely flexible solution with reasonable complexity (though I'd be glad to hear suggestions), rather some design decisions need to be reached on #479 that we're ready to live with for awhile and/or have a plan for evolving. It's a conceptual problem before it's a technical one. If we're ready to go with #479 (comment) as the first steps, I think that should be a clear part of specs in this issue or an explicit non-goal, since #631 seems meant to cover that at least in part.
With this perspective, the first problem could be appreciably addressed without being blocked by exhaustive answers for #479, except that it might overlap a lot with work for #631. Once goals of this issue are clearly established, it might just become a subtask of #631.
Definitively solving the second is blocked by #479 in such ways that it could put this issue at risk of not being closeable in the near term.
ches commentedon May 4, 2020
Putting this another way, I'd like to explore how this might look with only code changes that are not proto schema changes, then evaluate if and when proto changes should be made. If there can be some win from the first step, it seems more in line with @woop's inclination in #479 (comment) to be conservative about the conceptual + data model direction, to defer until we learn more.
stale commentedon Jul 15, 2020
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.