Set created_timestamp and last_updated_timestamp fields
#2266
Conversation
Signed-off-by: Judah Rand <[email protected]>
Signed-off-by: Judah Rand <[email protected]>
Signed-off-by: Judah Rand <[email protected]>
feast-ci-bot
added
release-note
and removed
do-not-merge/release-note-label-needed
labels
|
/kind bug |
|
/ok-to-test |
Codecov Report
@@ Coverage Diff @@
## master #2266 +/- ##
==========================================
+ Coverage 85.82% 86.01% +0.19%
==========================================
Files 116 116
Lines 9790 9809 +19
==========================================
+ Hits 8402 8437 +35
+ Misses 1388 1372 -16
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Signed-off-by: Judah Rand <[email protected]>
judahrand
changed the title
last_updated_timestamp to BaseFeatureViewcreated_timestamp and last_updated_timestamp fields
| @@ -47,6 +48,8 @@ class OnDemandFeatureView(BaseFeatureView): | |||
| input_feature_view_projections: Dict[str, FeatureViewProjection] | |||
| input_request_data_sources: Dict[str, RequestDataSource] | |||
| udf: MethodType | |||
| created_timestamp: Optional[datetime] = None | |||
There was a problem hiding this comment.
What do you mean? The other FeatureView classes don't take it in __init__. I was just making sure they all matched in terms of behaviour.
Arguably, this setting of created_timestamp to None should maybe be inherited behaviour.
There was a problem hiding this comment.
I mean the docstring. I also think this logic should ideally be inherited.
There was a problem hiding this comment.
Still not sure what you mean 🤔 The Docstring shouldn't mention this should it as it isn't an arg?
There was a problem hiding this comment.
Turns out this logic was already performed by the parent class. The logic must've been left in accidentally.
|
On a different note, it's kind of weird how we have both |
Signed-off-by: Judah Rand <[email protected]>
judahrand
force-pushed
the
odfv-updated
branch
from
188053c
to
5de401d
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adchia, felixwang9817, judahrand 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:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Both the BaseFeatureView and OnDemandFeatureView were missing this field, whereas FeatureView had it.
It was also not set when applying a FeatureView.
Which issue(s) this PR fixes:
Add the field where it is missing and actually set it when applying changes to the Registry.
Fixes #2267
Does this PR introduce a user-facing change?: