Refactor Python SDK to remove v1 concepts - #1023
Conversation
bbfa265 to
54bb754
Compare
|
/retest |
60fbc86 to
6741c94
Compare
There was a problem hiding this comment.
Why do we have to convert to_proto()? are users expected to do this as well?
There was a problem hiding this comment.
We should be using specific sources like FileSource here and not the underlying base class. Otherwise the API is too low level.
There was a problem hiding this comment.
In protos, we have the different DataSources identified via its options here. Thus, in the sdk, I only exposed DataSource, and let the different options be configurable via native classes FileOptions, BigQueryOptions, KafkaOptions, KinesisOptions.
There was a problem hiding this comment.
Is our preference here to create native classes FileSource, BigQuerySource, KafkaSource and KinesisSource? I felt that following how protos were defined would be a more standardized approach for the codebase.
There was a problem hiding this comment.
The Source based approach (as in the RFC) is a more abstracted view. It allows us to define a source specific constructor. So they dont have to define the type, we can define that. We can also have input arguments that are specific to each source, and we dont require them to define FileOptions which is basically the same as having FileSource at the end of the day. So its better to just have a single constructor instead of two.
There was a problem hiding this comment.
Agreed that would be cleaner. I've updated the FeatureTable class and only expose FileSource, BigQuerySource, KafkaSource and KinesisSource instead of the respective options native classes.
Signed-off-by: Terence <terencelimxp@gmail.com>
Signed-off-by: Terence <terencelimxp@gmail.com>
Signed-off-by: Terence <terencelimxp@gmail.com>
Signed-off-by: Terence <terencelimxp@gmail.com>
Signed-off-by: Terence <terencelimxp@gmail.com>
Signed-off-by: Terence <terencelimxp@gmail.com>
Signed-off-by: Terence <terencelimxp@gmail.com>
This reverts commit d567937eaf80190cde59128c19af4644c810e7d9. Signed-off-by: Terence <terencelimxp@gmail.com>
Signed-off-by: Terence <terencelimxp@gmail.com>
Signed-off-by: Terence <terencelimxp@gmail.com>
This reverts commit 7e74e9069f97af9c0e108aba8f4bd1197ba5c3ed. Signed-off-by: Terence <terencelimxp@gmail.com>
Signed-off-by: Terence <terencelimxp@gmail.com>
Signed-off-by: Terence <terencelimxp@gmail.com>
Signed-off-by: Terence <terencelimxp@gmail.com>
e144e7d to
ef014ad
Compare
Signed-off-by: Terence <terencelimxp@gmail.com>
ef014ad to
b6ee24b
Compare
|
@terryyylim: The following tests failed, say
DetailsInstructions 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. I understand the commands that are listed here. |
|
/retest |
| features: Union[FeatureV2, List[FeatureV2]], | ||
| batch_source: Optional[DataSource] = None, | ||
| stream_source: Optional[DataSource] = None, | ||
| entities: List[str], |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: terryyylim, woop The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
What this PR does / why we need it:
In upcoming releases, we'll be slowly shifting towards a different architecture as defined in this RFC. This PR is the first of a series that'll be related to cleaning up older concepts of Feast in the codebase.
The following changes would be included in subsequent PRs, likely when new retrieval method has been implemented.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: