Make online store nullable

[mirayyuce]

What this PR does / why we need it: At "feast init" step if no provider is specified the current version initializes the online store with default type based on the provider. However, some users don't want to specify an online store. When the online store parameter is not set in feature_store.yaml with provider either gcp or aws, it will fail at apply().

Which issue(s) this PR fixes:

Fixes #1809

Does this PR introduce a user-facing change?:

NONE

[feast-ci-bot]

Hi @mirayyuce. Thanks for your PR.

I'm waiting for a feast-dev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[pyalex]

@mirayyuce thanks for your contribution!

However, using Feast without an online store is rather an exception than a rule, simply because this is one of the core features of Feast. I also suspect the proposed change might introduce inconsistency for users who don't specify online store type and instead use inferred value.
That being said, I think the fix should be to enable setting online_store explicitly to null in feature_store.yaml instead of setting sqlite as default always.

[pyalex]

Better solution would be to make online_store nullable.
PR also should be cleaned up: .DS_Store files should be removed, unrelated changes should be removed.

[mirayyuce]

@mirayyuce thanks for your contribution!

However, using Feast without an online store is rather an exception than a rule, simply because this is one of the core features of Feast. I also suspect the proposed change might introduce inconsistency for users who don't specify online store type and instead use inferred value. That being said, I think the fix should be to enable setting online_store explicitly to null in feature_store.yaml instead of setting sqlite as default always.

Thank you for the review @pyalex

In the issue I see two users who don't specify an online store with provider aws having some errors. GCP also causes different errors if we don't set the online store. It searches for Datastore or Firebase in GCP project. These errors were caused by the fact that apply() updates the online store with the latest feature values whether or not there is an online store. If there isn't an online/offline store, repo_config is setting the store type based on the provider, however because of the required parameters for both online and offline store those two users had issues.

I tried giving some dummy hardcoded values to aws online store parameters, but then it tried to connect and had credential errors.

At first I set the online store to null, however this caused problems with the tests. In all tests I see that sqlite is assumed as the default online store if it is not specified. This change will be the quickest change with minimum effect. It will help users having errors when they don't set an online store.

[mavysavydav]

/ok-to-test

[codecov-commenter]

Codecov Report

Merging #2224 (765a91a) into master (2a95629) will decrease coverage by 24.69%.
The diff coverage is 62.00%.

@@             Coverage Diff             @@
##           master    #2224       +/-   ##
===========================================
- Coverage   84.98%   60.28%   -24.70%     
===========================================
  Files         105      106        +1     
  Lines        8450     8609      +159     
===========================================
- Hits         7181     5190     -1991     
- Misses       1269     3419     +2150     

Flag	Coverage Δ
integrationtests	?
unittests	60.28% <62.00%> (+0.43%)	⬆️

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

Impacted Files	Coverage Δ
sdk/python/feast/infra/aws.py	23.55% <0.00%> (-48.78%)	⬇️
.../python/tests/integration/registration/test_cli.py	55.78% <44.00%> (-44.22%)	⬇️
sdk/python/feast/repo_config.py	77.09% <50.00%> (-9.92%)	⬇️
sdk/python/feast/infra/local.py	95.45% <100.00%> (+0.10%)	⬆️
sdk/python/feast/infra/passthrough_provider.py	89.85% <100.00%> (-10.15%)	⬇️
.../tests/integration/scaffolding/test_repo_config.py	100.00% <100.00%> (ø)
.../integration/online_store/test_universal_online.py	15.20% <0.00%> (-82.94%)	⬇️
.../integration/online_store/test_online_retrieval.py	17.39% <0.00%> (-82.61%)	⬇️
...fline_store/test_universal_historical_retrieval.py	19.18% <0.00%> (-80.82%)	⬇️
sdk/python/tests/utils/online_read_write_test.py	20.68% <0.00%> (-79.32%)	⬇️
... and 64 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 2a95629...765a91a. Read the comment docs.

[pyalex]

1. Sqlite is not a really good default choice for AWS or GCP. Simply because you will need to store sqlite file on S3 or GS to make it accessible by all services / clients (this is not local mode anymore) and natively sqlite is not even supporting S3 or GS (there are some hacks to make it work, but I would avoid them if possible).

2. For most users (that are using online store) it's totally fine and expected that Feast is trying to setup datastore / dynamo during apply. And some of them might have feature_store.yaml looking like this:

provider: aws
online_store:
  region: us-west-2

The proposed change will break their setup.

3. That's why I propose to make online_store Optional and allow explicitly set it to null. Yes, this will require some changes to apply flow and maybe updating some tests. But this would be a more consistent solution that won't break existing users' configurations and actually would solve the original problem, which was that some users don't need an online store, which is not equivalent to "some users want to have sqlite as default on AWS/GCP".

[felixwang9817]

looks great overall!

[felixwang9817]

@mirayyuce looks great overall! I think with a few more changes, this should be ready to merge in

[felixwang9817]

/lgtm

[feast-ci-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: felixwang9817, mirayyuce

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 [felixwang9817]

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

cc @pyalex I think your comments have been addressed

[felixwang9817]

@mirayyuce thanks for the contribution to Feast!!

[mirayyuce]

@mirayyuce thanks for the contribution to Feast!!

Thank you so much for the guidance!