chore: Update docs for offline and online stores #2946

kevjumba · 2022-07-15T19:32:59Z

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

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

codecov-commenter · 2022-07-15T19:42:00Z

Codecov Report

Merging #2946 (5bc74db) into master (aa2a86a) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2946      +/-   ##
==========================================
- Coverage   77.65%   77.61%   -0.05%     
==========================================
  Files         186      186              
  Lines       16354    16354              
==========================================
- Hits        12700    12693       -7     
- Misses       3654     3661       +7

Flag	Coverage Δ
integrationtests	`68.58% <ø> (-0.14%)`	⬇️
unittests	`58.51% <ø> (ø)`

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

Impacted Files	Coverage Δ
sdk/python/tests/utils/online_read_write_test.py	`93.54% <0.00%> (-6.46%)`	⬇️
.../integration/online_store/test_online_retrieval.py	`96.84% <0.00%> (-3.16%)`	⬇️
sdk/python/feast/infra/online_stores/datastore.py	`87.96% <0.00%> (-1.39%)`	⬇️
...east/infra/materialization/lambda/lambda_engine.py	`93.75% <0.00%> (+1.04%)`	⬆️

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 aa2a86a...5bc74db. Read the comment docs.

docs/how-to-guides/adding-a-new-offline-store.md

adchia · 2022-07-20T15:26:02Z

docs/how-to-guides/adding-a-new-offline-store.md

might call out that this method is optional since it is only used to work with SavedDatasets

I think you commented on the wrong method , you mean write_logged_features right?

nope. pull_all_from_table_or_query right now in our logic is only exposed for SavedDatasets afaict

adchia · 2022-07-20T15:27:21Z

docs/how-to-guides/adding-a-new-offline-store.md

idk if I would use this as an example. It's a really large PR with also online store + registry store components

Would be better if we created some example PR (similar to what is in this guide)

Hm, I can link the spark pr instead. I think the example guide for the feast custom offline store is fine for high level but I dont' see a clear way of implementing some dummy procedures with. more clarity than the feast custom offline store. Actual implementations to reference would actually then be actual real world pr would help them work out the little kinks in implementation. I agree that the postgres one is a little bit too large but I think the spark one is relevant and useful

Could you convert the dummy example in the guide into an actual PR so they can see?

You can also just point users to the directory that has the offline / online store implementations too

adchia · 2022-07-20T15:28:26Z

docs/how-to-guides/adding-a-new-offline-store.md

Suggested change

Users who want to have their offline store support batch materialization (detailed in this [RFC](https://docs.google.com/document/d/1J7XdwwgQ9dY_uoV9zkRVGQjK9Sy43WISEW6D5V9qzGo/edit#heading=h.9gaqqtox9jg6)) will also need to implement `to_remote_storage` to write distribute the reading and writing of offline store records to a distributed framework. If this functionality is not needed, the RetrievalJob will default to local materialization.

Users who want to have their offline store support scalable batch materialization for online use cases (detailed in this [RFC](https://docs.google.com/document/d/1J7XdwwgQ9dY_uoV9zkRVGQjK9Sy43WISEW6D5V9qzGo/edit#heading=h.9gaqqtox9jg6)) will also need to implement `to_remote_storage` to write distribute the reading and writing of offline store records to a distributed framework. If this is not implemented, Feast will default to local materialization (pulling all records in memory to materialize).

adchia · 2022-07-20T15:30:19Z

docs/how-to-guides/adding-a-new-offline-store.md

Might be more clear if you left this as a comment in the example feature_store.yaml

adchia · 2022-07-20T15:31:39Z

docs/how-to-guides/adding-a-new-offline-store.md

alpha functionality tags is ambiguous imo.

Maybe say our standard is to print messages stating it's alpha status? This is also where a sample simple PR we provide is useful

adchia · 2022-07-20T15:39:25Z

docs/how-to-guides/adding-a-new-offline-store.md

nit: "add documentation" instead of update.

There's also additional work around adding python code docs (i.e. call make build-sphinx)

adchia · 2022-07-20T15:39:46Z

docs/how-to-guides/adding-a-new-offline-store.md

again would not use this as an example because it's too large and hard to parse

addressed above.

docs/how-to-guides/adding-support-for-a-new-online-store.md

adchia · 2022-07-20T15:44:13Z

docs/how-to-guides/adding-support-for-a-new-online-store.md

we might additionally want to cover what the data model is

adchia · 2022-07-20T15:44:22Z

go.sum

unintentional?

fixed after rebase.

kevjumba · 2022-07-20T18:46:29Z

@adchia pTal

achals · 2022-07-20T19:32:49Z

docs/how-to-guides/adding-a-new-offline-store.md

Suggested change

Users who want to have their offline store support scalable batch materialization for online use cases (detailed in this [RFC](https://docs.google.com/document/d/1J7XdwwgQ9dY_uoV9zkRVGQjK9Sy43WISEW6D5V9qzGo/edit#heading=h.9gaqqtox9jg6)) will also need to implement `to_remote_storage` to distribute the reading and writing of offline store records to a distributed framework. If this is not implemented, Feast will default to local materialization (pulling all records into memory to materialize).

Users who want to have their offline store support scalable batch materialization for online use cases (detailed in this [RFC](https://docs.google.com/document/d/1J7XdwwgQ9dY_uoV9zkRVGQjK9Sy43WISEW6D5V9qzGo/edit#heading=h.9gaqqtox9jg6)) will also need to implement `to_remote_storage` to distribute the reading and writing of offline store records to blob storage (such as S3). This may be used by a custom [Materialization Engine](https://github.com/feast-dev/feast/blob/master/sdk/python/feast/infra/materialization/batch_materialization_engine.py#L72) to parallelize the materialization of data by processing it in chunks. If this is not implemented, Feast will default to local materialization (pulling all records into memory to materialize).

achals · 2022-07-20T19:34:25Z

docs/how-to-guides/adding-a-new-offline-store.md

Suggested change

In order to test against the test suite, you need to create a custom `DataSourceCreator`. This class will need to implement our testing infrastructure methods, `create_data_source` and `created_saved_dataset_destination`. `create_data_source` creates a datasource for testing from the dataframe given that will register the dataframe with your offline store and return a datasource pointing to that location. See `BigQueryDataSourceCreator` for an implementation of a datawarehouse data source creator. **Saved datasets** are special datasets used for data validation and is Feast's way of snapshotting your data for future data validation.

In order to test against the test suite, you need to create a custom `DataSourceCreator`. This class will need to implement our testing infrastructure methods, `create_data_source` and `created_saved_dataset_destination`. `create_data_source` should create a datasource forbased on the dataframe passed in. It may be implemented by uploading the contents of the dataframe into the offline store and returning a datasource object pointing to that location. See `BigQueryDataSourceCreator` for an implementation of a data source creator. **Saved datasets** are special datasets used for data validation and is Feast's way of snapshotting your data for future data validation.

achals · 2022-07-20T19:36:14Z

docs/how-to-guides/adding-a-new-offline-store.md

Suggested change

This should run the unit tests and the unit tests should all pass. Please add unit tests for your data source that test out basic functionality of reading and writing to and from the datasource. This should just be class level functionality that ensures that the methods you implemented for the OfflineStore and the DataSource associated with it work as expected. In order to be approved to merge into Feast, these unit tests should all pass and demonstrate that the DataSource works as intended.

This command runs the python unit tests. It's required that unit tests should all pass for contributed components.

Contributors should add unit tests for contributed data source that test out basic functionality of reading and writing to and from the datasource. This should just be class level functionality that ensures that the methods you implemented for the OfflineStore and the DataSource associated with it work as expected. In order to be approved to merge into Feast, these unit tests should all pass and demonstrate that the DataSource works as intended.

test out basic functionality of reading and writing to and from the datasource

This actually is not clear at all. Do you mean the test should write from say bigquery? How is that a unit test then?

Hm, I guess most datasources will not have unit tests then. I guess as long as the offline source passes the integration tests, we can trust the store.

achals · 2022-07-20T19:39:54Z

docs/how-to-guides/adding-a-new-offline-store.md

This is not necessary right? i.e. the docs are generated automatically when changes are merged, we just need to make sure they are referenced sdk/python/docs/index.rst.

Ok let me update.

achals · 2022-07-20T19:40:32Z

docs/how-to-guides/adding-support-for-a-new-online-store.md

Same change and questions as the offline store.

Removed this section entirely. Will basically point to universal tests only to validate the datasource

adchia · 2022-07-20T20:28:04Z

docs/how-to-guides/adding-support-for-a-new-online-store.md

would follow the pattern of the other code snippets, which also include a "file title" that shows where this lives

See the example of using:
{% code title="feast_custom_offline_store/file.py" %}
... your code
{% endcode %}

that we did before

adchia

took a pass through the offline store only, but I imagine most of the comments also translate to the online store version

adchia · 2022-07-21T01:01:43Z

docs/how-to-guides/adding-support-for-a-new-online-store.md

you didn't add the make command here?

Achal mentioned that that is no longer needed and to just update the rst file instead.

adchia · 2022-07-21T01:03:02Z

docs/how-to-guides/adding-a-new-offline-store.md

weird that you only have "write_logged_features" as optional when really pretty much everything is optional except for get_historical_features

Would have the "optional" methods towards the end.

I just treated "optional" as any methods that are labeled abstract even though they have a "pass" I still think the decorator necessitates implementation. I added optional to offline_write batch though.

adchia · 2022-07-21T01:11:28Z

docs/how-to-guides/adding-a-new-offline-store.md

This is generally pretty hard to read imo. Can you make this simpler and more scannable?

e.g.

Contrib offline stores

New offline stores go in sdk/python/feast/infra/offline_stores/contrib/.

What is a contrib plugin?

Not guaranteed to implement all interface methods

Not guaranteed to be stable

Should have warnings for users to indicate this is contrib

How do I make a contrib plugin an "official" plugin?
To move an offline store plugin out of contrib, you need:

GitHub actions (i.e. make test-python-integration) is setup to run all tests against the offline store and pass.

At least two contributors own the plugin (ideally tracked in an OWNERS / CODEOWNERS)

adchia · 2022-07-21T01:12:05Z

docs/how-to-guides/adding-a-new-offline-store.md

this seems to contradict the text you have below

docs/how-to-guides/adding-a-new-offline-store.md

adchia · 2022-07-21T01:20:49Z

docs/how-to-guides/adding-a-new-offline-store.md

why is offline in brackets? shouldn't it be <offline_store> ?

adchia · 2022-07-21T01:21:18Z

docs/how-to-guides/adding-a-new-offline-store.md

Remove Finally since this isn't the last step

adchia · 2022-07-21T01:21:54Z

docs/how-to-guides/adding-a-new-offline-store.md

Also, would remove the "if your offline store requires special packages" part. Every offline store will need to pull in some deps to support a new offline store

adchia · 2022-07-21T01:22:21Z

docs/how-to-guides/adding-a-new-offline-store.md

nit: Convert this to individual steps they can follow instead of many sentences

docs/how-to-guides/adding-a-new-offline-store.md

adchia

again, comments mostly translate to the online store guide too

adchia · 2022-07-22T17:05:10Z

docs/how-to-guides/adding-a-new-offline-store.md

Suggested change

#### How do I make a contrib plugin and "official" plugin?

#### How do I make a contrib plugin an "official" plugin?

adchia · 2022-07-22T17:05:31Z

docs/how-to-guides/adding-a-new-offline-store.md

Suggested change

- Github actions (i.e `make test-python-integration`) is setup to run all tests against the offline store and pass.

- GitHub actions (i.e `make test-python-integration`) is setup to run all tests against the offline store and pass.

adchia · 2022-07-22T17:05:38Z

docs/how-to-guides/adding-a-new-offline-store.md

Suggested change

- At least two contributors own the plugin (ideally tracked in our OWNERS / CODEOWNERS file).

- At least two contributors own the plugin (ideally tracked in our `OWNERS` / `CODEOWNERS` file).

adchia · 2022-07-22T17:17:59Z

docs/how-to-guides/adding-support-for-a-new-online-store.md

This is kind of confusing to me. I don't think users should try to "run the integration tests" since that is for official plugins.

It's useful context to understand the second bullet though. Would rephrase that Feast parametrizes integration tests using the FULL_REPO_CONFIGS variable, which you overwrite to enable testing for the contrib offline store

adchia · 2022-07-22T17:22:30Z

sdk/python/tests/unit/infra/test_key_encoding_utils.py

rebase against master?

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

adchia

/lgtm

feast-ci-bot · 2022-07-22T21:41:43Z

[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

feast-ci-bot added approved size/L labels Jul 15, 2022

kevjumba added the ok-to-test label Jul 15, 2022

achals reviewed Jul 19, 2022

View reviewed changes

docs/how-to-guides/adding-a-new-offline-store.md Outdated Show resolved Hide resolved

kevjumba force-pushed the update_offline_store_docs branch from a194832 to 36fb89c Compare July 19, 2022 23:01

adchia reviewed Jul 20, 2022

View reviewed changes

adchia self-assigned this Jul 20, 2022

kevjumba force-pushed the update_offline_store_docs branch from 36fb89c to eb028f6 Compare July 20, 2022 17:46

achals reviewed Jul 20, 2022

View reviewed changes

adchia reviewed Jul 20, 2022

View reviewed changes

adchia reviewed Jul 21, 2022

View reviewed changes

adchia reviewed Jul 22, 2022

View reviewed changes

kevjumba added 9 commits July 22, 2022 10:27

Update docs

b70c64b

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

address review

7343857

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

Address review

a9f6027

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

Fix

df11468

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

Fux

dc26144

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

Fix

fdfbb75

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

Fix

6adff74

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

Fix

c4b2519

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

Fix

977221f

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

kevjumba force-pushed the update_offline_store_docs branch from fa0e55d to 977221f Compare July 22, 2022 17:42

kevjumba added 6 commits July 22, 2022 10:46

Fix

846f5fe

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

Fix

6a85ce2

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

Fix

d000842

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

Fix

2694894

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

Fix

98134bf

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

Fix

b61f034

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

kevjumba added 7 commits July 22, 2022 12:58

Fix

69405df

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

Fix

6d9f507

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

Fix

76eab5c

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

Fix

aad3dba

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

Fix

3945684

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

Fix

5484ba6

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

Fix

1cc81e2

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

kevjumba removed the ok-to-test label Jul 22, 2022

kevjumba added 10 commits July 22, 2022 13:31

Fix

e5ecd48

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

Fix

e20e6c9

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

Fix

76353a3

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

Fix

b071dc7

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

Fix

6aadfca

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

Fix

392595a

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

Fix

ea7c982

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

Fix

a34cdba

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

Fix

efa51b1

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

Fix

5bc74db

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

adchia approved these changes Jul 22, 2022

View reviewed changes

feast-ci-bot added the lgtm label Jul 22, 2022

adchia enabled auto-merge (squash) July 22, 2022 21:41

feast-ci-bot merged commit 1479519 into feast-dev:master Jul 22, 2022

-This should run the unit tests and the unit tests should all pass. Please add unit tests for your data source that test out basic functionality of reading and writing to and from the datasource. This should just be class level functionality that ensures that the methods you implemented for the OfflineStore and the DataSource associated with it work as expected. In order to be approved to merge into Feast, these unit tests should all pass and demonstrate that the DataSource works as intended.
+This command runs the python unit tests. It's required that unit tests should all pass for contributed components.
+Contributors should add unit tests for contributed data source that test out basic functionality of reading and writing to and from the datasource. This should just be class level functionality that ensures that the methods you implemented for the OfflineStore and the DataSource associated with it work as expected. In order to be approved to merge into Feast, these unit tests should all pass and demonstrate that the DataSource works as intended.

	#### How do I make a contrib plugin and "official" plugin?
	#### How do I make a contrib plugin an "official" plugin?

	- Github actions (i.e `make test-python-integration`) is setup to run all tests against the offline store and pass.
	- GitHub actions (i.e `make test-python-integration`) is setup to run all tests against the offline store and pass.

	- At least two contributors own the plugin (ideally tracked in our OWNERS / CODEOWNERS file).
	- At least two contributors own the plugin (ideally tracked in our `OWNERS` / `CODEOWNERS` file).

chore: Update docs for offline and online stores #2946

chore: Update docs for offline and online stores #2946

Uh oh!

Conversation

kevjumba commented Jul 15, 2022

Uh oh!

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

Uh oh!

Codecov Report

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

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!

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!

kevjumba commented Jul 20, 2022

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!

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!

codecov-commenter commented Jul 15, 2022 •

edited

Loading