Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Python rules for Bazel #3751

Closed
wants to merge 37 commits into from
Closed

Conversation

joshfischer1108
Copy link
Member

No description provided.

@joshfischer1108
Copy link
Member Author

joshfischer1108 commented Jan 3, 2022

Steps to completion: Not exhaustive

  • Add native python rules to workspace
  • Update docker rules because of a deprecated pip_import dependency
  • Remove pex rules from the project
  • Add requirements.txt to support all of the python dependencies for the project. This is only a first pass to loading python deps. After the initial migration is done we can lazily load python deps for improved build times, if desired
  • Remove tools/rules/newgenproto.bzl file as it seems to be a duplicate.
  • Swap out pex_library with native py_libray rules
  • and pex_binary with native py_binary rules
  • Investigate the use of starlark to replace python build scripts
  • Investigate the use for py_proto_libaries from google https://thethoughtfulkoala.com/posts/2020/05/08/py-protobuf-bazel.html
  • pex_binary has a number of rules which do not translate over to py_binary: resources, zip_safe. We need to find an alternative to these.

There are no references to this file in any of the build scripts. A full build with the complete test battery, without this file, yielded no issues.
@nicknezis
Copy link
Contributor

nicknezis commented Jan 3, 2022

If we are removing all use of pex, we will need to ensure we can still Launch HeronPy topologies. The Heron Executor code currently decides which binary type based on file extension. The logic in this file will need to be updated based on these changes.

if self.pkg_type == 'jar' or self.pkg_type == 'tar':
retval.update(self._get_java_instance_cmd(instance_info))
elif self.pkg_type == 'pex':
retval.update(self._get_python_instance_cmd(instance_info))
elif self.pkg_type == 'so':
retval.update(self._get_cpp_instance_cmd(instance_info))
elif self.pkg_type == 'dylib':
retval.update(self._get_cpp_instance_cmd(instance_info))

@surahman surahman force-pushed the joshfischer/native-python-rules branch from 2c91881 to 806c25b Compare January 25, 2022 23:13
@surahman surahman force-pushed the joshfischer/native-python-rules branch from d2675cf to eec9864 Compare January 25, 2022 23:47
@surahman
Copy link
Member

Draft removal of most references to pex_binary and pex_library.

@nwangtw
Copy link
Contributor

nwangtw commented Jan 26, 2022

This would be really cool~

There is no direct translation for this in py_binary.
There is no direct translation for this. It also appears that newer versions of <bazel_rules_pex> have switched to calling this <data> which aligns with <py_binary>.
@surahman surahman force-pushed the joshfischer/native-python-rules branch from 821b857 to 7a809e8 Compare January 26, 2022 19:44
@surahman
Copy link
Member

surahman commented Jan 26, 2022

There is no zip_safe and resources in py_binary. I have opted to disable zip_safe and placed the change in its own commit so it can be dropped if needed.

resources in bazel_pex_rules appears to have switched to calling this data:

List of labels; Optional; Default is []

Files to include as resources in the final pex binary.

Putting other rules here will cause the outputs of those rules to be embedded in this one. Files will be included as-is. Paths in the archive will be relative to the workspace root.

I think this may align with Bazel's definition of data, but it uses those files at rule runtime:

List of labels; optional

Files needed by this rule at runtime. May list file or rule targets. Generally allows any target.

@surahman surahman force-pushed the joshfischer/native-python-rules branch from d50c8e4 to 6c524b4 Compare January 27, 2022 22:12
@surahman
Copy link
Member

surahman commented Jan 27, 2022

I just reverted the commits on pylint and cpplint.

Edit: I am still learning Bazel and understanding the build scripts. I can not trigger the infinite loop that CI is experiencing on my local machines:

(00:13:40) ERROR: Cycle detected but could not be properly displayed due to an internal problem. 
Please file an issue. Raw display: topLevelKey: ConfiguredTargetKey{label=//heron/api/src/cpp:cxx-api, config=BuildConfigurationValue.Key[a5ba794ad3f6c1fd9f8a7f4293435609fb1ef8a34f21b35e3d8a289fcb873f5e]}

The error seems to be caused by an infinite recursion/expansion of the build rule cpplint. I think the culprit is main being set to cpplint because when I set it to main.py there is no infinite loop anymore.

entrypoint = "cpplint",
reqs = ["cpplint==1.3.0"],
srcs = ["empty.py"],
main = "cpplint",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be causing an infinite recursion/expansion of the build rule?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it could be possible. @nicknezis would have a better idea. He's been our go to Bazel person,

@surahman
Copy link
Member

I think the best way to approach this enormous task is to completely remove a single PEX build rule at a time and make sure everything compiles and all tests pass. Start with the simpler build rules and move to the more complex ones. That way we can verify in a concrete fashion that the changes we are making are correct. It also means the work we are completing can be incrementally merged back into master so that there is progress.

Conceivably a Kanban board that tracks the rules to remove? We can clump some together for fewer PR's.

@joshfischer1108
Copy link
Member Author

I like the idea of removing a single build rule at a time. After jumping into a lot this code I think we've learned a lot (a good time for growth 😄 ) Are you thinking we should abandon this branch and start fresh?

@surahman
Copy link
Member

Are you thinking we should abandon this branch and start fresh?

We are thinking alike 😁

@joshfischer1108
Copy link
Member Author

Closing to account for smaller chunks of work.

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

Successfully merging this pull request may close these issues.

4 participants