feat: Write logged features to Offline Store (Go - Python integration) #2621

pyalex · 2022-04-27T22:52:42Z

What this PR does / why we need it:

Following previous PRs (#2574 and #2614) this PR finalizes integration between producing logs in Go feature server and writing them to an offline store via python implementation.
Here I added end-to-end test that starts go gRPC server and check that offline store has received served features.

Which issue(s) this PR fixes:

Fixes #

codecov-commenter · 2022-04-27T23:04:37Z

Codecov Report

Merging #2621 (f2ed4ef) into master (cff0133) will decrease coverage by 22.23%.
The diff coverage is 28.99%.

@@             Coverage Diff             @@
##           master    #2621       +/-   ##
===========================================
- Coverage   81.50%   59.26%   -22.24%     
===========================================
  Files         162      162               
  Lines       13380    13507      +127     
===========================================
- Hits        10905     8005     -2900     
- Misses       2475     5502     +3027

Flag	Coverage Δ
integrationtests	`?`
unittests	`59.26% <28.99%> (-0.25%)`	⬇️

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

Impacted Files	Coverage Δ
sdk/python/feast/infra/offline_stores/snowflake.py	`40.30% <0.00%> (-51.45%)`	⬇️
sdk/python/feast/infra/passthrough_provider.py	`77.77% <ø> (-22.23%)`	⬇️
sdk/python/feast/infra/provider.py	`83.84% <ø> (-4.62%)`	⬇️
sdk/python/feast/infra/utils/aws_utils.py	`29.48% <10.00%> (-55.83%)`	⬇️
sdk/python/feast/infra/offline_stores/bigquery.py	`31.53% <16.66%> (-56.55%)`	⬇️
sdk/python/feast/infra/utils/snowflake_utils.py	`24.13% <18.18%> (-58.68%)`	⬇️
...ython/feast/embedded_go/online_features_service.py	`25.86% <25.00%> (-71.23%)`	⬇️
sdk/python/feast/infra/offline_stores/redshift.py	`40.00% <25.00%> (-52.86%)`	⬇️
sdk/python/feast/infra/offline_stores/file.py	`82.54% <33.33%> (-12.67%)`	⬇️
...on/tests/integration/e2e/test_go_feature_server.py	`42.24% <35.71%> (-57.76%)`	⬇️
... and 83 more

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 cff0133...f2ed4ef. Read the comment docs.

Signed-off-by: pyalex <[email protected]>

achals · 2022-05-03T17:23:21Z

go/embedded/online_features.go

 func (s *OnlineFeatureService) StartGprcServer(host string, port int) error {
-	// TODO(oleksii): enable logging
-	// Disable logging for now
+	return s.StartGprcServerWithLogging(host, port, nil, LoggingOptions{})
+}
+
+func (s *OnlineFeatureService) StartGprcServerWithLoggingDefaultOpts(host string, port int, writeLoggedFeaturesCallback logging.OfflineStoreWriteCallback) error {
+	defaultOpts := LoggingOptions{
+		ChannelCapacity: logging.DefaultOptions.ChannelCapacity,
+		EmitTimeout:     logging.DefaultOptions.EmitTimeout,
+		WriteInterval:   logging.DefaultOptions.WriteInterval,
+		FlushInterval:   logging.DefaultOptions.FlushInterval,
+	}
+	return s.StartGprcServerWithLogging(host, port, writeLoggedFeaturesCallback, defaultOpts)
+}


What's the difference between default logging opts and no logging opts? Is logging disable? Can we add comments for the methods?

achals · 2022-05-03T17:23:52Z

go/embedded/online_features.go

 		fmt.Println("Stopping the gRPC server...")
 		grpcServer.GracefulStop()
+		if loggingService != nil {
+			loggingService.Stop()
+		}
+		fmt.Println("gRPC server terminated")


We should really move some of this stuff to use a proper logger.

feast-ci-bot · 2022-05-03T17:30:17Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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

kevjumba · 2022-05-03T17:34:05Z

go/internal/feast/server/logging/offlinestoresink.go

+	"github.com/google/uuid"
+)
+
+type OfflineStoreWriteCallback func(featureServiceName, datasetDir string) string


probably should specify name as string too?

it's a short form

Signed-off-by: Oleksii Moskalenko <[email protected]>

achals · 2022-05-03T17:50:08Z

/lgtm

# [0.21.0](v0.20.0...v0.21.0) (2022-05-13) ### Bug Fixes * Addresses ZeroDivisionError when materializing file source with same timestamps ([#2551](#2551)) ([1e398d9](1e398d9)) * Asynchronously refresh registry for the feast ui command ([#2672](#2672)) ([1b09ca2](1b09ca2)) * Build platform specific python packages with ci-build-wheel ([#2555](#2555)) ([b10a4cf](b10a4cf)) * Delete data sources from registry when using the diffing logic ([#2669](#2669)) ([fc00ca8](fc00ca8)) * Enforce kw args featureservice ([#2575](#2575)) ([160d7b7](160d7b7)) * Enforce kw args in datasources ([#2567](#2567)) ([0b7ec53](0b7ec53)) * Feature logging to Redshift is broken ([#2655](#2655)) ([479cd51](479cd51)) * Feature service to templates ([#2649](#2649)) ([1e02066](1e02066)) * Feature with timestamp type is incorrectly interpreted by Go FS ([#2588](#2588)) ([e3d9588](e3d9588)) * Fix `__hash__` methods ([#2556](#2556)) ([ebb7dfe](ebb7dfe)) * Fix AWS bootstrap template ([#2604](#2604)) ([c94a69c](c94a69c)) * Fix broken proto conversion methods for data sources ([#2603](#2603)) ([00ed65a](00ed65a)) * Fix case where on demand feature view tab is broken if no custom tabs are passed. ([#2682](#2682)) ([01d3568](01d3568)) * Fix DynamoDB fetches when there are entities that are not found ([#2573](#2573)) ([7076fe0](7076fe0)) * Fix Feast UI parser to work with new APIs ([#2668](#2668)) ([8d76751](8d76751)) * Fix java server after odfv update ([#2602](#2602)) ([0ca6297](0ca6297)) * Fix materialization with ttl=0 bug ([#2666](#2666)) ([ab78702](ab78702)) * Fix push sources and add docs / tests pushing via the python feature server ([#2561](#2561)) ([e8e418e](e8e418e)) * Fixed data mapping errors for Snowflake ([#2558](#2558)) ([53c2ce2](53c2ce2)) * Forcing ODFV udfs to be __main__ module and fixing false positive duplicate data source warning ([#2677](#2677)) ([2ce33cd](2ce33cd)) * Include the ui/build directory, and remove package data ([#2681](#2681)) ([0384f5f](0384f5f)) * Infer features for feature services when they depend on feature views without schemas ([#2653](#2653)) ([87c194c](87c194c)) * Pin dependencies to nearest major version ([#2647](#2647)) ([bb72b7c](bb72b7c)) * Pin pip<22.1 to get around breaking change in pip==22.1 ([#2678](#2678)) ([d3e01bc](d3e01bc)) * Punt deprecation warnings and clean up some warnings. ([#2670](#2670)) ([f775d2e](f775d2e)) * Reject undefined features when using `get_historical_features` or `get_online_features` ([#2665](#2665)) ([36849fb](36849fb)) * Remove ci extra from the feature transformation server dockerfile ([#2618](#2618)) ([25613b4](25613b4)) * Remove incorrect call to logging.basicConfig ([#2676](#2676)) ([8cbf51c](8cbf51c)) * Small typo in CLI ([#2578](#2578)) ([f372981](f372981)) * Switch from `join_key` to `join_keys` in tests and docs ([#2580](#2580)) ([d66c931](d66c931)) * Teardown trino container correctly after tests ([#2562](#2562)) ([72f1558](72f1558)) * Update build_go_protos to use a consistent python path ([#2550](#2550)) ([f136f8c](f136f8c)) * Update data source timestamp inference error message to make sense ([#2636](#2636)) ([3eaf6b7](3eaf6b7)) * Update field api to add tag parameter corresponding to labels in Feature. ([#2610](#2610)) ([689d20b](689d20b)) * Update java integration tests and add more logging ([#2637](#2637)) ([10e23b4](10e23b4)) * Update on demand feature view api ([#2587](#2587)) ([38cd7f9](38cd7f9)) * Update RedisCluster to use redis-py official implementation ([#2554](#2554)) ([ce5606f](ce5606f)) * Use cwd when getting module path ([#2577](#2577)) ([b550e59](b550e59)) * Use ParquetDataset for Schema Inference ([#2686](#2686)) ([4f85e3e](4f85e3e)) * Use timestamp type when converting unixtimestamp feature type to arrow ([#2593](#2593)) ([c439611](c439611)) ### Features * Add hbase online store support in feast ([#2590](#2590)) ([c9eda79](c9eda79)) * Adding SSL options for Postgres ([#2644](#2644)) ([0e809c2](0e809c2)) * Allow Feast UI to be spun up with CLI command: feast ui ([#2667](#2667)) ([44ca9f5](44ca9f5)) * Allow to pass secrets and environment variables to transformation service ([#2632](#2632)) ([ffa33ad](ffa33ad)) * CLI command 'feast serve' should start go-based server if flag is enabled ([#2617](#2617)) ([f3ff812](f3ff812)) * Create stream and batch feature view abstractions ([#2559](#2559)) ([d1f76e5](d1f76e5)) * Postgres supported as Registry, Online store, and Offline store ([#2401](#2401)) ([ed2f979](ed2f979)) * Support entity fields in feature view `schema` parameter by dropping them ([#2568](#2568)) ([c8fcc35](c8fcc35)) * Write logged features to an offline store (Python API) ([#2574](#2574)) ([134dc5f](134dc5f)) * Write logged features to Offline Store (Go - Python integration) ([#2621](#2621)) ([ccad832](ccad832)) ### Reverts * Revert "chore: Deprecate value type (#2611)" (#2643) ([4fbdfb1](4fbdfb1)), closes [#2611](#2611) [#2643](#2643)

feast-ci-bot added the size/L label Apr 27, 2022

pyalex added do-not-merge/work-in-progress ok-to-test and removed size/L labels Apr 27, 2022

feast-ci-bot added the size/L label Apr 27, 2022

pyalex added ok-to-test and removed ok-to-test labels Apr 27, 2022

pyalex added 5 commits April 29, 2022 11:31

write logged features by path

fb26fe8

Signed-off-by: pyalex <[email protected]>

fix dummy sink

37f85f4

Signed-off-by: pyalex <[email protected]>

share snowflake cursor and stage name

081eed4

Signed-off-by: pyalex <[email protected]>

share snowflake cursor and stage name

b5ca9ac

Signed-off-by: pyalex <[email protected]>

e2e tests WIP

5f22182

Signed-off-by: pyalex <[email protected]>

pyalex force-pushed the feature-logging-integration branch from 7267e41 to 5f22182 Compare April 29, 2022 23:02

feast-ci-bot added size/XL and removed size/L labels Apr 29, 2022

pyalex added ok-to-test and removed ok-to-test labels May 2, 2022

graceful stop (grpc & logging)

25fc614

Signed-off-by: pyalex <[email protected]>

feast-ci-bot removed the do-not-merge/work-in-progress label May 2, 2022

revert accidental change

f2ed4ef

Signed-off-by: pyalex <[email protected]>

pyalex requested review from tsotnet and achals May 2, 2022 20:47

achals approved these changes May 3, 2022

View reviewed changes

feast-ci-bot added the approved label May 3, 2022

kevjumba reviewed May 3, 2022

View reviewed changes

add comments to go embedded methods

83f51a3

Signed-off-by: Oleksii Moskalenko <[email protected]>

feast-ci-bot assigned achals May 3, 2022

feast-ci-bot added the lgtm label May 3, 2022

feast-ci-bot merged commit ccad832 into feast-dev:master May 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Write logged features to Offline Store (Go - Python integration) #2621

feat: Write logged features to Offline Store (Go - Python integration) #2621

pyalex commented Apr 27, 2022 •

edited

Loading

codecov-commenter commented Apr 27, 2022 •

edited

Loading

achals May 3, 2022

pyalex May 3, 2022

achals May 3, 2022

feast-ci-bot commented May 3, 2022

kevjumba May 3, 2022

pyalex May 3, 2022

achals commented May 3, 2022

feat: Write logged features to Offline Store (Go - Python integration) #2621

feat: Write logged features to Offline Store (Go - Python integration) #2621

Conversation

pyalex commented Apr 27, 2022 • edited Loading

codecov-commenter commented Apr 27, 2022 • edited Loading

Codecov Report

achals May 3, 2022

Choose a reason for hiding this comment

pyalex May 3, 2022

Choose a reason for hiding this comment

achals May 3, 2022

Choose a reason for hiding this comment

feast-ci-bot commented May 3, 2022

kevjumba May 3, 2022

Choose a reason for hiding this comment

pyalex May 3, 2022

Choose a reason for hiding this comment

achals commented May 3, 2022

pyalex commented Apr 27, 2022 •

edited

Loading

codecov-commenter commented Apr 27, 2022 •

edited

Loading