feat: Contrib azure provider with synapse/mssql offline store and Azure registry store by kevjumba · Pull Request #3072 · feast-dev/feast

kevjumba · 2022-08-11T20:30:16Z

What this PR does / why we need it:
Moving https://github.com/Azure/feast-azure feast_azure_provider package into feast as contrib. Does not move the kubernetes cluster configuration.

Which issue(s) this PR fixes:

Fixes #

codecov-commenter · 2022-08-11T20:40:49Z

Codecov Report

Merging #3072 (516ff76) into master (29f2895) will increase coverage by 8.16%.
The diff coverage is 44.50%.

@@            Coverage Diff             @@
##           master    #3072      +/-   ##
==========================================
+ Coverage   67.86%   76.03%   +8.16%     
==========================================
  Files         176      214      +38     
  Lines       15507    17897    +2390     
==========================================
+ Hits        10524    13608    +3084     
+ Misses       4983     4289     -694

Flag	Coverage Δ
integrationtests	`66.96% <42.94%> (-0.91%)`	⬇️
unittests	`57.87% <40.31%> (?)`

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

Impacted Files	Coverage Δ
sdk/python/feast/data_source.py	`81.68% <ø> (+23.44%)`	⬆️
sdk/python/feast/feature_store.py	`85.60% <ø> (+3.66%)`	⬆️
sdk/python/feast/infra/provider.py	`77.94% <ø> (ø)`
sdk/python/feast/infra/registry/registry.py	`80.83% <ø> (+5.20%)`	⬆️
...sts/integration/registration/test_universal_cli.py	`100.00% <ø> (ø)`
setup.py	`0.00% <0.00%> (ø)`
sdk/python/feast/type_map.py	`57.62% <10.52%> (-2.75%)`	⬇️
sdk/python/feast/repo_config.py	`87.54% <25.00%> (+4.40%)`	⬆️
sdk/python/feast/inference.py	`85.39% <33.33%> (-0.66%)`	⬇️
...ffline_stores/contrib/mssql_offline_store/mssql.py	`38.42% <38.42%> (ø)`
... and 119 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

adchia

not needed, but we could theoretically setup CI by using https://docs.microsoft.com/en-us/azure/azure-sql-edge/disconnected-deployment and using test-containers.

adchia · 2022-08-12T20:36:37Z

docs/reference/offline-stores/mssql.md

+## Functionality Matrix
+
+The set of functionality supported by offline stores is described in detail [here](overview.md#functionality).
+Below is a matrix indicating which functionality is supported by the Spark offline store.


adchia · 2022-08-12T20:54:09Z

docs/roadmap.md

  * [x] [BigQuery source](https://docs.feast.dev/reference/data-sources/bigquery)
  * [x] [Parquet file source](https://docs.feast.dev/reference/data-sources/file)
-  * [x] [Synapse source (community plugin)](https://github.com/Azure/feast-azure)
+  * [x] [Synapse source (contrib plugin)](https://docs.feast.dev/reference/data-sources/mssql)


should this be Azure Synapse + Azure SQL?

adchia · 2022-08-12T21:32:35Z

sdk/python/feast/infra/contrib/azure_provider.py

+    make_tzaware,
+)
+
+# Make sure spark warning doesn't raise more than once.


as discussed, we can kill this provider probably or use passthroughprovider

Lejboelle · 2022-08-15T08:55:17Z

sdk/python/feast/data_source.py


        if data_source_type == DataSourceProto.SourceType.CUSTOM_SOURCE:
+            data_source.data_source_class_type = "feast.infra.offline_stores.contrib.mssql_offline_store.mssqlserver_source.MsSqlServerSource"
            cls = get_data_source_class_from_type(data_source.data_source_class_type)


Instead of hard coding the class type here, I believe it should be declared in the to_proto() function of the MsSqlServerSource class. This is done in case of the other offline contrib stores and also ensures that there are no conflicts with any other future custom sources.

adchia · 2022-08-17T20:17:53Z

sdk/python/feast/infra/offline_stores/contrib/mssql_offline_store/tests/data_source.py

+        super().__init__(project_name)
+        self.tables_created: List[str] = []
+        self.container = SqlServerContainer(user=MSSQL_USER, password=MSSQL_PASSWORD)
+        # self.container = fixture_request.getfixturevalue("mssql_container")


Remove all the commented line here?

sdk/python/feast/infra/offline_stores/contrib/mssql_offline_store/tests/data_source.py

adchia · 2022-08-17T20:21:46Z

sdk/python/feast/type_map.py

+    pa_type_as_str = str(pa_type).lower()
+    if pa_type_as_str.startswith("timestamp"):
+        if "tz=" in pa_type_as_str:
+            return "DATETIME2"


any reason why this is upper case and not lower case like everything else?

adchia · 2022-08-17T22:21:07Z

docs/reference/data-sources/mssql.md

doesn't look fixed to me? (Microsoft sql table sources)

oh sorry, I realized I hadn't pushed my changes last night.

sdk/python/feast/infra/contrib/azure_provider.py

sdk/python/feast/infra/offline_stores/contrib/mssql_offline_store/tests/data_source.py

sdk/python/feast/infra/offline_stores/contrib/mssql_offline_store/mssqlserver_source.py

adchia · 2022-08-17T22:40:55Z

docs/reference/offline-stores/mssql.md

call out synapse or sql server?

adchia · 2022-08-17T22:41:36Z

docs/tutorials/azure/README.md

these numbers seem wrong

sdk/python/feast/infra/offline_stores/contrib/mssql_offline_store/mssql.py

sdk/python/feast/infra/offline_stores/contrib/mssql_offline_store/tests/data_source.py

adchia · 2022-08-18T15:44:25Z

sdk/python/feast/infra/offline_stores/contrib/mssql_offline_store/tests/data_source.py

if you always use the fixture as I suggest above, then this shouldn't be needed

sdk/python/feast/infra/offline_stores/contrib/mssql_offline_store/tests/data_source.py

adchia · 2022-08-18T15:46:39Z

sdk/python/feast/infra/offline_stores/contrib/mssql_offline_store/mssql.py

can you throw an exception here? e.g. some not implemented error so users know they can't pass a SQL string for MsSQL to generate the entity dataframe?

adchia · 2022-08-18T15:48:10Z

docs/reference/data-sources/mssql.md

doesn't look fixed to me? (Microsoft sql table sources)

adchia · 2022-08-18T15:48:36Z

docs/tutorials/azure/README.md

+## 6. Running Feast Azure Tutorials locally without Azure workspace
+
+* If you are on a free tier instance, you will not be able to deploy the azure deployment because the azure workspace requires VCPUs and the free trial subscription does not have a quota.
+* The workaround is to remove the `Microsoft.MachineLearningServices/workspaces/computes` resource from `fs_snapse_azure_deploy.json` and setting up the environment locally.


typo? assuming should be synapse

thanks for the catch! fixed

adchia · 2022-08-18T18:16:42Z

sdk/python/feast/infra/offline_stores/contrib/mssql_offline_store/tests/data_source.py

pass in the azure-sql-edge image as mentioned above?

Oh I didn't realize you actually wanted to use this image. Updated.

sdk/python/tests/integration/registration/test_universal_cli.py

adchia · 2022-08-18T18:17:46Z

sdk/python/tests/utils/e2e_test_validation.py

you shouldn't need to pass a fixture_request actually. the test environment already has a datasourcecreator

Signed-off-by: Kevin Zhang <[email protected]>

Signed-off-by: Danny Chiao <[email protected]>

adchia

/lgtm

feast-ci-bot · 2022-08-19T19:53:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adchia, kevjumba

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [adchia,kevjumba]

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

Signed-off-by: Danny Chiao <[email protected]>

adchia · 2022-08-19T20:07:46Z

/lgtm

# [0.24.0](v0.23.0...v0.24.0) (2022-08-25) ### Bug Fixes * Check if on_demand_feature_views is an empty list rather than None for snowflake provider ([#3046](#3046)) ([9b05e65](9b05e65)) * FeatureStore.apply applies BatchFeatureView correctly ([#3098](#3098)) ([41be511](41be511)) * Fix Feast Java inconsistency with int64 serialization vs python ([#3031](#3031)) ([4bba787](4bba787)) * Fix feature service inference logic ([#3089](#3089)) ([4310ed7](4310ed7)) * Fix field mapping logic during feature inference ([#3067](#3067)) ([cdfa761](cdfa761)) * Fix incorrect on demand feature view diffing and improve Java tests ([#3074](#3074)) ([0702310](0702310)) * Fix Java helm charts to work with refactored logic. Fix FTS image ([#3105](#3105)) ([2b493e0](2b493e0)) * Fix on demand feature view output in feast plan + Web UI crash ([#3057](#3057)) ([bfae6ac](bfae6ac)) * Fix release workflow to release 0.24.0 ([#3138](#3138)) ([a69aaae](a69aaae)) * Fix Spark offline store type conversion to arrow ([#3071](#3071)) ([b26566d](b26566d)) * Fixing Web UI, which fails for the SQL registry ([#3028](#3028)) ([64603b6](64603b6)) * Force Snowflake Session to Timezone UTC ([#3083](#3083)) ([9f221e6](9f221e6)) * Make infer dummy entity join key idempotent ([#3115](#3115)) ([1f5b1e0](1f5b1e0)) * More explicit error messages ([#2708](#2708)) ([e4d7afd](e4d7afd)) * Parse inline data sources ([#3036](#3036)) ([c7ba370](c7ba370)) * Prevent overwriting existing file during `persist` ([#3088](#3088)) ([69af21f](69af21f)) * Register BatchFeatureView in feature repos correctly ([#3092](#3092)) ([b8e39ea](b8e39ea)) * Return an empty infra object from sql registry when it doesn't exist ([#3022](#3022)) ([8ba87d1](8ba87d1)) * Teardown tables for Snowflake Materialization testing ([#3106](#3106)) ([0a0c974](0a0c974)) * UI error when saved dataset is present in registry. ([#3124](#3124)) ([83cf753](83cf753)) * Update sql.py ([#3096](#3096)) ([2646a86](2646a86)) * Updated snowflake template ([#3130](#3130)) ([f0594e1](f0594e1)) ### Features * Add authentication option for snowflake connector ([#3039](#3039)) ([74c75f1](74c75f1)) * Add Cassandra/AstraDB online store contribution ([#2873](#2873)) ([feb6cb8](feb6cb8)) * Add Snowflake materialization engine ([#2948](#2948)) ([f3b522b](f3b522b)) * Adding saved dataset capabilities for Postgres ([#3070](#3070)) ([d3253c3](d3253c3)) * Allow passing repo config path via flag ([#3077](#3077)) ([0d2d951](0d2d951)) * Contrib azure provider with synapse/mssql offline store and Azure registry store ([#3072](#3072)) ([9f7e557](9f7e557)) * Custom Docker image for Bytewax batch materialization ([#3099](#3099)) ([cdd1b07](cdd1b07)) * Feast AWS Athena offline store (again) ([#3044](#3044)) ([989ce08](989ce08)) * Implement spark offline store `offline_write_batch` method ([#3076](#3076)) ([5b0cc87](5b0cc87)) * Initial Bytewax materialization engine ([#2974](#2974)) ([55c61f9](55c61f9)) * Refactor feature server helm charts to allow passing feature_store.yaml in environment variables ([#3113](#3113)) ([85ee789](85ee789))

bgalvao · 2022-09-08T13:48:45Z

Hi, I am wondering if it is possible to use this for a self-hosted MSSQL Server database as an Offline Store?

feast-ci-bot added approved size/XXL labels Aug 11, 2022

kevjumba changed the title ~~Azure provider~~ feat: Contrib azure provider with synapse/mssql offline store and Azure registry store Aug 11, 2022

kevjumba force-pushed the azure_provider branch 2 times, most recently from dba6b1f to 7fdc2ae Compare August 12, 2022 19:42

kevjumba added the ok-to-test label Aug 12, 2022

adchia reviewed Aug 12, 2022

View reviewed changes

Lejboelle reviewed Aug 15, 2022

View reviewed changes

kevjumba force-pushed the azure_provider branch 2 times, most recently from 2eaebdd to 8d92603 Compare August 17, 2022 18:57

adchia reviewed Aug 17, 2022

View reviewed changes

adchia self-assigned this Aug 17, 2022

adchia reviewed Aug 18, 2022

View reviewed changes

sdk/python/feast/infra/offline_stores/contrib/mssql_offline_store/tests/data_source.py Outdated Show resolved Hide resolved

adchia reviewed Aug 18, 2022

View reviewed changes

kevjumba force-pushed the azure_provider branch 2 times, most recently from 35b123d to 452379f Compare August 18, 2022 17:24

kevjumba requested a review from adchia August 18, 2022 17:26

adchia reviewed Aug 18, 2022

View reviewed changes

kevjumba force-pushed the azure_provider branch 2 times, most recently from de40738 to f998d8b Compare August 19, 2022 02:57

kevjumba added 8 commits August 19, 2022 10:02

Broken state

e1e210d

Signed-off-by: Kevin Zhang <[email protected]>

working state

011d1e0

Signed-off-by: Kevin Zhang <[email protected]>

Fix the lint issues

a6a2fce

Signed-off-by: Kevin Zhang <[email protected]>

Semi working state

57b63bb

Signed-off-by: Kevin Zhang <[email protected]>

Fix

ae7ed8a

Signed-off-by: Kevin Zhang <[email protected]>

Fremove print

421645b

Signed-off-by: Kevin Zhang <[email protected]>

Fix lint

07fece5

Signed-off-by: Kevin Zhang <[email protected]>

Run build-sphinx

4062031

Signed-off-by: Kevin Zhang <[email protected]>

kevjumba added 11 commits August 19, 2022 10:02

Fix

ee30e73

Signed-off-by: Kevin Zhang <[email protected]>

Fix

ab17db9

Signed-off-by: Kevin Zhang <[email protected]>

Revert

be162f5

Signed-off-by: Kevin Zhang <[email protected]>

Fix

f5aa476

Signed-off-by: Kevin Zhang <[email protected]>

Fix

4423dfa

Signed-off-by: Kevin Zhang <[email protected]>

Fix

5806507

Signed-off-by: Kevin Zhang <[email protected]>

Fix lint

7a4d055

Signed-off-by: Kevin Zhang <[email protected]>

Fix

78b74b1

Signed-off-by: Kevin Zhang <[email protected]>

Fix lint

a9e8119

Signed-off-by: Kevin Zhang <[email protected]>

Fix pyarrow

1341e3e

Signed-off-by: Kevin Zhang <[email protected]>

Fix lint

3d42093

Signed-off-by: Kevin Zhang <[email protected]>

kevjumba force-pushed the azure_provider branch from f998d8b to 3d42093 Compare August 19, 2022 17:11

add requirements files

1c591f0

Signed-off-by: Danny Chiao <[email protected]>

adchia approved these changes Aug 19, 2022

View reviewed changes

feast-ci-bot added the lgtm label Aug 19, 2022

fix name of docs

b4da607

Signed-off-by: Danny Chiao <[email protected]>

feast-ci-bot removed the lgtm label Aug 19, 2022

adchia added 4 commits August 19, 2022 15:58

fix offline store readme

c3a0423

Signed-off-by: Danny Chiao <[email protected]>

fix offline store readme

576b57e

Signed-off-by: Danny Chiao <[email protected]>

fix

69940ac

Signed-off-by: Danny Chiao <[email protected]>

fix

516ff76

Signed-off-by: Danny Chiao <[email protected]>

feast-ci-bot added the lgtm label Aug 19, 2022

kevjumba added ok-to-test and removed ok-to-test labels Aug 19, 2022

feast-ci-bot merged commit 9f7e557 into feast-dev:master Aug 19, 2022

amommendes mentioned this pull request Jan 6, 2023

Fetching historical feature values by passing sql as entity_df #3234

Open

Comments

Conversation

kevjumba commented Aug 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Aug 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

adchia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lejboelle Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

kevjumba commented Aug 11, 2022 •

edited

Loading

codecov-commenter commented Aug 11, 2022 •

edited

Loading

Lejboelle Aug 15, 2022 •

edited

Loading