Refactor Python SDK to remove v1 concepts #1023
Conversation
terryyylim
requested review from
davidheryanto,
khorshuheng,
pyalex,
woop and
zhilingc
as code owners
terryyylim
force-pushed
the
v1-clean-up
branch
9 times, most recently
from
bbfa265 to
54bb754
Compare
|
/retest |
terryyylim
changed the title
terryyylim
force-pushed
the
v1-clean-up
branch
from
60fbc86 to
6741c94
Compare
| name="alltypes", | ||
| entities=["alltypes_id"], | ||
| features=[ | ||
| Feature(name="float_feature", dtype=ValueType.FLOAT).to_proto(), |
There was a problem hiding this comment.
Why do we have to convert to_proto()? are users expected to do this as well?
sdk/python/tests/test_client.py
Outdated
| date_partition_column="date_partition_col", | ||
| ) | ||
|
|
||
| stream_source = DataSource( |
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 <[email protected]>
Signed-off-by: Terence <[email protected]>
Signed-off-by: Terence <[email protected]>
Signed-off-by: Terence <[email protected]>
Signed-off-by: Terence <[email protected]>
Signed-off-by: Terence <[email protected]>
Signed-off-by: Terence <[email protected]>
This reverts commit d567937eaf80190cde59128c19af4644c810e7d9. Signed-off-by: Terence <[email protected]>
Signed-off-by: Terence <[email protected]>
Signed-off-by: Terence <[email protected]>
This reverts commit 7e74e9069f97af9c0e108aba8f4bd1197ba5c3ed. Signed-off-by: Terence <[email protected]>
Signed-off-by: Terence <[email protected]>
Signed-off-by: Terence <[email protected]>
Signed-off-by: Terence <[email protected]>
terryyylim
force-pushed
the
v1-clean-up
branch
3 times, most recently
from
e144e7d to
ef014ad
Compare
Signed-off-by: Terence <[email protected]>
terryyylim
force-pushed
the
v1-clean-up
branch
from
ef014ad to
b6ee24b
Compare
|
@terryyylim: The following tests failed, say
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. 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
Needs 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?: