oes the GCP provider, so having the offline_store type option might be confusing to users especially since it doesn't affect interaction with the offline store. Not sure if that's already been discussed, and not sure what the better alternative would be though. Thoughts?

Ultimately it seems like we have to support decoupling storage (offline, online) from the provider if we want to have reuse, even if the provider still has to opt-in (or can override) the storage implementation. So at first glance, it seems natural to have the offline_store object to me.

since it doesn't affect interaction with the offline store

What is the "it" here, the "offline_store" object, or the "provider" option?

+1 to Willem's question, I am not sure what the "it" here is. This seems like a fine change to our API, but I might be missing something here:

1. You choose a provider. Providers opt into supporting different offline stores
2. You choose an offline store with the type parameter. Right now, there is only one offline store, BQ. There should be one default offline store per provider.
3. Additional configs to the offline store go in additional keyword parameters.

@tsotnet, is there a default offline store type for each provider? I just want to check, but I don't think users should have to specify the type field in the happy path case. There is only one right now after all.

"it" here is the offline_store type - users can set the type to be bigquery and then use file sources or both file and bigquery sources, or vice versa

@tsotnet, is there a default offline store type for each provider? I just want to check, but I don't think users should have to specify the type field in the happy path case. There is only one right now after all.

@jay there is a default offline store type. As you'd expect FileOfflineStore for local provider and BigqueryOfflineStore for gcp provider.

"it" here is the offline_store type - users can set the type to be bigquery and then use file sources or both file and bigquery sources, or vice versa

@jklegar There are 2 directions we can go from here:

1. During feast apply validate that all data sources are of the expected type.
2. Enable multiple offline stores to be used in one GCP project. In this case we'll need to make offline_store field a list instead of struct in feature_store.yaml. So for example:

project: master_kitten
registry: data/registry.db
provider: gcp
offline_store:
    - type: bigquery
      entity_dataset_name: foo
    - type: local
      imaginary_config: imaginary_value

I think in a long term we should support multiple offline stores (and even multiple online stores). But it's not as simple. Currently we partially support this: we can have different data sources in a project, but when getting a historical data all requested features need to be from the same offline store. I recommend a middle ground: enable defining multiple offline store configs, but the behavior of getting a historical data stays the same for now.

Thoughts?

Codecov Report

Merging #1552 (27128b7) into master (f050976) will decrease coverage by 0.12%.
The diff coverage is 87.91%.

@@            Coverage Diff             @@
##           master    #1552      +/-   ##
==========================================
- Coverage   83.71%   83.59%   -0.13%     
==========================================
  Files          65       65              
  Lines        5632     5680      +48     
==========================================
+ Hits         4715     4748      +33     
- Misses        917      932      +15     

Flags with carried forward coverage won't be shown. Click here to find out 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 f050976...27128b7. Read the comment docs.

/lgtm

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tsotnet, woop

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

The pull request process is described here