Added --skip-source-validation flag to feast apply

[mavysavydav]

Signed-off-by: David Y Liu [email protected]

What this PR does / why we need it:
Companies that have permission controls may make validation impossible since one user may not have access to all the tables of the data sources, and therefore can't run feast apply.

Example error msg upon running feast apply
google.api_core.exceptions.Forbidden: 403 GET https://bigquery.googleapis.com/bigquery/v2/projects/twttr-gcp-aggregates-vteam-stg/datasets/wtf/tables/country_candidate_user_aggregates_v1?prettyPrint=false: Access Denied: Table twttr-gcp-aggregates-vteam-stg:wtf.country_candidate_user_aggregates_v1: Permission bigquery.tables.get denied on table twttr-gcp-aggregates-vteam-stg:wtf.country_candidate_user_aggregates_v1 (or it may not exist).

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

The newly added --skip-source-validation flag in `feast apply` turns off data source validation when calling feast apply. This may be useful if you have strict permissioning and can't check for the existence of each table. 

[feast-ci-bot]

Hi @mavysavydav. 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.

[codecov-commenter]

Codecov Report

Merging #1702 (4a7ea3f) into master (0d0492d) will decrease coverage by 0.74%.
The diff coverage is 42.85%.

@@            Coverage Diff             @@
##           master    #1702      +/-   ##
==========================================
- Coverage   82.75%   82.00%   -0.75%     
==========================================
  Files          76       78       +2     
  Lines        6754     7981    +1227     
==========================================
+ Hits         5589     6545     +956     
- Misses       1165     1436     +271     

Flag	Coverage Δ
integrationtests	84.19% <42.85%> (+1.51%)	⬆️
unittests	69.72% <42.85%> (-0.01%)	⬇️

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

Impacted Files	Coverage Δ
sdk/python/feast/repo_operations.py	31.21% <25.00%> (-0.16%)	⬇️
sdk/python/feast/cli.py	46.52% <66.66%> (+0.37%)	⬆️
sdk/python/tests/test_cli_redis.py	83.33% <0.00%> (-16.67%)	⬇️
sdk/python/feast/infra/offline_stores/bigquery.py	61.17% <0.00%> (-14.98%)	⬇️
...hon/tests/test_offline_online_store_consistency.py	87.15% <0.00%> (-12.85%)	⬇️
sdk/python/feast/entity.py	93.49% <0.00%> (-2.88%)	⬇️
sdk/python/feast/feature_store.py	94.03% <0.00%> (-0.41%)	⬇️
...dk/python/tensorflow_metadata/proto/v0/path_pb2.py	100.00% <0.00%> (ø)
.../python/tensorflow_metadata/proto/v0/schema_pb2.py	100.00% <0.00%> (ø)
...hon/tensorflow_metadata/proto/v0/statistics_pb2.py	100.00% <0.00%> (ø)
... and 17 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 0d0492d...4a7ea3f. Read the comment docs.

[mavysavydav]

lint error is due to this unrelated thing:

[woop]

lint error is due to this unrelated thing:

That's weird, it's not happening on master.

[woop]

Signed-off-by: David Y Liu [email protected]

What this PR does / why we need it:
Companies that have permission controls may make validation impossible since one user may not have access to all the tables of the data sources, and therefore can't run feast apply.

Example error msg upon running feast apply
google.api_core.exceptions.Forbidden: 403 GET https://bigquery.googleapis.com/bigquery/v2/projects/twttr-gcp-aggregates-vteam-stg/datasets/wtf/tables/country_candidate_user_aggregates_v1?prettyPrint=false: Access Denied: Table twttr-gcp-aggregates-vteam-stg:wtf.country_candidate_user_aggregates_v1: Permission bigquery.tables.get denied on table twttr-gcp-aggregates-vteam-stg:wtf.country_candidate_user_aggregates_v1 (or it may not exist).

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

The newly added --validate-off flag in `feast apply` turns off data source validation when calling feast apply. This may be useful if you have strict permissioning and can't check for the existence of each table. 

Should we perhaps be a bit more explicit in the naming? What about --skip-source-validation? Or what about --validate_sources and set the default to True?

[mavysavydav]

sounds good, i'll rename flag to --skip-source-validation. For the lint issue, is there anything i can do to resolve it or will we ignore it for this PR? i've checked for pip upgrades and also ran pip install -e "sdk/python[ci]".

[mavysavydav]

-- also, when i run make lint-python on master, i get teh same error

[woop]

I think it's a dependency issue. @achals is looking into it.

[achals]

I think it's a dependency issue. @achals is looking into it.

I just landed a fix. @mavysavydav Can you please rebase and see if the problem is fixed?

[mavysavydav]

/kind feature

[achals]

Hey @mavysavydav, can we add a simple test for this?

[mavysavydav]

thanks for the review @achals. I think writing a test case for this may introduce more complexity in the test files than it's worth especially since there's no natural place to put the test. We'd also be essentially testing one "if" statement. I would probably put it in test_repo_operations but right now that test file is super minimal and i'd probably need to pull in a bunch of dependencies to test appropriately.

[achals]

thanks for the review @achals. I think writing a test case for this may introduce more complexity in the test files than it's worth especially since there's no natural place to put the test. We'd also be essentially testing one "if" statement. I would probably put it in test_repo_operations but right now that test file is super minimal and i'd probably need to pull in a bunch of dependencies to test appropriately.

Fair. We're working on refactoring the tests the coming few days, so hopefully a test for this kind of thing would be easier to add in the future.

[achals]

/lgtm

[feast-ci-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: achals, mavysavydav

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

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