This is an archive of the discontinued LLVM Phabricator instance.

Add Python bindings guide.
ClosedPublic

Authored by stellaraccident on Jul 9 2020, 5:35 PM.

Diff Detail

Event Timeline

stellaraccident created this revision.Jul 9 2020, 5:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2020, 5:35 PM
mehdi_amini added inline comments.Jul 9 2020, 8:12 PM
mlir/docs/Bindings/Python.md
34

Can this default to python3?

65

Nit: registerAllDialects is a free function and not a global initializer. There aren't any global initializers in mlir/lib

90

(same as above: there aren't any global init in MLIR core)

176

Can you add an example here?
Is there a doc about what is / isn't pythonic we can link to?

182

Can you make "doctest" a link to a doc about it? I don't assume everyone to be familiar with the python usual way of doing things.

Edit: actually just a link to the "Testing" section below.

301

Seems like something we don't want to repeat in every single file?

stellaraccident marked 9 inline comments as done.

Comments.

mlir/docs/Bindings/Python.md
34

Not reliably at present. This actually is already in the cmake cache due to other things auto-detecting it for lit and such. Also, what the python executable is called is very OS/install dependent. For example, many Linuxes differentiate 'python3' from 'python' (2), but default installs on Windows will typically only provide 'python' (3).

I foresee needing to rework the detection rules and make them specific to building the python bindings (versus conflating general access to the python interpreter with the version used for headers/libs, but that will require some surgery on various CMake files to get right (i.e. it is common in various multi-python build docker images to have a bunch of python installs and you want to generate shared objects for each version). As such, I see this current state as temporary and ok for now.

65

Rephrased: it is more about implying linkage of potentially optional components vs a concept of global constructor/initializer.

176

Added a blurb/link to pep8.

301

Added a TODO to create a test utility class once we have anything that exists.

mehdi_amini accepted this revision.Jul 9 2020, 8:38 PM
This revision is now accepted and ready to land.Jul 9 2020, 8:38 PM
This revision was automatically updated to reflect the committed changes.