Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updating CLI apply to use FeatureStore & remove feature table logic #1741

Closed
wants to merge 4 commits into from

Conversation

adchia
Copy link
Collaborator

@adchia adchia commented Jul 28, 2021

What this PR does / why we need it:

Moves the feast CLI's apply method (feast apply) to use the SDK FeatureStore class more. This includes some cleanup that also removes obsolete logic around feature tables. Note that because the feast CLI's apply is a total apply whereas the SDK apply is partial, there is still leftover business logic (in particular for deleting entities / FVs that don't match the repo).

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:


action required: feature tables were unofficially supported via the CLI and now no longer are

@adchia adchia requested review from achals, tsotnet, woop and a team as code owners July 28, 2021 22:51
@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: adchia
To complete the pull request process, please assign jklegar after the PR has been reviewed.
You can assign the PR to them by writing /assign @jklegar in a comment when ready.

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

Needs approval from an approver in each of these files:

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

@adchia
Copy link
Collaborator Author

adchia commented Jul 28, 2021

/assign @jklegar

@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2021

Codecov Report

Merging #1741 (24cc75a) into master (95a245a) will decrease coverage by 16.98%.
The diff coverage is 14.28%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1741       +/-   ##
===========================================
- Coverage   79.10%   62.11%   -16.99%     
===========================================
  Files          81       79        -2     
  Lines        6938     6834      -104     
===========================================
- Hits         5488     4245     -1243     
- Misses       1450     2589     +1139     
Flag Coverage Δ
integrationtests ?
unittests 62.11% <14.28%> (+0.17%) ⬆️

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

Impacted Files Coverage Δ
sdk/python/feast/repo_operations.py 34.32% <9.37%> (+3.13%) ⬆️
sdk/python/feast/feature_store.py 91.25% <66.66%> (-2.39%) ⬇️
.../integration/online_store/test_online_retrieval.py 17.39% <0.00%> (-82.61%) ⬇️
sdk/python/tests/utils/online_read_write_test.py 18.18% <0.00%> (-81.82%) ⬇️
...gration/registration/test_feature_service_apply.py 31.25% <0.00%> (-68.75%) ⬇️
...hon/tests/integration/registration/test_cli_gcp.py 31.70% <0.00%> (-68.30%) ⬇️
...n/tests/integration/registration/test_cli_redis.py 32.55% <0.00%> (-67.45%) ⬇️
sdk/python/feast/infra/online_stores/redis.py 30.35% <0.00%> (-64.29%) ⬇️
sdk/python/feast/infra/online_stores/datastore.py 31.03% <0.00%> (-62.07%) ⬇️
sdk/python/feast/infra/online_stores/dynamodb.py 30.37% <0.00%> (-60.76%) ⬇️
... and 31 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 95a245a...24cc75a. Read the comment docs.

@achals
Copy link
Member

achals commented Jul 28, 2021

Looks good to me. I wonder if we should change the apply method on the feature store to be a total apply, or add a total_apply method, so that this logic can be used without relying on repo_operations.

@adchia
Copy link
Collaborator Author

adchia commented Jul 29, 2021

Moving to different PR since I added a dev branch to the main repo

@adchia adchia closed this Jul 29, 2021
@adchia adchia deleted the cliRefactor branch July 29, 2021 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants