feat: generated protobuf classes by danepitkin · Pull Request #1 · substrait-io/substrait-python

danepitkin · 2023-04-17T20:50:32Z

A python package containing the generated protobuf classes.

Use buf and protoletariat to generate the protobuf classes
Use setuptools for python packaging and dynamic versioning
Pin Substrait submodule to latest release (v0.28.2)

Testing:

Ran through README instructions to generate proto, build python package, run pytests, and import module locally.

vibhatha · 2023-04-18T01:07:02Z

@@ -0,0 +1,10 @@
+def test_imports():
+    from pysubstrait.proto.pysubstrait.algebra_pb2 import Expression


Just curious about the pysubstrait.proto.pysubstrait... could we make it something like
from pysubstrait.proto.core.algebra_pb2 import Expression or just from pysubstrait.proto.algebra_pb2 import Expression

I would also have preferred that, but the second pysubstrait was added as a namespace for the generated protobuf classes so as not to conflict with other implementations like C++. I used this script in gen_proto.sh to generate it: https://github.com/substrait-io/substrait/blob/main/tools/proto_prefix.py. I'm open to a better naming convention if anyone has one!

Aha, I understand your point. 🤔

We can always merge as-is and change it in the future while the project is still pre- v1.0. For reference, Ibis-substrait sort of does it this way, too: https://github.com/ibis-project/ibis-substrait/tree/main/ibis_substrait/proto/substrait/ibis

Note that this approach has some dangers when using extensions (e.g. google.protobuf.Any) Though my understanding was that Ibis-substrait had found some way to solve this?

I think we haven't actually run into this in ibis-substrait yet, but I don't know that anyone has tried it.

Do you mind elaborating on the dangers with an example?

Sure. Let's pretend I created a message (EstimatedSelectivity) that was intended to be used as an advanced extension on RelCommon::Hint and we merged it into Substrait:

message RelCommon { ... Hint hint = 3; ... message Hint { ... substrait.extensions.AdvancedExtension advanced_extension = 10; ... } ... } message EstimatedSelectivity { double selectivity = 1; }

Then, I attached that hint onto a filter relation. Here is the plan, serialized as JSON:

{ ... "filter": { "common": { "hint": { "optimization": { "@type": "substrait.EstimatedSelectivity", "selectivity": 0.03 } } }, "condition": { ... }, "input": { ... } } ... }

As you can see, the Substrait package is included as part of the @type when an Any is serialized.

However, I don't know of any existing use like this. In Acero we use google.protobuf.Any to add new nodes (currently as-of join and segmented aggregation). In both cases the new message is not part of the core Substrait set of messages. It's a part of Acero's custom protobuf. This custom protobuf uses Substrait messages (e.g. an as-of join has field references in it and these are Substrait expressions) but the only type name that gets serialized is the extension itself.

I think it's reasonable to expect that will never be the case. For example, consider the above example. One could argue that a new "standard hint" should be included in the hint definition as:

message RelCommon { ... Hint hint = 3; ... message Hint { ... EstimatedSelectivity estimated_selectivity = 11; substrait.extensions.AdvancedExtension advanced_extension = 10; ... } ... }

In other words, any time google.protobuf.Any is used it must refer to an object that is not part of the core Substrait protobuf files.

However, I'm not completely convinced it will never happen.

I believe what Weston is describing is how I planned to approach doing my own extensions. I think the most concise example is here, where I use ibis-substrait to translate and then I use a google.protobuf.Any for an ExtensionLeafRel:
https://github.com/drin/mohair-extension/blob/mainline/mohair_extension/extension.py#L100

Using just the standard string representation, here's what I generate:
https://media.githubusercontent.com/media/drin/mohair-extension/mainline/resources/projection.mohair.txt

an object that is not part of the core Substrait protobuf files

I am assuming that if something becomes core, then I would change the way this is done for that specific message type. Using ibis-substrait's approach to translation makes it an easy change, not sure that approach holds true for any other, large projects.

Thank you, both! The examples are super helpful. It's a good thing to note!

vibhatha · 2023-04-18T01:08:53Z

+## Generate protocol buffers
+Generate the protobuf files manually. Requires protobuf `v3.20.1`.
+```
+./gen_proto.sh


Just curious:

Could we make this part of the setup which would automatically run?

I don't think we want to do that -- one of the conveniences here should be that the files are already generated (using the correct version of protobuf) and bundled into the package for the end-user.

gforsyth

I think this is a good starting point and the general structure is good in re: protobuf generation, namespacing, etc.

My preference would be to call the module and the pypi package substrait instead of pysubstrait, but that's not a blocker.

danepitkin · 2023-04-19T15:26:54Z

I think this is a good starting point and the general structure is good in re: protobuf generation, namespacing, etc.

My preference would be to call the module and the pypi package substrait instead of pysubstrait, but that's not a blocker.

Since there are a lot of opinions on the two names, is it worth voting in the ML? It's probably best that we definitively pick one early on.

westonpace

Looks like a fine starting point to me.

westonpace · 2023-04-19T15:38:19Z

+# Getting Started
+```
+git clone --recursive https://github.com/substrait-io/substrait-python.git
+cd substrait-python
+```
+
+# Setting up your environment
+## Conda env
+Create a conda environment with developer dependencies.
+```
+conda env create -f environment.yml
+conda activate pysubstrait
+```
+
+# Build
+## Python package
+### Editable installation
+```
+pip install -e .
+```
+
+## Generate protocol buffers
+Generate the protobuf files manually. Requires protobuf `v3.20.1`.
+```
+./gen_proto.sh


Are these instructions for pysubstrait developers? Or are these instructions for pysubstrait users?

If these are for developers should they be in a CONTRIBUTING.md? If they are for users then why does it list gen_proto.sh? Is that something a user has to do?

Everything at and below "Getting started" is for devs! I'll move it to CONTRIBUTING.MD. Great idea!

westonpace · 2023-04-19T15:39:35Z

@@ -0,0 +1,10 @@
+def test_imports():
+    from pysubstrait.proto.pysubstrait.algebra_pb2 import Expression


Actually "standard" (as in, the message defining the hint is in the core substrait proto files) hints will probably be more of a danger than extensions.

drin

looks good to me.

vibhatha · 2023-04-20T03:46:43Z

Looks good to me.

gforsyth · 2023-04-20T13:51:00Z

Since there are a lot of opinions on the two names, is it worth voting in the ML? It's probably best that we definitively pick one early on.

I haven't familiarized myself with voting procedures for the project, but it's an option.

I think it's worth looking at what other languages have done:

Java (original implementation), refers to itself as io.substrait: https://mvnrepository.com/artifact/io.substrait
Rust, refers to itself as substrait: https://docs.rs/substrait/latest/substrait/
Go, refers to itself as substrait-go: https://pkg.go.dev/github.com/substrait-io/substrait-go
C++, uses the namespace io::substrait::common: https://github.com/substrait-io/substrait-cpp/blob/main/include/substrait/common/Exceptions.h#L9

All but go elide the language abbreviation from the name, and I believe go automatically removes go- and -go when referring to libraries internally (needs a fact check).

westonpace · 2023-04-20T14:10:45Z

C++, uses the namespace io::substrait::common: https://github.com/substrait-io/substrait-cpp/blob/main/include/substrait/common/Exceptions.h#L9

To be fair, the cmake project name (which is probably a closer equivalent) is actually substrait-cpp. That being said, I'm in favor of substrait over pysubstrait.

gforsyth · 2023-04-20T14:29:43Z

To be fair, the cmake project name (which is probably a closer equivalent) is actually substrait-cpp.

Sorry! I didn't mean to misrepresent 😅

danepitkin · 2023-04-20T16:14:04Z

Okay, I think there is a pretty substantial majority leaning towards substrait over pysubstrait. I will update this PR with that change!

I'll still nickname this project PySubstrait from time to time outside of this repo, though. ;)

danepitkin · 2023-04-20T16:30:00Z

@@ -0,0 +1,12 @@
+name: pysubstrait


How do we feel about leaving the conda environment as pysubstrait? Alternatively we can rename to the repo: substrait-python

I have minimal opinion since I don't know what it affects. Naively, it seems most straightforward for it to match the package name

Yeah, this you can leave alone. A person can always override it if they want to, and it won't impact the package name on conda-forge (which should probably be substrait-python to follow convention there)

danepitkin · 2023-04-20T16:31:17Z

@@ -0,0 +1,10 @@
+def test_imports():
+    from substrait.proto.pysubstrait.algebra_pb2 import Expression


What should the protobuf class namespace be in the package? pysubstrait? python? python.substrait? etc.

i think pysubstrait -> substrait. that matches ibis-substrait's naming scheme so I don't think it is problematic

I think if it's proto.substrait.algebra_pb2 that it can collide with the C descriptor pool.
what if we namespace it to proto and then move the whole thing up a directory, so you have substrait.proto.algebra_pb2?

I think that works with all the relative imports? But I could be wrong.

danepitkin · 2023-04-20T20:19:50Z

@@ -1,4 +0,0 @@
-try:


Hmm, this gets overwritten by ./gen_proto.sh when we don't use a subdirectory within the package.

hm, I was looking to see if we could move this somewhere, but I'm suddenly unsure why this is getting overwritten if it should be putting files into a proto directory.

I think I prefer to keep the generated files in substrait.proto.<namespace>

I guess it generates an empty init.py as a safety precaution? not ideal in this scenario haha

drin · 2023-04-20T20:25:45Z

@@ -0,0 +1 @@
+from . import proto


I was trying to see if we could move the version info to this .pyi file you have, but it seems like pyi files are only supposed to have type information. does it make sense to have this stub file if there's only this import?

I don't think so. It gets autogenerated from protol I believe. It used to generate .pyi files for all the classes, but stopped (maybe my version was updated locally mid-development?). Maybe we just add *.pyi to .gitignore.

drin · 2023-04-20T20:30:55Z

-tmp_dir=./proto
-dest_dir=./src/substrait/proto
+tmp_dir=./buf_work_dir
+dest_dir=./src/substrait


I think the hack-y way is to make this an intermediate dir and then rename it or put it in a different namespace (like pysubstrait) and then alias the imports via the __init__.py file?

edit: I suspect that this is overwriting the __init__.py because it by default creates the wrappers as a submodule into a directory it expects to be empty; maybe just modify the gen_proto.sh script to not generate an empty __init__.py file, since it wouldn't be necessary (you're not putting it in an empty directory)?

JK, the script just calls protol. Maybe remove the --create-package option and it will stop creating the empty __init__.py file

ooh ill give this a try!

It doesn't quite work as we'd like. --create-package will recursively create init files, which we do want (except at the top level). Changing --in-place to --not-in-place would disallow overwriting of files, which we don't quite want either. Ultimately the cleanest approach to me still feels like placing these files in a proto subdir.

Unless I'm misunderstanding you, @gforsyth 's suggestion is a proto subdir. I just assumed that the protol tool is creating an extra __init__.py file at the same level as the proto subdir.

I imagine the directory layout to be something like:

substrait ├── __init__.py (protol generates this by default) ├── proto (this is the proto subdir) ├── algebra_pb2.py ├── plan_pb2.py ├── ...

but that being said, I see you added a gen directory which is probably fine in the near-term. I'm not sure what's generating that __init__.py file

Yep your initial understanding is correct! IMO it would be better to generate files into a subdir if protol doesn't support exactly what we want. I'm hesitant to introduce any opaque behavior for devs to reduce bugs in the future. Having to choose between overwriting the top level __init__.py or not recursively generating __init__.py's seems bad. I'd rather have both if it means generating files into a subdir instead of root dir.

Oh and I forgot to mention that protol was deleting the top level __init__.py file when removing the --create-package option!

That was because I had --in-place option set (aka overwrite).

danepitkin · 2023-04-20T21:31:05Z

@@ -0,0 +1,11 @@
+def test_imports():
+    """Temporary sanity test"""
+    from substrait.gen.proto.algebra_pb2 import Expression


How do we feel about substrait.gen.proto?

danepitkin · 2023-04-20T21:41:21Z

I'm satisfied with this implementation if folks are as well. IMO it is about ready to merge!

vibhatha · 2023-04-21T00:54:46Z

I feel like now it is in a good shape. @gforsyth any thoughts?

gforsyth

This looks great @danepitkin ! Thanks for accommodating all of the change discussions.

gforsyth · 2023-04-21T12:29:15Z

I'll leave this open for the rest of today-ish in case anyone else wants to take a look, but I'll merge it in tonight.

danepitkin · 2023-04-21T15:38:57Z

Thanks everyone for all the feedback, it was a huge help! Looking forward to seeing how Substrait Python grows from here.

gforsyth · 2023-04-24T18:53:11Z

Thanks for leading the charge on this, @danepitkin !

Dane Pitkin added 3 commits April 17, 2023 16:32

feat: generated protobuf classes

a4211f8

Update README

ef8d56d

Update .gitattributes

1ace412

vibhatha reviewed Apr 18, 2023

View reviewed changes

vibhatha requested review from cpcloud, jacques-n, richtia and westonpace April 18, 2023 02:07

vibhatha assigned danepitkin Apr 18, 2023

vibhatha added documentation Improvements or additions to documentation enhancement New feature or request labels Apr 18, 2023

gforsyth reviewed Apr 19, 2023

View reviewed changes

westonpace approved these changes Apr 19, 2023

View reviewed changes

drin approved these changes Apr 19, 2023

View reviewed changes

Dane Pitkin added 4 commits April 19, 2023 14:13

Create CONTRIBUTING.md

dceaa23

Remove unnecessary whitespace

6b3b562

Update CONTRIBUTING.md

354c6e7

Update CONTRIBUTING.md again

e5cc85f

vibhatha approved these changes Apr 20, 2023

View reviewed changes

Rename package to substrait

e4f3040

danepitkin commented Apr 20, 2023

View reviewed changes

Dane Pitkin added 2 commits April 20, 2023 13:32

Update .gitattributes

1730a1d

Move proto files

5eb4f01

danepitkin commented Apr 20, 2023

View reviewed changes

drin reviewed Apr 20, 2023

View reviewed changes

Dane Pitkin added 2 commits April 20, 2023 17:25

Move protobuf to substrait.gen.proto

83d8ca2

Update .gitattributes

09596c4

danepitkin commented Apr 20, 2023

View reviewed changes

jorisvandenbossche approved these changes Apr 21, 2023

View reviewed changes

gforsyth approved these changes Apr 21, 2023

View reviewed changes

gforsyth merged commit 70a3cb8 into substrait-io:main Apr 24, 2023

		@@ -0,0 +1,10 @@
		def test_imports():
		from pysubstrait.proto.pysubstrait.algebra_pb2 import Expression

		@@ -0,0 +1,10 @@
		def test_imports():
		from substrait.proto.pysubstrait.algebra_pb2 import Expression

Conversation

danepitkin commented Apr 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danepitkin Apr 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drin Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gforsyth left a comment

Choose a reason for hiding this comment

Uh oh!

danepitkin commented Apr 19, 2023

Uh oh!

westonpace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drin left a comment

Choose a reason for hiding this comment

Uh oh!

vibhatha commented Apr 20, 2023

Uh oh!

gforsyth commented Apr 20, 2023

Uh oh!

westonpace commented Apr 20, 2023

Uh oh!

gforsyth commented Apr 20, 2023

Uh oh!

danepitkin commented Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drin Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gforsyth Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

danepitkin Apr 18, 2023 •

edited

Loading

drin Apr 19, 2023 •

edited

Loading

danepitkin commented Apr 20, 2023 •

edited

Loading

drin Apr 20, 2023 •

edited

Loading

gforsyth Apr 20, 2023 •

edited

Loading

drin Apr 20, 2023 •

edited

Loading

danepitkin Apr 20, 2023 •

edited

Loading

danepitkin Apr 20, 2023 •

edited

Loading