feat: Update the go feature server from Expedia code repo. #4665

shuchu · 2024-10-22T16:47:39Z

What this PR does / why we need it:

Update the improved Go feature server code from ExpediaGroup/Feast to our code repo.

Which issue(s) this PR fixes:

shuchu · 2024-10-22T16:49:51Z

shuchu · 2024-10-23T03:04:09Z

[Update 10-22-2024] Now it can compile, build and build docker image. All configures in the Makefile under the "#Go SDK & embedded" are working, except the "install-go-ci-dependencies".

The current "install-go-ci-dependencies" is obsoleted. We need to fix it when we setup the integration test for Go feature server.

!!Warning, the current version my not function correctly. need more testing and investigating.

shuchu · 2024-11-01T03:53:59Z

[Update 10-31-2024]

"make test-go" works now
DataDog related observability instrumentation code were commented out but not removed. I keep these code and plan to use OTEL to replace them.

EXPEbdodla · 2024-11-01T16:09:21Z

go/internal/feast/registry/http.go

@@ -0,0 +1,212 @@
+package registry


This needs to be switched to remote registry. We built http registry to overcome the password sharing for SQL Registry. HTTP Registry which is FastAPI based is not contributed to Feast. We are also planning to leverage Remote registry and move over from HTTP one.

Thank you for the info, @EXPEbdodla ! Let me try to resolve this.

I have removed the code of HTTP Registry. And now, only the File based Registry is supported.

shuchu · 2024-11-05T03:27:04Z

[Update 11/04/2024]

The HTTP Registry code is removed as it depends on an external HTTP server as @EXPEbdodla pointed out.
The default Registry is File based one now.

shuchu · 2024-11-08T03:26:52Z

go/internal/feast/featurestore.go

+	}
+	sanitizedProjectName := strings.Replace(config.Project, "_", "-", -1)
+	productName := os.Getenv("PRODUCT")
+	endpoint := fmt.Sprintf("%s-transformations.%s.svc.cluster.local:80", sanitizedProjectName, productName)


Is this an external Transformation server? I guess we need to remove this and related code? @EXPEbdodla

Initially transformations are running with GOPY bindings. That's not working at scale. So we implemented it similar to Java Feature Server.

Thank you, @EXPEbdodla I guess I need to work on this part.

I chose an "easy" fix for this. 😿

Yes. We should get the transformation server endpoint from feature_store.yaml.

updated. :)

Signed-off-by: Shuchu Han <[email protected]>

…ntation code. Signed-off-by: Shuchu Han <[email protected]>

Signed-off-by: Shuchu Han <[email protected]>

…point. Signed-off-by: Shuchu Han <[email protected]>

shuchu · 2024-11-21T04:43:06Z

[Update 11-20-2024]
1, about the transformation service endpoint issue, I couldn't find a better solution now since this is a design choice between the "not working" callback function and using Python based transformation gRPC service. As for now, I use a general string to represent the endpoint. Sorry about this. I will spend more time to understand the code and see if we can do it in a better way.
cc: @EXPEbdodla @tokoko @franciscojavierarceo

HaoXuAI · 2024-11-21T06:30:13Z

go/internal/feast/featurestore.go

@@ -297,6 +315,10 @@ func (fs *FeatureStore) readFromOnlineStore(ctx context.Context, entityRows []*p
 	requestedFeatureViewNames []string,
 	requestedFeatureNames []string,
 ) ([][]onlinestore.FeatureData, error) {
+	// Create a Datadog span from context


I think these can be removed or updated as datadog is not part of feast

Yea. Datadog needs to be replaced with OTEL.

yes, I will replace them with OTEL after this PR get merged.

HaoXuAI · 2024-11-21T06:31:02Z

go/internal/feast/onlinestore/redisonlinestore.go

 			}
 		}
 	}

-	if t == redisNode {
+	// Metrics are not showing up when the service name is set to DD_SERVICE


similar here

HaoXuAI · 2024-11-21T06:32:52Z

go/README.md

@@ -1,109 +1,12 @@
-This directory contains the Go logic that's executed by the `EmbeddedOnlineFeatureServer` from Python.


Any reason we are removing the document here?

That piece of code is not needed if we remove GOPY bindings.

…re.yaml file instead of OS env. Signed-off-by: Shuchu Han <[email protected]>

shuchu · 2024-11-25T04:15:48Z

[Update 11/24/2024]

use the feature_store.yaml file as the source of defining Python transformation service. Example:

# feature_store.yaml file
feature_store:
   transformation_service_endpoint: "localhost:50051"

@EXPEbdodla @feast-dev/reviewers-and-approvers

franciscojavierarceo · 2024-11-26T13:12:41Z

Makefile

+	go install google.golang.org/protobuf/cmd/[email protected]
+	go install google.golang.org/grpc/cmd/[email protected]
+
+install-go-ci-dependencies:


this is all commented out, do we need it?

franciscojavierarceo · 2024-11-26T13:13:38Z

Makefile

+	# go install golang.org/x/tools/cmd/goimports
+	# python -m pip install "pybindgen==0.22.1" "grpcio-tools>=1.56.2,<2" "mypy-protobuf>=3.1"
+
+build-go: compile-protos-go


the inline seems to be at odds with the rest of the Makefile format, could you adjust it?

franciscojavierarceo · 2024-11-26T13:13:43Z

Makefile

+install-feast-ci-locally:
+	pip install -e ".[ci]"
+
+test-go: compile-protos-go compile-protos-python install-feast-ci-locally


franciscojavierarceo · 2024-11-26T13:13:52Z

Makefile

+format-go:
+	gofmt -s -w go/
+
+lint-go: compile-protos-go


franciscojavierarceo · 2024-11-26T13:19:46Z

go/internal/feast/featurestore.go

@@ -161,13 +181,15 @@ func (fs *FeatureStore) GetOnlineFeatures(
 		result = append(result, vectors...)
 	}

-	if fs.transformationCallback != nil {
+	if fs.transformationCallback != nil || fs.transformationService != nil {


When we go to unify the transformations we'll have to update here too #4584

franciscojavierarceo · 2024-11-26T13:27:22Z

go/internal/feast/onlinestore/redisonlinestore.go

+	return featureViewIndices, indicesFeatureView, index
+}
+
+func (r *RedisOnlineStore) buildHsetKeys(featureViewNames []string, featureNames []string, indicesFeatureView map[int]string, index int) ([]string, []string) {


nit:

Suggested change

func (r *RedisOnlineStore) buildHsetKeys(featureViewNames []string, featureNames []string, indicesFeatureView map[int]string, index int) ([]string, []string) {

func (r *RedisOnlineStore) buildRedisHashSetKeys(featureViewNames []string, featureNames []string, indicesFeatureView map[int]string, index int) ([]string, []string) {

franciscojavierarceo · 2024-11-26T13:29:56Z

go/internal/feast/onlinestore/sqliteonlinestore.go

@@ -41,6 +41,7 @@ func NewSqliteOnlineStore(project string, repoConfig *registry.RepoConfig, onlin
 			return nil, fmt.Errorf("cannot find convert sqlite path to string %s", db_path)
 		} else {
 			store.path = fmt.Sprintf("%s/%s", repoConfig.RepoPath, dbPathStr)
+


was this just a lint format change?

franciscojavierarceo · 2024-11-26T13:31:14Z

go/internal/feast/registry/registry.go

 	registryStoreType := registryConfig.RegistryStoreType
 	registryPath := registryConfig.Path
 	r := &Registry{
-		cachedRegistryProtoTtl: time.Duration(registryConfig.CacheTtlSeconds),
+		project:                project,
+		cachedRegistryProtoTtl: time.Duration(registryConfig.CacheTtlSeconds) * time.Second,


This seems like a pretty large change in time proportional to the number of seconds, no? Was this on purpose?

franciscojavierarceo · 2024-11-26T15:03:03Z

go/internal/feast/transformation/transformation.go

@@ -91,103 +99,6 @@ func ReleaseArrowContext(requestContextArrow map[string]arrow.Array) {
 	}
 }

-func CallTransformations(


shuchu requested review from tokoko and franciscojavierarceo October 22, 2024 16:47

shuchu changed the title ~~feat: Update the go feature server from Expedia code repo.~~ [WIP] feat: Update the go feature server from Expedia code repo. Oct 22, 2024

shuchu force-pushed the feature/migrate-expedia-go-fs branch from 80285e6 to d371247 Compare October 23, 2024 02:43

shuchu force-pushed the feature/migrate-expedia-go-fs branch from 9ee4605 to c5aaf98 Compare October 31, 2024 15:12

shuchu changed the title ~~[WIP] feat: Update the go feature server from Expedia code repo.~~ feat: Update the go feature server from Expedia code repo. Nov 1, 2024

shuchu marked this pull request as ready for review November 1, 2024 03:55

shuchu requested a review from a team as a code owner November 1, 2024 03:55

shuchu requested a review from HaoXuAI November 1, 2024 04:08

EXPEbdodla reviewed Nov 1, 2024

View reviewed changes

shuchu commented Nov 8, 2024

View reviewed changes

shuchu marked this pull request as draft November 11, 2024 03:06

shuchu added 8 commits November 20, 2024 23:15

feat: Update the go feature server from Expedia code repo.

df2f0a6

Signed-off-by: Shuchu Han <[email protected]>

fix: Add go package definition to RegistryServer and Grpcserver.

be909c3

Signed-off-by: Shuchu Han <[email protected]>

fix: Fix the make build-go

23a8156

Signed-off-by: Shuchu Han <[email protected]>

fix: Fix makefile to make test-go work.

37b7266

Signed-off-by: Shuchu Han <[email protected]>

fix: Removed and commented out DataDog related observability instrume…

20aefa9

…ntation code. Signed-off-by: Shuchu Han <[email protected]>

doc: Update the README to mention the contirbution from Expedia Group.

b9eff42

Signed-off-by: Shuchu Han <[email protected]>

fix: Remove the HTTP based Registry.

4eaa6c7

Signed-off-by: Shuchu Han <[email protected]>

fix: Use a general string to represent the transformation service end…

953d62e

…point. Signed-off-by: Shuchu Han <[email protected]>

shuchu force-pushed the feature/migrate-expedia-go-fs branch from b65a6a4 to 953d62e Compare November 21, 2024 04:31

shuchu marked this pull request as ready for review November 21, 2024 04:43

HaoXuAI reviewed Nov 21, 2024

View reviewed changes

shuchu mentioned this pull request Nov 25, 2024

feat: Added feast Go operator db stores support #4771

Merged

fix: Set the transformation service endpoint defintion to feature_sto…

63dcefa

…re.yaml file instead of OS env. Signed-off-by: Shuchu Han <[email protected]>

shuchu requested review from EXPEbdodla and a team and removed request for EXPEbdodla November 26, 2024 01:36

franciscojavierarceo added the ok-to-test label Nov 26, 2024

franciscojavierarceo reviewed Nov 26, 2024

View reviewed changes

Makefile

format-go:

gofmt -s -w go/

lint-go: compile-protos-go

Copy link

Member

franciscojavierarceo Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

franciscojavierarceo reviewed Nov 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Update the go feature server from Expedia code repo. #4665

feat: Update the go feature server from Expedia code repo. #4665

shuchu commented Oct 22, 2024 •

edited

Loading

shuchu commented Oct 22, 2024

shuchu commented Oct 23, 2024

shuchu commented Nov 1, 2024

EXPEbdodla Nov 1, 2024

shuchu Nov 2, 2024 •

edited

Loading

shuchu Nov 5, 2024 •

edited

Loading

shuchu commented Nov 5, 2024

shuchu Nov 8, 2024

EXPEbdodla Nov 8, 2024

shuchu Nov 11, 2024

shuchu Nov 21, 2024

EXPEbdodla Nov 22, 2024

shuchu Nov 25, 2024

shuchu commented Nov 21, 2024

HaoXuAI Nov 21, 2024

EXPEbdodla Nov 22, 2024

shuchu Nov 23, 2024

HaoXuAI Nov 21, 2024

HaoXuAI Nov 21, 2024

EXPEbdodla Nov 22, 2024

shuchu commented Nov 25, 2024

franciscojavierarceo Nov 26, 2024

franciscojavierarceo Nov 26, 2024

franciscojavierarceo Nov 26, 2024

franciscojavierarceo Nov 26, 2024

franciscojavierarceo Nov 26, 2024

franciscojavierarceo Nov 26, 2024

franciscojavierarceo Nov 26, 2024

franciscojavierarceo Nov 26, 2024

franciscojavierarceo Nov 26, 2024

		@@ -1,109 +1,12 @@
		This directory contains the Go logic that's executed by the `EmbeddedOnlineFeatureServer` from Python.

	func (r *RedisOnlineStore) buildHsetKeys(featureViewNames []string, featureNames []string, indicesFeatureView map[int]string, index int) ([]string, []string) {
	func (r *RedisOnlineStore) buildRedisHashSetKeys(featureViewNames []string, featureNames []string, indicesFeatureView map[int]string, index int) ([]string, []string) {

feat: Update the go feature server from Expedia code repo. #4665

Are you sure you want to change the base?

feat: Update the go feature server from Expedia code repo. #4665

Conversation

shuchu commented Oct 22, 2024 • edited Loading

What this PR does / why we need it:

Which issue(s) this PR fixes:

shuchu commented Oct 22, 2024

shuchu commented Oct 23, 2024

shuchu commented Nov 1, 2024

Choose a reason for hiding this comment

shuchu Nov 2, 2024 • edited Loading

Choose a reason for hiding this comment

shuchu Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

shuchu commented Nov 5, 2024

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

shuchu commented Nov 21, 2024

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

shuchu commented Nov 25, 2024

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

shuchu commented Oct 22, 2024 •

edited

Loading

shuchu Nov 2, 2024 •

edited

Loading

shuchu Nov 5, 2024 •

edited

Loading