Implementing initial on demand transforms for historical retrieval to_df #1824
Conversation
adchia
requested review from
achals,
tsotnet,
woop and
a team
as code owners
|
Now that #1803 has been merged in, can you update this diff to only have the changes for the historical retrieval API? |
Signed-off-by: Danny Chiao <[email protected]>
Signed-off-by: Danny Chiao <[email protected]>
adchia
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1824 +/- ##
==========================================
- Coverage 84.60% 84.60% -0.01%
==========================================
Files 94 94
Lines 6946 6989 +43
==========================================
+ Hits 5877 5913 +36
- Misses 1069 1076 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Done! |
| @property | ||
| @abstractmethod | ||
| def full_feature_names(self) -> bool: | ||
| pass | ||
|
|
||
| @property | ||
| @abstractmethod | ||
| def on_demand_feature_views(self) -> Optional[List[OnDemandFeatureView]]: | ||
| pass |
There was a problem hiding this comment.
Why are we changing the API for the base class here and adding these fields? Are they necessary?
There was a problem hiding this comment.
NVM, I see why they're needed now. While I think this is okay for a first pass I want to see if we can get away from adding these additional fields on to the job. I think they are pretty inelegant for a user to have to implement themselves.
| @property | ||
| @abstractmethod | ||
| def full_feature_names(self) -> bool: | ||
| pass | ||
|
|
||
| @property | ||
| @abstractmethod | ||
| def on_demand_feature_views(self) -> Optional[List[OnDemandFeatureView]]: | ||
| pass |
There was a problem hiding this comment.
NVM, I see why they're needed now. While I think this is okay for a first pass I want to see if we can get away from adding these additional fields on to the job. I think they are pretty inelegant for a user to have to implement themselves.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: achals, adchia 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:
Sets up a limited on demand transform for historical retrieval for to_df (not to_arrow yet)
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: