fix: Enforce kw args in datasources #2567

kevjumba · 2022-04-18T22:28:38Z

What this PR does / why we need it:
Enforces keyword args and add deprecations so that we can more easily manage apis in future.
Which issue(s) this PR fixes:

Fixes #

codecov-commenter · 2022-04-18T22:51:07Z

Codecov Report

Merging #2567 (e023177) into master (ebb7dfe) will increase coverage by 0.10%.
The diff coverage is 92.18%.

@@            Coverage Diff             @@
##           master    #2567      +/-   ##
==========================================
+ Coverage   83.49%   83.60%   +0.10%     
==========================================
  Files         147      147              
  Lines       12199    12314     +115     
==========================================
+ Hits        10186    10295     +109     
- Misses       2013     2019       +6

Flag	Coverage Δ
integrationtests	`72.75% <20.31%> (-1.01%)`	⬇️
unittests	`60.36% <92.18%> (+0.36%)`	⬆️

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

Impacted Files	Coverage Δ
...thon/feast/infra/offline_stores/bigquery_source.py	`86.60% <ø> (ø)`
...stores/contrib/spark_offline_store/spark_source.py	`60.58% <ø> (ø)`
...stores/contrib/trino_offline_store/trino_source.py	`56.17% <ø> (ø)`
...thon/feast/infra/offline_stores/redshift_source.py	`87.40% <ø> (ø)`
...hon/feast/infra/offline_stores/snowflake_source.py	`88.05% <ø> (ø)`
...k/python/feast/infra/offline_stores/file_source.py	`85.98% <40.00%> (-4.84%)`	⬇️
sdk/python/feast/data_source.py	`78.26% <95.06%> (+7.19%)`	⬆️
sdk/python/feast/feature_view.py	`87.03% <100.00%> (ø)`
sdk/python/tests/unit/test_data_sources.py	`100.00% <100.00%> (ø)`
sdk/python/tests/utils/online_read_write_test.py	`93.54% <0.00%> (-6.46%)`	⬇️
... and 5 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 ebb7dfe...e023177. Read the comment docs.

adchia · 2022-04-19T16:21:00Z

sdk/python/feast/data_source.py

+        if args:
+            warnings.warn(
+                (
+                    "kafka parameters should be specified as a keyword argument instead of a positional arg."


nit: upper case k in Kafka?

same elsewhere

adchia · 2022-04-19T16:21:58Z

sdk/python/feast/data_source.py

+                _topic = args[4]
+
+        if _message_format is None:
+            raise ValueError("message format must be specified for kafka source")


Suggested change

raise ValueError("message format must be specified for kafka source")

raise ValueError("Message format must be specified for Kafka source")

adchia · 2022-04-19T16:22:23Z

sdk/python/feast/data_source.py

+        if args:
+            warnings.warn(
+                (
+                    "requestsource parameters should be specified as a keyword argument instead of a positional arg."


similar comment. RequestSource instead of requestsource

adchia · 2022-04-19T16:23:30Z

sdk/python/feast/infra/offline_stores/file_source.py

@@ -20,6 +20,7 @@
 class FileSource(DataSource):
    def __init__(
        self,
+        *,


do we not need warnings in the other sources too?

Other sources already have optional args(if they don't have positional args I didn't touch them.

sdk/python/feast/infra/offline_stores/contrib/trino_offline_store/trino_source.py

achals

I think we need to add tests

sdk/python/feast/data_source.py

achals · 2022-04-19T19:02:53Z

sdk/python/feast/data_source.py

+        if args:
+            warnings.warn(
+                (
+                    "Requestsource parameters should be specified as a keyword argument instead of a positional arg."


nit, Request source

achals · 2022-04-19T19:04:14Z

sdk/python/feast/data_source.py

+
+        super().__init__(name=_name, description=description, tags=tags, owner=owner)
+        if not _batch_source:
+            raise ValueError(f"Batch_source is needed for push source {self.name}")


nit, batch_source

sdk/python/feast/feature_view.py

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

adchia

/lgtm

feast-ci-bot · 2022-04-19T21:00:48Z

[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

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

* Update Signed-off-by: Kevin Zhang <[email protected]> * Fix Signed-off-by: Kevin Zhang <[email protected]> * Update to keyword args Signed-off-by: Kevin Zhang <[email protected]> * Fix lint Signed-off-by: Kevin Zhang <[email protected]> * Fix Signed-off-by: Kevin Zhang <[email protected]> * Fix Signed-off-by: Kevin Zhang <[email protected]> * Change kinesis to optional Signed-off-by: Kevin Zhang <[email protected]> * Fix review issues Signed-off-by: Kevin Zhang <[email protected]> * Fix lint Signed-off-by: Kevin Zhang <[email protected]> * Add unit tests Signed-off-by: Kevin Zhang <[email protected]> * Fix Signed-off-by: Kevin Zhang <[email protected]> * Fix Signed-off-by: Kevin Zhang <[email protected]> * Fix imports Signed-off-by: Kevin Zhang <[email protected]> * Fix lint Signed-off-by: Kevin Zhang <[email protected]> * Fix Signed-off-by: Kevin Zhang <[email protected]>

## [0.20.1](v0.20.0...v0.20.1) (2022-04-20) ### Bug Fixes * Addresses ZeroDivisionError when materializing file source with same timestamps ([#2551](#2551)) ([5539c51](5539c51)) * Build platform specific python packages with ci-build-wheel ([#2555](#2555)) ([1757639](1757639)) * Enforce kw args featureservice ([#2575](#2575)) ([4dce254](4dce254)) * Enforce kw args in datasources ([#2567](#2567)) ([6374634](6374634)) * Fix `__hash__` methods ([#2556](#2556)) ([dd8b854](dd8b854)) * Fix DynamoDB fetches when there are entities that are not found ([#2573](#2573)) ([882328f](882328f)) * Fix push sources and add docs / tests pushing via the python feature server ([#2561](#2561)) ([c5006c2](c5006c2)) * Fixed data mapping errors for Snowflake ([#2558](#2558)) ([abd6be7](abd6be7)) * Small typo in CLI ([#2578](#2578)) ([8717bc8](8717bc8)) * Switch from `join_key` to `join_keys` in tests and docs ([#2580](#2580)) ([6130b80](6130b80)) * Update build_go_protos to use a consistent python path ([#2550](#2550)) ([1c523bf](1c523bf)) * Update RedisCluster to use redis-py official implementation ([#2554](#2554)) ([c47fa2a](c47fa2a)) * Use cwd when getting module path ([#2577](#2577)) ([28752f2](28752f2))

# [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)

kevjumba requested a review from achals April 18, 2022 22:28

kevjumba assigned achals Apr 18, 2022

feast-ci-bot added the approved label Apr 18, 2022

kevjumba added the ok-to-test label Apr 18, 2022

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

kevjumba added area/infra ok-to-test and removed size/L ok-to-test labels Apr 18, 2022

adchia reviewed Apr 19, 2022

View reviewed changes

kevjumba force-pushed the enforce_kw_args branch from e510fc6 to 0753d98 Compare April 19, 2022 16:57

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

kevjumba assigned achals and adchia and unassigned achals Apr 19, 2022

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

kevjumba requested a review from adchia April 19, 2022 18:57

achals reviewed Apr 19, 2022

View reviewed changes

kevjumba assigned kevjumba and unassigned achals and adchia Apr 19, 2022

kevjumba added 7 commits April 19, 2022 13:19

Update

6c98880

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

Fix

092a1f0

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

Update to keyword args

44af590

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

Fix lint

50bd918

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

Fix

8ddc48c

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

Fix

d388599

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

Change kinesis to optional

06cd1b6

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

kevjumba added 8 commits April 19, 2022 13:19

Fix review issues

f9c374f

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

Fix lint

67eca66

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

Add unit tests

e292214

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

Fix

ad3ba3f

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

Fix

a6007ef

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

Fix imports

2a6797a

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

Fix lint

8c487b4

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

Fix

e023177

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

kevjumba force-pushed the enforce_kw_args branch from 1f7a964 to e023177 Compare April 19, 2022 20:24

adchia approved these changes Apr 19, 2022

View reviewed changes

feast-ci-bot assigned adchia Apr 19, 2022

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

feast-ci-bot merged commit 0b7ec53 into feast-dev:master Apr 19, 2022

kyungwan-nam mentioned this pull request Oct 11, 2022

Support Feast UI bennfocus/feast-hive#22

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Enforce kw args in datasources #2567

fix: Enforce kw args in datasources #2567

kevjumba commented Apr 18, 2022

codecov-commenter commented Apr 18, 2022 •

edited

Loading

adchia Apr 19, 2022

adchia Apr 19, 2022

kevjumba Apr 19, 2022

adchia Apr 19, 2022

kevjumba Apr 19, 2022

adchia Apr 19, 2022

kevjumba Apr 19, 2022

adchia Apr 19, 2022

kevjumba Apr 19, 2022

achals left a comment

achals Apr 19, 2022

kevjumba Apr 19, 2022

achals Apr 19, 2022

kevjumba Apr 19, 2022

adchia left a comment

feast-ci-bot commented Apr 19, 2022

	raise ValueError("message format must be specified for kafka source")
	raise ValueError("Message format must be specified for Kafka source")

fix: Enforce kw args in datasources #2567

fix: Enforce kw args in datasources #2567

Conversation

kevjumba commented Apr 18, 2022

codecov-commenter commented Apr 18, 2022 • 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

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

achals 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

adchia left a comment

Choose a reason for hiding this comment

feast-ci-bot commented Apr 19, 2022

codecov-commenter commented Apr 18, 2022 •

edited

Loading