Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data source conversion from protobuf doesn't use type information #2409

Closed
woop opened this issue Mar 15, 2022 · 2 comments · Fixed by #2424
Closed

Data source conversion from protobuf doesn't use type information #2409

woop opened this issue Mar 15, 2022 · 2 comments · Fixed by #2424

Comments

@woop
Copy link
Member

woop commented Mar 15, 2022

For some reason we are using field names

if data_source.request_data_options and data_source.request_data_options.schema:
to infer data source types. We should be using the actual data source types in the protos.

Also,

if data_source.data_source_class_type:
doesnt exist

@felixwang9817
Copy link
Collaborator

felixwang9817 commented Mar 15, 2022

@woop what do you mean by doesn't exist? I see data_source_class_type in DataSource.proto:

// This is an internal field that is represents the python class for the data source object a proto object represents.
// This should be set by feast, and not by users.
string data_source_class_type = 17;

and I also see it being set correctly in the code:

batch_source_proto = self.batch_source.to_proto()
batch_source_proto.data_source_class_type = f"{self.batch_source.__class__.__module__}.{self.batch_source.__class__.__name__}"

@adchia
Copy link
Collaborator

adchia commented Mar 15, 2022

Yep. I think there is some bug that might have been introduced when we started storing data sources separately since the feature view class adds that, but i don't think we add this class_type with applying data sources to the registry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants