[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: imjuanleonard
To complete the pull request process, please assign zhilingc
You can assign the PR to them by writing /assign @zhilingc in a comment when ready.

The full list of commands accepted by this bot can be found here.

Hi @imjuanleonard. Thanks for your PR.

I'm waiting for a gojek member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Hello @woop

Do let me know whether we Run python Black linter for the generated files as well, or not, I will make the changes accordingly.
Thanks

oh yes sorry @imjuanleonard
After another look I think we should not include this in in setup.py

"tensorflow-metadata>=0.21,<0.22",

All the tensorflow metadata dependencies actually already exist in protos/tensorflow_metadata folder, we just did not generate the Python code in our Makefile.

So I think we should instead update the Makefile.
https://github.com/gojek/feast/blob/0319b632dcf1d56ba9fa055910a97c8d05df422d/protos/Makefile#L15

This also ensures that we will have the same version of Tensorflow metadata proto across different Feast components. Otherwise the one used in Python SDK may be newer/older than the ones in Golang or Java SDK.

/ok-to-test

The following users are mentioned in OWNERS file(s) but are not members of the gojek org.

Once all users have been added as members of the org, you can trigger verification by writing /verify-owners in a comment.

• imjuanleonard
  • OWNERS

This appears to overlap with #450 FWIW.

Do let me know whether we Run python Black linter for the generated files as well, or not, I will make the changes accordingly.

It seems like a waste (formatting very large generated files is time developers will pay for over and over), but as they cohabit the same module as non-generated code, I suppose a bit of fiddly blacklist or whitelist configuration would be needed to work around it.

IMO a few things would be nice (not suggesting in the scope of this PR):

• Separate module for generated code in the Python SDK
• Generated code produced by build automation and kept out of source control (Remove auto generated files from git #494)
• Usage and version of code formatter managed by build automation

Just my opinions, but all these are true for Java.

This appears to overlap with #450 FWIW.

Do let me know whether we Run python Black linter for the generated files as well, or not, I will make the changes accordingly.

It seems like a waste (formatting very large generated files is time developers will pay for over and over), but as they cohabit the same module as non-generated code, I suppose a bit of fiddly blacklist or whitelist configuration would be needed to work around it.

IMO a few things would be nice (not suggesting in the scope of this PR):

• Separate module for generated code in the Python SDK
• Generated code produced by build automation and kept out of source control (Remove auto generated files from git #494)
• Usage and version of code formatter managed by build automation

Just my opinions, but all these are true for Java.

• I am happy blacklisting that code from black.
• We discussed the option of not actually version controlling generated code, but we think that there is an argument for keeping it. It allows users to pip install the code easily, especially straight from GitHub. MLflow is an example of popular project that does the same. For non-Python languages I feel less opinionated.
• How about we just have make lint or make format at the root of the Feast repo? That would be based on hardcoded configuration that everyone can see, and should ideally cover the whole repo (Java, Go, Python).

• We discussed the option of not actually version controlling generated code, but we think that there is an argument for keeping it. It allows users to pip install the code easily, especially straight from GitHub.

pip install from Git makes sense. A setup.py could potentially do it, but maybe that's a hack and a problem would be requiring protoc installed. Similar for Go, where you could otherwise possibly support go get with go:generate build tags.

Versioned, platform-specific protoc from a Maven package is pretty handy 😎

• How about we just have make lint or make format at the root of the Feast repo? That would be based on hardcoded configuration that everyone can see, and should ideally cover the whole repo (Java, Go, Python).

👍 Created #526, now back on course from this diversion!

I am closing this PR in favour of #545.

@ches I think we can just remove the Python generated code for now. Doesnt seem to be worth all the extra trouble. We can assess the impact later. Java and Go remain unchanged.

Lets continue the discussion on #526