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 <shuchu.han@gmail.com>

…ntation code. Signed-off-by: Shuchu Han <shuchu.han@gmail.com>

Signed-off-by: Shuchu Han <shuchu.han@gmail.com>

…point. Signed-off-by: Shuchu Han <shuchu.han@gmail.com>

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 <shuchu.han@gmail.com>

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/protoc-gen-go@v1.31.0
+	go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@v1.3.0
+
+install-go-ci-dependencies:


this is all commented out, do we need it?

commented out as I'm not sure if this setting is good/ready for CI test.

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?

This one may need @EXPEbdodla 's help to explain.

I assume the CacheTtlSeconds means TTL in the unit of second. If that is our original design, we should be good.

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(


Signed-off-by: Shuchu Han <shuchu.han@gmail.com>

franciscojavierarceo

🚀 🚀

…#4665) * feat: Update the go feature server from Expedia code repo. Signed-off-by: Shuchu Han <shuchu.han@gmail.com> * fix: Add go package definition to RegistryServer and Grpcserver. Signed-off-by: Shuchu Han <shuchu.han@gmail.com> * fix: Fix the make build-go Signed-off-by: Shuchu Han <shuchu.han@gmail.com> * fix: Fix makefile to make test-go work. Signed-off-by: Shuchu Han <shuchu.han@gmail.com> * fix: Removed and commented out DataDog related observability instrumentation code. Signed-off-by: Shuchu Han <shuchu.han@gmail.com> * doc: Update the README to mention the contirbution from Expedia Group. Signed-off-by: Shuchu Han <shuchu.han@gmail.com> * fix: Remove the HTTP based Registry. Signed-off-by: Shuchu Han <shuchu.han@gmail.com> * fix: Use a general string to represent the transformation service endpoint. Signed-off-by: Shuchu Han <shuchu.han@gmail.com> * fix: Set the transformation service endpoint defintion to feature_store.yaml file instead of OS env. Signed-off-by: Shuchu Han <shuchu.han@gmail.com> * fix: Fix few format issues. Signed-off-by: Shuchu Han <shuchu.han@gmail.com> --------- Signed-off-by: Shuchu Han <shuchu.han@gmail.com>

# [0.42.0](v0.41.0...v0.42.0) (2024-12-05) ### Bug Fixes * Add adapters for sqlite datetime conversion ([#4797](#4797)) ([e198b17](e198b17)) * Added grpcio extras to default feature-server image ([#4737](#4737)) ([e9cd373](e9cd373)) * Changing node version in release ([7089918](7089918)) * Feast create empty online table when FeatureView attribute online=False ([#4666](#4666)) ([237c453](237c453)) * Fix db store types in Operator CRD ([#4798](#4798)) ([f09339e](f09339e)) * Fix the config issue for postgres ([#4776](#4776)) ([a36f7e5](a36f7e5)) * Fixed example materialize-incremental and improved explanation ([#4734](#4734)) ([ca8a7ab](ca8a7ab)) * Fixed SparkSource docstrings so it wouldn't used inhereted class docstrings ([#4722](#4722)) ([32e6aa1](32e6aa1)) * Fixing PGVector integration tests ([#4778](#4778)) ([88a0320](88a0320)) * Incorrect type passed to assert_permissions in materialize endpoints ([#4727](#4727)) ([b72c2da](b72c2da)) * Issue of DataSource subclasses using parent abstract class docstrings ([#4730](#4730)) ([b24acd5](b24acd5)) * Operator envVar positioning & tls.SecretRef.Name ([#4806](#4806)) ([1115d96](1115d96)) * Populates project created_time correctly according to created ti… ([#4686](#4686)) ([a61b93c](a61b93c)) * Reduce feast-server container image size & fix dev image build ([#4781](#4781)) ([ccc9aea](ccc9aea)) * Removed version func from feature_store.py ([#4748](#4748)) ([f902bb9](f902bb9)) * Support registry instantiation for read-only users ([#4719](#4719)) ([ca3d3c8](ca3d3c8)) * Syntax Error in BigQuery While Retrieving Columns that Start wit… ([#4713](#4713)) ([60fbc62](60fbc62)) * Update release version in a pertinent Operator file ([#4708](#4708)) ([764a8a6](764a8a6)) ### Features * Add api contract to fastapi docs ([#4721](#4721)) ([1a165c7](1a165c7)) * Add Couchbase as an online store ([#4637](#4637)) ([824859b](824859b)) * Add Operator support for spec.feastProject & status.applied fields ([#4656](#4656)) ([430ac53](430ac53)) * Add services functionality to Operator ([#4723](#4723)) ([d1d80c0](d1d80c0)) * Add TLS support to the Operator ([#4796](#4796)) ([a617a6c](a617a6c)) * Added feast Go operator db stores support ([#4771](#4771)) ([3302363](3302363)) * Added support for setting env vars in feast services in feast controller ([#4739](#4739)) ([84b24b5](84b24b5)) * Adding docs outlining native Python transformations on singletons ([#4741](#4741)) ([0150278](0150278)) * Adding first feast operator e2e test. ([#4791](#4791)) ([8339f8d](8339f8d)) * Adding github action to run the operator end-to-end tests. ([#4762](#4762)) ([d8ccb00](d8ccb00)) * Adding ssl support for registry server. ([#4718](#4718)) ([ccf7a55](ccf7a55)) * Adding SSL support for the React UI server and feast UI command. ([#4736](#4736)) ([4a89252](4a89252)) * Adding support for native Python transformations on a single dictionary ([#4724](#4724)) ([9bbc1c6](9bbc1c6)) * Adding TLS support for offline server. ([#4744](#4744)) ([5d8d03f](5d8d03f)) * Building the feast image ([#4775](#4775)) ([6635dde](6635dde)) * File persistence definition and implementation ([#4742](#4742)) ([3bad4a1](3bad4a1)) * Object store persistence in operator ([#4758](#4758)) ([0ae86da](0ae86da)) * OIDC authorization in Feast Operator ([#4801](#4801)) ([eb111d6](eb111d6)) * Operator will create k8s serviceaccount for each feast service ([#4767](#4767)) ([cde5760](cde5760)) * Printing more verbose logs when we start the offline server ([#4660](#4660)) ([9d8d3d8](9d8d3d8)) * PVC configuration and impl ([#4750](#4750)) ([785a190](785a190)) * Qdrant vectorstore support ([#4689](#4689)) ([86573d2](86573d2)) * RBAC Authorization in Feast Operator ([#4786](#4786)) ([0ef5acc](0ef5acc)) * Support for nested timestamp fields in Spark Offline store ([#4740](#4740)) ([d4d94f8](d4d94f8)) * Update the go feature server from Expedia code repo. ([#4665](#4665)) ([6406625](6406625)) * Updated feast Go operator db stores ([#4809](#4809)) ([2c5a6b5](2c5a6b5)) * Updated sample secret following review ([#4811](#4811)) ([dc9f825](dc9f825))

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 <shuchu.han@gmail.com>

fix: Add go package definition to RegistryServer and Grpcserver.

be909c3

Signed-off-by: Shuchu Han <shuchu.han@gmail.com>

fix: Fix the make build-go

23a8156

Signed-off-by: Shuchu Han <shuchu.han@gmail.com>

fix: Fix makefile to make test-go work.

37b7266

Signed-off-by: Shuchu Han <shuchu.han@gmail.com>

fix: Removed and commented out DataDog related observability instrume…

20aefa9

…ntation code. Signed-off-by: Shuchu Han <shuchu.han@gmail.com>

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

b9eff42

Signed-off-by: Shuchu Han <shuchu.han@gmail.com>

fix: Remove the HTTP based Registry.

4eaa6c7

Signed-off-by: Shuchu Han <shuchu.han@gmail.com>

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

953d62e

…point. Signed-off-by: Shuchu Han <shuchu.han@gmail.com>

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 <shuchu.han@gmail.com>

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

fix: Fix few format issues.

8e4893c

Signed-off-by: Shuchu Han <shuchu.han@gmail.com>

shuchu requested a review from a team November 30, 2024 04:36

franciscojavierarceo approved these changes Dec 4, 2024

View reviewed changes

franciscojavierarceo merged commit 6406625 into feast-dev:master Dec 4, 2024
22 checks passed

		@@ -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

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

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 Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franciscojavierarceo left a 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

shuchu Nov 29, 2024 •

edited

Loading