Add an interface for TransformationService and a basic implementation #1932

achals · 2021-10-11T18:50:39Z

Signed-off-by: Achal Shah achals@gmail.com

What this PR does / why we need it:

Working Example:

$ grpcurl -plaintext  -d '{"transformation_name": "conv_rate_plus_100"}' localhost:6569 feast.serving.TransformationService/TransformFeatures
{
  "transformation_output": {
    "arrow_value": "QVJST1cxAAD/////iAIAABAAAAAAAAoADgAGAAUACAAKAAAAAAEEABAAAAAAAAoADAAAAAQACAAKAAAAAAIAAAQAAAABAAAADAAAAAgADAAEAAgACAAAANgBAAAEAAAAywEAAHsiaW5kZXhfY29sdW1ucyI6IFt7ImtpbmQiOiAicmFuZ2UiLCAibmFtZSI6IG51bGwsICJzdGFydCI6IDAsICJzdG9wIjogMSwgInN0ZXAiOiAxfV0sICJjb2x1bW5faW5kZXhlcyI6IFt7Im5hbWUiOiBudWxsLCAiZmllbGRfbmFtZSI6IG51bGwsICJwYW5kYXNfdHlwZSI6ICJ1bmljb2RlIiwgIm51bXB5X3R5cGUiOiAib2JqZWN0IiwgIm1ldGFkYXRhIjogeyJlbmNvZGluZyI6ICJVVEYtOCJ9fV0sICJjb2x1bW5zIjogW3sibmFtZSI6ICJjb252X3JhdGVfcGx1c18xMDAiLCAiZmllbGRfbmFtZSI6ICJjb252X3JhdGVfcGx1c18xMDAiLCAicGFuZGFzX3R5cGUiOiAiZmxvYXQ2NCIsICJudW1weV90eXBlIjogImZsb2F0NjQiLCAibWV0YWRhdGEiOiBudWxsfV0sICJjcmVhdG9yIjogeyJsaWJyYXJ5IjogInB5YXJyb3ciLCAidmVyc2lvbiI6ICI1LjAuMCJ9LCAicGFuZGFzX3ZlcnNpb24iOiAiMS4zLjEifQAGAAAAcGFuZGFzAAABAAAAFAAAABAAFAAIAAYABwAMAAAAEAAQAAAAAAABAxAAAAAsAAAABAAAAAAAAAASAAAAY29udl9yYXRlX3BsdXNfMTAwAAAAAAYACAAGAAYAAAAAAAIAAAAAAP////+IAAAAFAAAAAAAAAAMABYABgAFAAgADAAMAAAAAAMEABgAAAAIAAAAAAAAAAAACgAYAAwABAAIAAoAAAA8AAAAEAAAAAEAAAAAAAAAAAAAAAIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAIAAAAAAAAAAAAAAABAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAIFlA/////wAAAAAQAAAADAAUAAYACAAMABAADAAAAAAABAA8AAAAKAAAAAQAAAABAAAAmAIAAAAAAACQAAAAAAAAAAgAAAAAAAAAAAAAAAAAAAAAAAoADAAAAAQACAAKAAAAAAIAAAQAAAABAAAADAAAAAgADAAEAAgACAAAANgBAAAEAAAAywEAAHsiaW5kZXhfY29sdW1ucyI6IFt7ImtpbmQiOiAicmFuZ2UiLCAibmFtZSI6IG51bGwsICJzdGFydCI6IDAsICJzdG9wIjogMSwgInN0ZXAiOiAxfV0sICJjb2x1bW5faW5kZXhlcyI6IFt7Im5hbWUiOiBudWxsLCAiZmllbGRfbmFtZSI6IG51bGwsICJwYW5kYXNfdHlwZSI6ICJ1bmljb2RlIiwgIm51bXB5X3R5cGUiOiAib2JqZWN0IiwgIm1ldGFkYXRhIjogeyJlbmNvZGluZyI6ICJVVEYtOCJ9fV0sICJjb2x1bW5zIjogW3sibmFtZSI6ICJjb252X3JhdGVfcGx1c18xMDAiLCAiZmllbGRfbmFtZSI6ICJjb252X3JhdGVfcGx1c18xMDAiLCAicGFuZGFzX3R5cGUiOiAiZmxvYXQ2NCIsICJudW1weV90eXBlIjogImZsb2F0NjQiLCAibWV0YWRhdGEiOiBudWxsfV0sICJjcmVhdG9yIjogeyJsaWJyYXJ5IjogInB5YXJyb3ciLCAidmVyc2lvbiI6ICI1LjAuMCJ9LCAicGFuZGFzX3ZlcnNpb24iOiAiMS4zLjEifQAGAAAAcGFuZGFzAAABAAAAFAAAABAAFAAIAAYABwAMAAAAEAAQAAAAAAABAxAAAAAsAAAABAAAAAAAAAASAAAAY29udl9yYXRlX3BsdXNfMTAwAAAAAAYACAAGAAYAAAAAAAIAsAIAAEFSUk9XMQ=="
  }
}

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

feast-ci-bot · 2021-10-11T18:50:40Z

@achals: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

woop · 2021-10-11T19:55:29Z

sdk/python/feast/cli.py

@@ -423,6 +423,20 @@ def serve_command(ctx: click.Context, port: int):
    store.serve(port)


+@cli.command("serve_transformations")


Any reason to use a different serving (compared to just serve)?

Just so that the transformation server can be started up separately from the feature server. I can collapse in a single command if that'd be simpler

Personally I like the idea of rolling it into a single server so that users don't need to run multiple servers during dev.

I think we do want the option though because this is also intended to work standalone when users deploy to higher scale? during dev users probably don't need an FTS at all since it's covered during the regular get_online_features flow?

could see that we expose a debug endpoint but that seems slightly different

I'm going to stick to a separate command and we can revisit if it becomes too cumbersome during dev cycles, unless anyone feels strongly.

adchia

generally lgtm!

adchia · 2021-10-12T15:52:04Z

protos/feast/serving/TransformationService.proto

+
+message GetTransformationServiceInfoRequest {}
+
+message GetTransformationServiceInfoResponse {


this makes sense for the most advanced user.

for a user just getting started, i'd want maybe an "EMBEDDED" mode too where transformations happen on the feature server itself?

You mean in the python feature server, right? How would that be different from just running get_online_features pointing to a python feature server API, which should look up the values and do the transformation?

maybe the earlier question is what is the BATCH or ONLINE mode used for? seems like in both cases, we're applying the same row-level udf to all values?

protos/feast/serving/TransformationService.proto

adchia · 2021-10-12T15:55:22Z

protos/feast/serving/TransformationService.proto

+    string transformation_name = 1;
+    string project = 2;
+
+    ValueType transformation_input = 3;


Assuming that this input is WIP? since you call .arrow_value below on this, which ValueType doesn't have.

Are you envisioning the FTS would communicate with the registry? My preference is probably to start without to have as many stateless servers as possible

unless of course we run into performance issues, in which case having a cached registry would be needed

It would have to get the registry at least once because that's how it would unpickle the transformations, right?

hm yeah it needs the udf definition which we currently store in the FTS.

no strong feelings. an alternative would be to pass the udf directly in the request so the FTS becomes purely about execution. though at that point, we may consider using a ray cluster directly instead of managing an FTS.

If both this and the FTS can pull the registry, wondering if there can now be synchronization issues where e.g. a feature server that calls into this has a different version of the registry than what is on the FTS.

Passing in the UDF seems like a performance and dependency nightmare - I think we should stick with a simple approach for now assuming access to the registry. If we don't want to do that then yea we can farm out to a ray cluster from the client directly.

Re: sync issues, i think it's possible, but I don't know if we want to solve that problem now.

sgtm for the first cut

unclear imo though whether udf passing would be more complex.
e.g .the dependency issues we'd run into today anyways on the regular feature server no?
e.g. performance here referring to payload increases causing issues? payload size isn't really directly correlated with network latency, and it probably will be relatively negligible esp if users are passing in large sets of entity keys.

sdk/python/feast/feature_store.py

sdk/python/feast/transformation_server.py

codecov-commenter · 2021-10-12T17:24:20Z

Codecov Report

Merging #1932 (9a21fa2) into master (cd0e0e0) will decrease coverage by 0.31%.
The diff coverage is 39.65%.

@@            Coverage Diff             @@
##           master    #1932      +/-   ##
==========================================
- Coverage   82.26%   81.94%   -0.32%     
==========================================
  Files          96       97       +1     
  Lines        7669     7727      +58     
==========================================
+ Hits         6309     6332      +23     
- Misses       1360     1395      +35

Flag	Coverage Δ
integrationtests	`74.07% <37.50%> (-0.09%)`	⬇️
unittests	`59.24% <39.65%> (-0.14%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
sdk/python/feast/repo_config.py	`78.97% <ø> (ø)`
sdk/python/feast/feature_store.py	`93.30% <25.00%> (-1.25%)`	⬇️
sdk/python/feast/transformation_server.py	`40.47% <40.47%> (ø)`
sdk/python/feast/cli.py	`39.00% <50.00%> (+0.30%)`	⬆️
sdk/python/feast/infra/utils/aws_utils.py	`71.01% <0.00%> (-0.73%)`	⬇️
sdk/python/tests/conftest.py	`95.31% <0.00%> (+1.56%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd0e0e0...9a21fa2. Read the comment docs.

felixwang9817 · 2021-10-13T23:26:08Z

protos/feast/serving/TransformationService.proto

+    // Feast version of this transformation service deployment.
+    string version = 1;
+
+    // Type of serving deployment, either ONLINE or BATCH. Different store types support different


not sure what this comment means - ONLINE and BATCH also don't match up with the TransformationServiceType options below?

sdk/python/feast/feature_store.py

felixwang9817 · 2021-10-13T23:28:23Z

sdk/python/feast/repo_config.py

@@ -76,6 +76,19 @@ class RegistryConfig(FeastBaseModel):
     expire. Users can manually refresh the cache by calling feature_store.refresh_registry() """


+class TransformationServerConfig(FeastBaseModel):
+    """Server Configuration that determines to how the transformation server is configured."""


nit: "determines how"

felixwang9817 · 2021-10-13T23:28:30Z

sdk/python/feast/repo_config.py

+
+
+class ServerConfig(FeastBaseModel):
+    """Server Configuration that determines to how feast servers are configured. """


nit: same as above

felixwang9817 · 2021-10-13T23:31:55Z

sdk/python/feast/repo_config.py

@@ -175,6 +194,11 @@ def _validate_online_store_config(cls, values):
                [ErrorWrapper(e, loc="online_store")], model=RepoConfig,
            )

+        if "servers" not in values:


if we're validating servers here, shouldn't we have a separate validate_servers_config function?

felixwang9817 · 2021-10-13T23:35:59Z

sdk/python/feast/transformation_server.py

+            context.set_code(grpc.StatusCode.INVALID_ARGUMENT)
+            raise
+
+        assert odfv


nit: throw an exception instead of assert

Signed-off-by: Achal Shah <achals@gmail.com>

felixwang9817

/lgtm

feast-ci-bot · 2021-10-14T21:26:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: achals, felixwang9817

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [achals,felixwang9817]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

felixwang9817 · 2021-10-14T22:08:25Z

/kind feature

feast-ci-bot added do-not-merge/work-in-progress do-not-merge/release-note-label-needed approved needs-kind size/L labels Oct 11, 2021

woop reviewed Oct 11, 2021

View reviewed changes

achals changed the title ~~Add an interface for TransformationService, along with a basic implem…~~ Add an interface for TransformationService and a basic implementation Oct 11, 2021

achals force-pushed the achal/transformation-service branch from a0a0216 to 26ef01c Compare October 11, 2021 21:57

adchia reviewed Oct 12, 2021

View reviewed changes

feast-ci-bot added size/XXL and removed size/L labels Oct 12, 2021

achals marked this pull request as ready for review October 13, 2021 19:22

achals requested review from felixwang9817, tsotnet and a team as code owners October 13, 2021 19:22

feast-ci-bot removed the do-not-merge/work-in-progress label Oct 13, 2021

felixwang9817 reviewed Oct 13, 2021

View reviewed changes

sdk/python/feast/feature_store.py Outdated Show resolved Hide resolved

felixwang9817 reviewed Oct 13, 2021

View reviewed changes

achals added 8 commits October 14, 2021 14:03

rebase

6f07a81

Signed-off-by: Achal Shah <achals@gmail.com>

upadtes

227ab15

Signed-off-by: Achal Shah <achals@gmail.com>

Update the protos

6f4bffa

Signed-off-by: Achal Shah <achals@gmail.com>

CR comments, change names

e19ac44

Signed-off-by: Achal Shah <achals@gmail.com>

tests

ae55967

Signed-off-by: Achal Shah <achals@gmail.com>

tests

b5bdd3a

Signed-off-by: Achal Shah <achals@gmail.com>

update comment

483892b

Signed-off-by: Achal Shah <achals@gmail.com>

cr updates

cc9f084

Signed-off-by: Achal Shah <achals@gmail.com>

kill config

2a73ae7

Signed-off-by: Achal Shah <achals@gmail.com>

achals force-pushed the achal/transformation-service branch from 148dadc to 2a73ae7 Compare October 14, 2021 21:13

achals added 4 commits October 14, 2021 14:16

revive config

8770015

Signed-off-by: Achal Shah <achals@gmail.com>

revive config

907aad4

Signed-off-by: Achal Shah <achals@gmail.com>

kill config

9e5a623

Signed-off-by: Achal Shah <achals@gmail.com>

kill config

9a21fa2

Signed-off-by: Achal Shah <achals@gmail.com>

felixwang9817 approved these changes Oct 14, 2021

View reviewed changes

feast-ci-bot assigned felixwang9817 Oct 14, 2021

feast-ci-bot added the lgtm label Oct 14, 2021

feast-ci-bot added kind/feature New feature or request and removed needs-kind labels Oct 14, 2021

feast-ci-bot merged commit df724a8 into feast-dev:master Oct 14, 2021

felixwang9817 mentioned this pull request Oct 18, 2021

Add support for on demand transforms using the Python FTS feast-dev/feast-java-old#40

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an interface for TransformationService and a basic implementation #1932

Add an interface for TransformationService and a basic implementation #1932

achals commented Oct 11, 2021 •

edited

Loading

feast-ci-bot commented Oct 11, 2021

woop Oct 11, 2021

achals Oct 11, 2021

woop Oct 13, 2021

adchia Oct 13, 2021

achals Oct 13, 2021

adchia left a comment

adchia Oct 12, 2021

achals Oct 12, 2021

adchia Oct 13, 2021

adchia Oct 12, 2021

adchia Oct 12, 2021

achals Oct 12, 2021

adchia Oct 13, 2021

achals Oct 13, 2021

adchia Oct 13, 2021

codecov-commenter commented Oct 12, 2021 •

edited

Loading

felixwang9817 Oct 13, 2021

felixwang9817 Oct 13, 2021

felixwang9817 Oct 13, 2021

felixwang9817 Oct 13, 2021

felixwang9817 Oct 13, 2021

felixwang9817 left a comment

feast-ci-bot commented Oct 14, 2021

felixwang9817 commented Oct 14, 2021

		@@ -423,6 +423,20 @@ def serve_command(ctx: click.Context, port: int):
		store.serve(port)


		@cli.command("serve_transformations")


		message GetTransformationServiceInfoRequest {}

		message GetTransformationServiceInfoResponse {



		class ServerConfig(FeastBaseModel):
		"""Server Configuration that determines to how feast servers are configured. """

Add an interface for TransformationService and a basic implementation #1932

Add an interface for TransformationService and a basic implementation #1932

Conversation

achals commented Oct 11, 2021 • edited Loading

feast-ci-bot commented Oct 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adchia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 12, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixwang9817 left a comment

Choose a reason for hiding this comment

feast-ci-bot commented Oct 14, 2021

felixwang9817 commented Oct 14, 2021

achals commented Oct 11, 2021 •

edited

Loading

codecov-commenter commented Oct 12, 2021 •

edited

Loading