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

python calling rust example #75

Closed
wants to merge 6 commits into from

Conversation

mfarrugi
Copy link
Collaborator

This is not well supported by python rules, but sorta works. bazelbuild/bazel#1475 outlines the hacky setup.

Might also want to create a pyo3 or similar example outside of this repo.

@acmcarther
Copy link
Collaborator

I can't unblock this for CI. Could I bother you to push a commit (perhaps removing "debug stuff", unless you think you'll need it to debug OSX)?

I think the ci pipeline forgot about this verification request and the link is now 404ing.

@mfarrugi
Copy link
Collaborator Author

mfarrugi commented Mar 7, 2018

I don't think it's right referring to the mangled solib path, but don't have a good idea of what The Right Way is yet. You just need any commit to re-trigger CI?

@acmcarther
Copy link
Collaborator

Oops, didn't submit my comment!

Yeah I just need a commit so that the CI sees it.

import os
from ctypes import cdll

# TODO: This is a hack to compensate for rootpath not giving us the _solib_* path...
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I cannot quite put my finger on how hacky this is.. let me know what ya'll think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wow yeah this is pretty scary.

I can't tell at a glance -- is this brittle w.r.t. where generated artifacts land? I don't much care either way since this is an example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This ultimately searches all of runfiles, so unless the design of collecting shared libraries in one location changes dramatically, it should be safe. That said, searching should really not be necessary and I'm not sure if it's a deficiency in python/c++ rules or my lack of knowledge here.

@mfarrugi mfarrugi changed the title [wip] *rough* draft of python calling rust python calling rust example Dec 11, 2018
Copy link
Collaborator

@acmcarther acmcarther left a comment

Choose a reason for hiding this comment

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

I've totally context switched from this, so I'm going to approach this in a really sterile way:

Including an example of something like this seems to me to be equivalent to suggesting that this is a good approach to do Py -> Rust FFI, and that we'll support such a thing into the future. Now, "support" may not be a real obligation on our part at all -- is it?

Also, if this is something you're interested in (personally or as an organization) can you informally commit to updating it if you discover a better/more "production-y" way to do it? I have vague nebulous concerns about stale examples.

#[no_mangle]
pub extern fn triple_it(x: i32) -> i32 {
x * 3
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit newline

@@ -0,0 +1,61 @@
# TODO: Native extensions are what we really want, cdll.LoadLibrary is not particularly great.
Copy link
Collaborator

@acmcarther acmcarther Dec 11, 2018

Choose a reason for hiding this comment

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

Maybe amend this file name with "example" so that it isn't viewed as some hidden functionality? Unsure what users or readers would think.

I suspect that anyone interested in this functionality would copy+paste this code into their workspace -- ideally the code in condition enough for users who would do that. Judgement call for you.

crate_name = rust_lib.split(":")[-1]
out = name + ".py"
native.genrule(
name = "__generate" + out,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit of nits: "generated"

import os
from ctypes import cdll

# TODO: This is a hack to compensate for rootpath not giving us the _solib_* path...
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wow yeah this is pretty scary.

I can't tell at a glance -- is this brittle w.r.t. where generated artifacts land? I don't much care either way since this is an example.

@mfarrugi
Copy link
Collaborator Author

I did this mostly to see how difficult / complicated setting up the integration was, I think it's reasonably straightforward (and would be dead simple if the $(rootpath) gave the path we wanted).

I think the best case scenario for this sort of integration is for python to have the capacity to take a header and a shared library and generate all of the boilerplate, and for rust to be able to produce the shared library and header (with cbindgen). IIUC this is roughly how milksnake works, as opposed to directly creating a python native extension. The API would look roughly the same as the current example, however would work for functions that aren't i32 in and i32 out.

In general I don't have an immediate use for this, but I think it's nice to say "look, it's easy to use rust with xxx". It is okay with me for this to continue to sit in a pull request (it'll be a year old soon!), but I don't mind providing a similar level of guidance / support as with the rest of the rules.

@aiuto
Copy link

aiuto commented Aug 18, 2019

I would rather not have it sit in a PR forever. if it works and we can put it in CI, then you should dust it off to commit it. If it doesn't really work, how about turning it into examples/ffi/python_calling_rust/readme.md. There you can describe the approach and how a more disciplined solution might work. A PR is essentially undiscoverable.

@mfarrugi
Copy link
Collaborator Author

Will close this for now, should be just as easy to find.

Would make sense to revisit when bazel-python has support for native extensions.

@mfarrugi mfarrugi closed this Aug 18, 2019
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 this pull request may close these issues.

4 participants