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

Clean up inferencing code #1645

Closed
woop opened this issue Jun 12, 2021 · 2 comments · Fixed by #1646
Closed

Clean up inferencing code #1645

woop opened this issue Jun 12, 2021 · 2 comments · Fixed by #1646
Assignees
Labels

Comments

@woop
Copy link
Member

woop commented Jun 12, 2021

A few small refactors are necessary for our feature inferencing code.

All inferencing of columns from sources should only happen during apply time, not during the construction of objects. Otherwise these objects will execute eagerly during instantiation which prevents Feast from controlling the execution order and may even lead to duplicate executions.

One example is over here:

or self._infer_event_timestamp_column("TIMESTAMP|DATETIME"),

Also, inferencing code should be more cleanly separated. Right now we have a DataSource being subclassed to implementations, which reference the DataSource base class, which contains hardcoded references to the same child classes, which then call methods on this child classes. The interdependencies are overly complex and cyclic.
f55b51c#diff-fada7a84510bd3d3051cffc414ee51ed92eff4cdd4c2ac53a1b8b7c47286cab4R523

We can maybe just extract _infer_event_timestamp_column code into a function on its own.

Finally, we don't need to query tables in order to get schemas. We can simply use the get_table() method and inspect the schema instead of pulling data to the client f55b51c#diff-fada7a84510bd3d3051cffc414ee51ed92eff4cdd4c2ac53a1b8b7c47286cab4R756

@woop woop added priority/p1 good first issue Good for newcomers labels Jun 12, 2021
@mavysavydav
Copy link
Collaborator

got it, working on it

@woop
Copy link
Member Author

woop commented Jun 13, 2021

Thanks @mavysavydav!

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

Successfully merging a pull request may close this issue.

2 participants