chore: Update docstrings for offline stores and retrieval jobs #3062
Conversation
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
| with self._query_generator() as query: | ||
| return query | ||
|
|
||
| def to_bigquery( | ||
| self, | ||
| job_config: bigquery.QueryJobConfig = None, | ||
| job_config: Optional[bigquery.QueryJobConfig] = None, | ||
| timeout: int = 1800, |
There was a problem hiding this comment.
this should really be a timedelta type
There was a problem hiding this comment.
agreed but this is public facing so we'd have to issue some kind of a deprecation warning?
| with self._query_generator() as query: | ||
| return query | ||
|
|
||
| def to_bigquery( | ||
| self, | ||
| job_config: bigquery.QueryJobConfig = None, | ||
| job_config: Optional[bigquery.QueryJobConfig] = None, | ||
| timeout: int = 1800, | ||
| retry_cadence: int = 10, |
There was a problem hiding this comment.
This should probably be a retry policy (or a retrying object from tenacity) but that's quite low on the priority list.
| @@ -152,36 +140,49 @@ def to_arrow( | |||
|
|
|||
| return pyarrow.Table.from_pandas(features_df) | |||
|
|
|||
| @abstractmethod | |||
| def _to_df_internal(self) -> pd.DataFrame: | |||
There was a problem hiding this comment.
The docs should clarify why this exists and is different form to_df.
| pass | ||
|
|
||
| @abstractmethod | ||
| def _to_arrow_internal(self) -> pyarrow.Table: |
|
|
||
| @property | ||
| @abstractmethod | ||
| def on_demand_feature_views(self) -> Optional[List[OnDemandFeatureView]]: |
There was a problem hiding this comment.
I'd love it if we just return an empty list instead of a None and simplify the return type on this method
Codecov Report
@@ Coverage Diff @@
## master #3062 +/- ##
==========================================
+ Coverage 67.23% 75.72% +8.49%
==========================================
Files 169 202 +33
Lines 14920 16845 +1925
==========================================
+ Hits 10031 12756 +2725
+ Misses 4889 4089 -800
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: achals, felixwang9817 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 |
…-dev#3062) * Update offline store docstrings Signed-off-by: Felix Wang <wangfelix98@gmail.com> * Update docstrings for retrieval job Signed-off-by: Felix Wang <wangfelix98@gmail.com> * Clarify docstrings Signed-off-by: Felix Wang <wangfelix98@gmail.com> * Change type of on_demand_feature_views property Signed-off-by: Felix Wang <wangfelix98@gmail.com> Signed-off-by: Felix Wang <wangfelix98@gmail.com> Signed-off-by: Francisco Javier Arceo <arceofrancisco@gmail.com>
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #