Add created timestamp for feature views #1952

amommendes · 2021-10-17T21:29:25Z

What this PR does / why we need it:
Adds created timestamp for feature views

Which issue(s) this PR fixes:

Fixes #1933

Added created timestamp to Feature Views

feast-ci-bot · 2021-10-17T21:29:36Z

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

* Update hero image Signed-off-by: Danny Chiao <danny@tecton.ai> * Fix roadmap extra line Signed-off-by: Danny Chiao <danny@tecton.ai> Signed-off-by: Amom Mendes <amommendes@hotmail.com>

Signed-off-by: Amom Mendes <amommendes@hotmail.com>

amommendes · 2021-10-17T21:34:20Z

Hey @achals , I think this is not good with a nested if. However BaseFeatureView does not have created_timestamp and since OnDemandFeatureView does not need created_timestamp does not make sense to add this in the base feature view... WDYT?

achals · 2021-10-18T06:28:33Z

Hey @achals , I think this is not good with a nested if. However BaseFeatureView does not have created_timestamp and since OnDemandFeatureView does not need created_timestamp does not make sense to add this in the base feature view... WDYT?

I think it's safer to add the created_timestamp to the OnDemandFeatureView for consistency, in the same way that it's defined for FeatureViews.

amommendes · 2021-10-18T12:21:12Z

Hey @achals , I think this is not good with a nested if. However BaseFeatureView does not have created_timestamp and since OnDemandFeatureView does not need created_timestamp does not make sense to add this in the base feature view... WDYT?

I think it's safer to add the created_timestamp to the OnDemandFeatureView for consistency, in the same way that it's defined for FeatureViews.

Thanks @achals... All right... If makes sense to have created_timestamp in the OnDemandFeatureView, then we can add it to the BaseFeatureView in order to maintain the signature consistency between child classes...

… view Signed-off-by: Amom Mendes <amommendes@hotmail.com>

felixwang9817 · 2021-10-20T19:45:58Z

/ok-to-test

felixwang9817 · 2021-10-20T19:46:06Z

/kind feature

codecov-commenter · 2021-10-20T19:48:54Z

Codecov Report

Merging #1952 (fc4bcab) into master (2435777) will decrease coverage by 0.25%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1952      +/-   ##
==========================================
- Coverage   84.64%   84.38%   -0.26%     
==========================================
  Files         103      103              
  Lines        8523     8531       +8     
==========================================
- Hits         7214     7199      -15     
- Misses       1309     1332      +23

Flag	Coverage Δ
integrationtests	`75.71% <100.00%> (-0.24%)`	⬇️
unittests	`60.01% <45.45%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
sdk/python/feast/feature_view.py	`89.24% <ø> (+1.19%)`	⬆️
sdk/python/feast/base_feature_view.py	`78.26% <100.00%> (+0.48%)`	⬆️
sdk/python/feast/on_demand_feature_view.py	`95.20% <100.00%> (+0.20%)`	⬆️
sdk/python/feast/registry.py	`80.90% <100.00%> (+0.13%)`	⬆️
sdk/python/feast/infra/aws.py	`72.59% <0.00%> (-11.06%)`	⬇️
sdk/python/feast/infra/online_stores/datastore.py	`91.33% <0.00%> (-2.37%)`	⬇️
.../integration/online_store/test_universal_online.py	`98.13% <0.00%> (+0.46%)`	⬆️

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 2435777...fc4bcab. Read the comment docs.

amommendes · 2021-10-20T23:21:51Z

Hey @achals , there is an error on linting step because of this exception:
feast/infra/gcp.py:25: error: Module 'google.cloud' has no attribute 'storage'

Is there some problem with the version of this lib?

SanRehmo · 2021-10-22T07:51:19Z

Hey @achals , there is an error on linting step because of this exception: feast/infra/gcp.py:25: error: Module 'google.cloud' has no attribute 'storage'

Is there some problem with the version of this lib?

This might be caused by an issue with mypy: python/mypy#10360

felixwang9817 · 2021-10-22T18:53:16Z

@SanRehmo, thanks for pointing that out! That was indeed the problem, which has now been fixed. @amommendes, would you mind rebasing your changes on master and then force pushing your changes up? Thanks!

amommendes · 2021-10-26T03:47:58Z

@SanRehmo, thanks for pointing that out! That was indeed the problem, which has now been fixed. @amommendes, would you mind rebasing your changes on master and then force pushing your changes up? Thanks!

Done @felixwang9817 , tks!

Signed-off-by: Amom Mendes <amommendes@hotmail.com>

sdk/python/feast/base_feature_view.py

achals

Thanks @amommendes , I think we need one more change for this to work as expected

Signed-off-by: Amom Mendes <amommendes@hotmail.com>

amommendes · 2022-01-06T12:47:08Z

Hey @achals , some update on this PR? WDYT?

achals

/lgtm

Sorry, I missed this in my inbox. Looks good!

feast-ci-bot · 2022-01-06T16:05:00Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

amommendes requested review from achals, adchia, felixwang9817, tsotnet, woop and a team as code owners October 17, 2021 21:29

feast-ci-bot added do-not-merge/release-note-label-needed needs-kind labels Oct 17, 2021

feast-ci-bot added the needs-ok-to-test label Oct 17, 2021

github-actions bot assigned woop Oct 17, 2021

feast-ci-bot added the size/XS label Oct 17, 2021

adchia and others added 2 commits October 17, 2021 18:29

Updating roadmap + hero image (feast-dev#1950)

9cd9f74

* Update hero image Signed-off-by: Danny Chiao <danny@tecton.ai> * Fix roadmap extra line Signed-off-by: Danny Chiao <danny@tecton.ai> Signed-off-by: Amom Mendes <amommendes@hotmail.com>

✨ Add created timestamp for feature view

2a7f2b0

Signed-off-by: Amom Mendes <amommendes@hotmail.com>

amommendes force-pushed the feature-view-create-time branch from 6003741 to 2a7f2b0 Compare October 17, 2021 21:29

feast-ci-bot added size/M and removed size/XS labels Oct 17, 2021

feast-ci-bot added release-note and removed do-not-merge/release-note-label-needed labels Oct 17, 2021

🎨 Adding created timestamp to base feature view and on demand feature…

2cd32b9

… view Signed-off-by: Amom Mendes <amommendes@hotmail.com>

amommendes force-pushed the feature-view-create-time branch from e0a7cbe to 2cd32b9 Compare October 20, 2021 01:34

feast-ci-bot added ok-to-test and removed needs-ok-to-test labels Oct 20, 2021

feast-ci-bot added kind/feature New feature or request and removed needs-kind labels Oct 20, 2021

🔀 Merge master branch

3942edc

🚨 Fix import

63bae67

Signed-off-by: Amom Mendes <amommendes@hotmail.com>

amommendes force-pushed the feature-view-create-time branch from 43f0e34 to 63bae67 Compare October 26, 2021 04:38

🔥 Remove unused import

cf41640

Signed-off-by: Amom Mendes <amommendes@hotmail.com>

amommendes force-pushed the feature-view-create-time branch from cabaab7 to cf41640 Compare October 26, 2021 04:45

Merge branch 'master' into feature-view-create-time

fb9e9db

feast-ci-bot added size/S and removed size/M labels Oct 29, 2021

Merge branch 'feast-dev:master' into feature-view-create-time

1405f9b

achals reviewed Oct 29, 2021

View reviewed changes

sdk/python/feast/base_feature_view.py Show resolved Hide resolved

achals requested changes Oct 29, 2021

View reviewed changes

Merge branch 'feast-dev:master' into feature-view-create-time

59f9a77

amommendes force-pushed the feature-view-create-time branch from 251d4f5 to 7d91390 Compare November 14, 2021 17:00

amommendes added 2 commits November 15, 2021 01:02

🎨 Add read/write created timestamp to ODFV

d903906

Signed-off-by: Amom Mendes <amommendes@hotmail.com>

🚨 Fix isort lint

fc4bcab

Signed-off-by: Amom Mendes <amommendes@hotmail.com>

amommendes force-pushed the feature-view-create-time branch from 7d91390 to fc4bcab Compare November 15, 2021 04:04

amommendes requested a review from achals November 19, 2021 12:23

achals approved these changes Jan 6, 2022

View reviewed changes

feast-ci-bot assigned achals Jan 6, 2022

feast-ci-bot added the lgtm label Jan 6, 2022

feast-ci-bot added the approved label Jan 6, 2022

feast-ci-bot merged commit ad3ea8d into feast-dev:master Jan 6, 2022

felixwang9817 mentioned this pull request Jan 7, 2022

Compare only specs in integration tests #2200

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add created timestamp for feature views #1952

Add created timestamp for feature views #1952

amommendes commented Oct 17, 2021 •

edited

Loading

feast-ci-bot commented Oct 17, 2021

amommendes commented Oct 17, 2021 •

edited

Loading

achals commented Oct 18, 2021

amommendes commented Oct 18, 2021

felixwang9817 commented Oct 20, 2021

felixwang9817 commented Oct 20, 2021

codecov-commenter commented Oct 20, 2021 •

edited

Loading

amommendes commented Oct 20, 2021

SanRehmo commented Oct 22, 2021

felixwang9817 commented Oct 22, 2021

amommendes commented Oct 26, 2021

achals left a comment

amommendes commented Jan 6, 2022

achals left a comment

feast-ci-bot commented Jan 6, 2022

Add created timestamp for feature views #1952

Add created timestamp for feature views #1952

Conversation

amommendes commented Oct 17, 2021 • edited Loading

feast-ci-bot commented Oct 17, 2021

amommendes commented Oct 17, 2021 • edited Loading

achals commented Oct 18, 2021

amommendes commented Oct 18, 2021

felixwang9817 commented Oct 20, 2021

felixwang9817 commented Oct 20, 2021

codecov-commenter commented Oct 20, 2021 • edited Loading

Codecov Report

amommendes commented Oct 20, 2021

SanRehmo commented Oct 22, 2021

felixwang9817 commented Oct 22, 2021

amommendes commented Oct 26, 2021

achals left a comment

Choose a reason for hiding this comment

amommendes commented Jan 6, 2022

achals left a comment

Choose a reason for hiding this comment

feast-ci-bot commented Jan 6, 2022

amommendes commented Oct 17, 2021 •

edited

Loading

amommendes commented Oct 17, 2021 •

edited

Loading

codecov-commenter commented Oct 20, 2021 •

edited

Loading