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

Support Python/C++ interoperability (native Python modules) #701

Closed
davidzchen opened this issue Dec 12, 2015 · 20 comments
Closed

Support Python/C++ interoperability (native Python modules) #701

davidzchen opened this issue Dec 12, 2015 · 20 comments
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Rules-Python Native rules for Python type: feature request

Comments

@davidzchen
Copy link
Member

According to the documentation for py_library.deps:

The list of other libraries to be linked in to the binary target. See general comments about deps at Attributes common to all build rules. These can be py_binary rules, py_library rules or cc_library rules,

However, when I try to build a py_library target that depends on a cc_library, I get the following error:

❯❯❯ bazel build //external:markupsafe
ERROR: /private/var/tmp/_bazel_dzc/a4c09a1fcefb8b21c5b0e7d7db6f08ba/external/markupsafe_archive/BUILD:9:12: in deps attribute of py_library rule @markupsafe_archive//:markupsafe: cc_library rule '@markupsafe_archive//:speedups' is misplaced here (expected py_binary or py_library).
ERROR: Analysis of target '//external:markupsafe' failed; build aborted.
INFO: Elapsed time: 0.213s

According to BazelPyRuleClasses.java, the only allowed rule types in py_library.deps is, in fact, py_binary and py_library:

  public static final String[] ALLOWED_RULES_IN_DEPS = new String[] {
      "py_binary",
      "py_library",
  };

What is the plan for Python-C interoperability? Should we update the documentation to remove the mention of depending on cc_library in the meantime?

The background for this is that I am trying to use Jinja for generating HTML for the Skylark docgen tool that I am working on. However, Jinja depends on Markupsafe, which is partly implemented in C.

@davidzchen
Copy link
Member Author

@ulfjack
Copy link
Contributor

ulfjack commented Jun 15, 2016

Recategorizing this as a feature request to support Python-C++ interop. I marked the other report that asked for that as a dupe.

@evanj
Copy link

evanj commented Aug 18, 2017

A workarounds I found: you can include pre-compiled .sos in the py_library's data attribute. No idea if you can somehow get the output of a cc_library to work in this way.

@benley
Copy link
Contributor

benley commented Aug 18, 2017

The trick to building a .so that works as a python extension seems to be making it a cc_binary instead of a cc_library. I think you have to build it with linkshared = 1 to make it work, but I'd love to be wrong about that.

@MarkusTeufelberger
Copy link
Contributor

I stumbled upon this one also recently. Please at least change the documentation or (ideally) increase the priority to get this fixed for good. Python libraries and binaries can and do depend on C libraries. This is currently not really easy to model in bazel.

In the meantime it would also be nice to have this workaround documented somewhere.

@buchgr
Copy link
Contributor

buchgr commented Sep 5, 2017

@MarkusTeufelberger we love contributions 😃

@ulfjack
Copy link
Contributor

ulfjack commented Sep 6, 2017

The problem is that we don't know how to fix it. We could export our internal model, but we're unsure if that's what people want. Our internal model is that all cc_library rules in deps of py_library rules get merged into a single .so at the py_binary level, together with all cc_library dependencies of py_extension rules, except the py_extension code itself, which gets compiled into a separate .so. It sounds awkward, and it is, but there doesn't seem to be any other principled approach to avoid ODR violations in the C++ code.

@ThomasColthurst
Copy link

+1 for exporting the internal model. My project is considering switching from bazel to Makefiles solely to avoid the ODR violations we are currently seeing when using bazel + clif + protobufs.

@hlopko
Copy link
Member

hlopko commented Jul 31, 2018

Work on this is blocked by #4570. #4570 is fixed, nothing should block this effort.

@brandjon
Copy link
Member

See related discussion in #1475.

@lberki lberki added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) and removed P3 We're not considering working on this, but happy to review a PR. (No assignee) labels Nov 18, 2020
@quval
Copy link
Contributor

quval commented Jan 11, 2021

I have a draft of a Starlark implementation of something like the internal Google model (all native deps are linked together into a statically-linked shared object; the modules themselves are dynamically linked to it). Would it be interesting to share this? Perhaps on rules_python?

@pvcnt
Copy link

pvcnt commented Jan 11, 2021

@quval It would certainly be interesting yes. :)

@quval
Copy link
Contributor

quval commented Jan 17, 2021

OK, here goes: https://github.com/quval/rules_native_python/
Got it to work on both Linux and Mac, but only with a toy setup, so I'd still consider this a draft. More importantly, I'm not sure I like the hack I did to work around the circular dependency (native deps -- compiled into native module -- linked against native deps). Any thoughts and suggestions are welcome.

(Edit: just pushed a revised version. Edit 2: seems to have some issues when using gold - will update again once resolved. Edit 3: seems to be all right now. Edit 4, 11/2022: just pushed a major update after the library got some real use.)

@mfarrugi
Copy link

mfarrugi commented May 26, 2021

The following implementation seems to work for me without issue, are there any deficiencies I'm overlooking?

# py_native_library.bzl

def py_native_library(name, **kwargs):
	native.cc_binary(
		name = name + ".so",
		linkshared = True,
		**kwargs
	)
	native.py_library(
		name = name,
		data = [name + ".so"],
	)

Edit:

# py_native_library.bzl

def py_native_library(name, **kwargs):
	native.cc_binary(
		name = name + ".so",
		linkshared = True,
                linkstatic = False,
		**kwargs
	)
	native.py_library(
		name = name,
		data = [name + ".so"],
	)

Is more correct, covering the common case where more than one native extension is loaded into a python executable (and so shared dependencies are properly dynamically linked, rather than statically linked into each extension).

@kkpattern
Copy link
Contributor

kkpattern commented Nov 12, 2021

The problem is that we don't know how to fix it. We could export our internal model, but we're unsure if that's what people want. Our internal model is that all cc_library rules in deps of py_library rules get merged into a single .so at the py_binary level, together with all cc_library dependencies of py_extension rules, except the py_extension code itself, which gets compiled into a separate .so. It sounds awkward, and it is, but there doesn't seem to be any other principled approach to avoid ODR violations in the C++ code.

Hi ulfjack, how does google handle multiple py_library with some common cc_library depends? For example, if I have two py_library both depends on some X cc_library. Initially we may use those two py_library seperately, but at some point people may use both of these two py_library. I think in this situation there is still ODR violations?

Riatre added a commit to Riatre/ray that referenced this issue Jul 12, 2022
Bazel macro py_test_module_list takes a `deps` argument, but completely
ignores it instead of passes it to `native.py_test`. Fixing that as we
are going to use deps of py_test_module_list in BUILD in later changes.

cpp/BUILD.bazel depends on the broken behaviour: it deps-on a cc_library
from a py_test, which isn't working, see upstream issue:
bazelbuild/bazel#701.
This is fixed by simply removing the (non-working) deps.
pcmoritz pushed a commit to ray-project/ray that referenced this issue Jul 13, 2022
See #23676 for context. This is another attempt at that as I figured out what's going wrong in `bazel test`. Supersedes #24828.

Now that there are Python 3.10 wheels for Ray 1.13 and this is no longer a blocker for supporting Python 3.10, I still want to make `bazel test //python/ray/tests/...` work for developing in a 3.10 env, and make it easier to add Python 3.10 tests to CI in future.

The change contains three commits with rather descriptive commit message, which I repeat here:

Pass deps to py_test in py_test_module_list

    Bazel macro py_test_module_list takes a `deps` argument, but completely
    ignores it instead of passes it to `native.py_test`. Fixing that as we
    are going to use deps of py_test_module_list in BUILD in later changes.

    cpp/BUILD.bazel depends on the broken behaviour: it deps-on a cc_library
    from a py_test, which isn't working, see upstream issue:
    bazelbuild/bazel#701.
    This is fixed by simply removing the (non-working) deps.

Depend on conftest and data files in Python tests BUILD files

    Bazel requires that all the files used in a test run should be
    represented in the transitive dependencies specified for the test
    target. For py_test, it means srcs, deps and data.

    Bazel enforces this constraint by creating a "runfiles" directory,
    symbolic links files in the dependency closure and run the test in the
    "runfiles" directory, so that the test shouldn't see files not in the
    dependency graph.

    Unfortunately, the constraint does not apply for a large number of
    Python tests, due to pytest (>=3.9.0, <6.0) resolving these symbolic
    links during test collection and effectively "breaks out" of the
    runfiles tree.

    pytest >= 6.0 introduces a breaking change and removed the symbolic link
    resolving behaviour, see pytest pull request
    pytest-dev/pytest#6523 for more context.

    Currently, we are underspecifying dependencies in a lot of BUILD files
    and thus blocking us from updating to newer pytest (for Python 3.10
    support). This change hopefully fixes all of them, and at least those in
    CI, by adding data or source dependencies (mostly for conftest.py-s)
    where needed.

Bump pytest version from 5.4.3 to 7.0.1

    We want at least pytest 6.2.5 for Python 3.10 support, but not past
    7.1.0 since it drops Python 3.6 support (which Ray still supports), thus
    the version constraint is set to <7.1.

    Updating pytest, combined with earlier BUILD fixes, changed the ground
    truth of a few error message based unit test, these tests are updated to
    reflect the change.

    There are also two small drive-by changes for making test_traceback and
    test_cli pass under Python 3.10. These are discovered while debugging CI
    failures (on earlier Python) with a Python 3.10 install locally.  Expect
    more such issues when adding Python 3.10 to CI.
Stefan-1313 pushed a commit to Stefan-1313/ray_mod that referenced this issue Aug 18, 2022
See ray-project#23676 for context. This is another attempt at that as I figured out what's going wrong in `bazel test`. Supersedes ray-project#24828.

Now that there are Python 3.10 wheels for Ray 1.13 and this is no longer a blocker for supporting Python 3.10, I still want to make `bazel test //python/ray/tests/...` work for developing in a 3.10 env, and make it easier to add Python 3.10 tests to CI in future.

The change contains three commits with rather descriptive commit message, which I repeat here:

Pass deps to py_test in py_test_module_list

    Bazel macro py_test_module_list takes a `deps` argument, but completely
    ignores it instead of passes it to `native.py_test`. Fixing that as we
    are going to use deps of py_test_module_list in BUILD in later changes.

    cpp/BUILD.bazel depends on the broken behaviour: it deps-on a cc_library
    from a py_test, which isn't working, see upstream issue:
    bazelbuild/bazel#701.
    This is fixed by simply removing the (non-working) deps.

Depend on conftest and data files in Python tests BUILD files

    Bazel requires that all the files used in a test run should be
    represented in the transitive dependencies specified for the test
    target. For py_test, it means srcs, deps and data.

    Bazel enforces this constraint by creating a "runfiles" directory,
    symbolic links files in the dependency closure and run the test in the
    "runfiles" directory, so that the test shouldn't see files not in the
    dependency graph.

    Unfortunately, the constraint does not apply for a large number of
    Python tests, due to pytest (>=3.9.0, <6.0) resolving these symbolic
    links during test collection and effectively "breaks out" of the
    runfiles tree.

    pytest >= 6.0 introduces a breaking change and removed the symbolic link
    resolving behaviour, see pytest pull request
    pytest-dev/pytest#6523 for more context.

    Currently, we are underspecifying dependencies in a lot of BUILD files
    and thus blocking us from updating to newer pytest (for Python 3.10
    support). This change hopefully fixes all of them, and at least those in
    CI, by adding data or source dependencies (mostly for conftest.py-s)
    where needed.

Bump pytest version from 5.4.3 to 7.0.1

    We want at least pytest 6.2.5 for Python 3.10 support, but not past
    7.1.0 since it drops Python 3.6 support (which Ray still supports), thus
    the version constraint is set to <7.1.

    Updating pytest, combined with earlier BUILD fixes, changed the ground
    truth of a few error message based unit test, these tests are updated to
    reflect the change.

    There are also two small drive-by changes for making test_traceback and
    test_cli pass under Python 3.10. These are discovered while debugging CI
    failures (on earlier Python) with a Python 3.10 install locally.  Expect
    more such issues when adding Python 3.10 to CI.

Signed-off-by: Stefan van der Kleij <[email protected]>
@vkresch
Copy link

vkresch commented Aug 18, 2022

@mfarrugi thanks for your example. This works when I am using pybind11. Meaning if I want to call C++ functions via python this works. But how about the other way around call python functions from C++. Can I do this then:

def cc_native_library(name, py_srcs, py_deps, cc_srcs, cc_deps, **kwargs):
	native.py_library(
		name = name + ".py",
                srcs = py_srcs,
                deps = py_deps,
		**kwargs
	)
	native.cc_library(
		name = name,
                srcs = cc_srcs,
                deps = cc_deps,
		data = [name + ".py"],
		linkshared = True,
                linkstatic = False,
                deps = [":python3"], # depend on python3 path location
	)

I want to use the cc_native_library for a cc_binary.
I tried this approach for pybind11 but the problem is here with the third party packages which do not get imported correctly since they are used in the data argument of cc_library. If using only builtin libraries this works. Any suggestions or examples? Thanks in advance!

Copy link

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Jan 13, 2024
Copy link

This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please post @bazelbuild/triage in a comment here and we'll take a look. Thanks!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 13, 2024
@jab
Copy link

jab commented Apr 13, 2024

@bazelbuild/triage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Rules-Python Native rules for Python type: feature request
Projects
None yet
Development

No branches or pull requests