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

"Cross compilation" of py_binary/py_image/py_library targets #260

Open
nimish opened this issue Nov 15, 2019 · 23 comments
Open

"Cross compilation" of py_binary/py_image/py_library targets #260

nimish opened this issue Nov 15, 2019 · 23 comments
Assignees

Comments

@nimish
Copy link

nimish commented Nov 15, 2019

Hi,

I have a py_binary that depends on a python pip library (grpcio) that has a native extension bundled in. This means that to create a linux container i'd need to have the pip_import rule download the manylinux wheel, not the host one (macos in my case).

Is there a way to force this? Otherwise py_image will happily just bundle up wheels with darwin native libs. py_binary will also only make host-runnable things.

@ali5h
Copy link

ali5h commented Dec 16, 2019

use https://github.com/ali5h/rules_pip/ and use pip_install(["--platform=linux_x86_64"])

@nimish
Copy link
Author

nimish commented Dec 16, 2019

@ali5h this works (thanks!) but is there integration into the bazel platform selection functions? I don't want separate targets for Linux and Mac.

E.g. Something that works with https://docs.bazel.build/versions/master/platforms.html

@ali5h
Copy link

ali5h commented Dec 16, 2019

you can define multiple piplib repos for different platforms and use select to pickup correct one

@nimish
Copy link
Author

nimish commented Dec 16, 2019

Is that documented and supported anywhere? That would be the ideal case, for bazel to automatically pick up the right pip repo for the right target platform.

E: should it not just be built in to the py_binary/py_library rule, to select the right target platform libs automatically?

@RemiTorracinta
Copy link

Would really, really like to see this as well.

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days.
Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_python!

@github-actions github-actions bot added the Can Close? Will close in 30 days if there is no new activity label Apr 14, 2021
@thundergolfer thundergolfer removed the Can Close? Will close in 30 days if there is no new activity label May 9, 2021
@thundergolfer thundergolfer self-assigned this May 9, 2021
@brduarte
Copy link

brduarte commented Aug 6, 2021

Help

@UebelAndre
Copy link
Contributor

Seems #510 and #531 and addressing the same issue? I'd love to have that functionality so excited to so folks interested enough to open PRs 😄

Hopefully @alexeagle and @meastham can combine forces to get the awesome functionality over the line! 🚀

@UebelAndre
Copy link
Contributor

Friendly ping to @alexeagle and @meastham, are either of you able to take another look at your pull requests? I'd love to have that functionality 😃

@UebelAndre
Copy link
Contributor

I wonder if pipenv could be used to facilitate generating cross platform dependency graphs. Does anyone have experience with the tool?

@meastham
Copy link

meastham commented Jan 9, 2022

Friendly ping to @alexeagle and @meastham, are either of you able to take another look at your pull requests? I'd love to have that functionality 😃

Sorry for the long delay on this! I did some investigation on some of the open questions, I'll see if we can find a path to getting this merged.

I don't think the PRs are exactly addressing the same issue. #531 allows having different requirements files for different platforms, but doesn't not allow downloading wheels for a different platform than the host platform. #510 allows downloading wheels for arbitrary platforms ("cross compiling"), but requires every platform to share one requirements file which has potential problems. Conceptually they could be compatible but the implementations would need to be modified to harmonize the way platform selection is done.

@jvolkman
Copy link
Contributor

I'm all for the wheel-only approach taken in #510. I don't want native compilation happening at all in the loading phase, as it can lead to hard-to-diagnose cache misses between machines (in my experience).

Thinking out loud here. If adding a wheel-only option, can we remove pip from the build-time process entirely? Instead of using pip download, why not just use bazel's built-in http tools to download the wheel? Instead of using pip-compile to generate a locked requirements.txt, write something that generates a .bzl file with compiled dependencies in a more bazel-centric fashion. Maybe resolvers from poetry or pipenv are used for this, which apparently support multi-platform resolve. Dependencies between libraries - including platform specific - could be explicit in the generated file and defined using bazel's own select or whatever.

I'm sure there are gotchas here (and significant work), but maybe detaching from pip could open up new avenues.

@alexeagle
Copy link
Collaborator

I think it should also be possible to compile python/C++ sources into wheels, however those need to happen in actions so they are debuggable and so that the target platform can be used.

@gopher-maker
Copy link

Ping on this @alexeagle and @meastham. #510 is exactly what I need today and having #531 would be a strong nice to have. Any progress here? Thanks!

@meastham
Copy link

Ping on this @alexeagle and @meastham. #510 is exactly what I need today and having #531 would be a strong nice to have. Any progress here? Thanks!

Hey @gopher-maker,

I'm just blocked on getting some guidance from a repository owner on how to resolve the outstanding issues with #510 (not a complaint btw, I'm sure everybody is quite busy!).

FWIW we've been using it for a fairly large Python codebase without significant problems for about 9 months now, so if you're feeling adventurous you can use it already. It looks like it now needs some non-trivial rebasing work; I'll see if I can get to that this week.

@adeeb10abbas
Copy link

Any leads on this yet? I am running into a similar issue trying to use Mujoco in a bazel workspace.

@alexeagle
Copy link
Collaborator

@f0rmiga has been working on something related to this at a client, I don't have any update sorry.

@f0rmiga
Copy link
Collaborator

f0rmiga commented Mar 16, 2023

There's a current effort that @philsc is writing in a doc, and has the collaboration from @jvolkman. I wrote a resolver to download Python packages using http_file. It's similar to how Gazelle does it for Go third-party deps. I'll start a draft PR in the coming days to have it maintained in rules_python. It will change quite a bit the current workflow to work with wheels in Bazel, and I don't have all the answers yet, so I don't foresee it being in a release soon.

github-merge-queue bot pushed a commit that referenced this issue Aug 24, 2024
…when build sdist (#2126)

Building sdist results in `Could not find a version that satisfies the
requirement setuptool` this regressed when a fix in parameter handling
got introduced in #2091.

Before this change the building from sdist when using
`experimental_index_url`
would break because `--no-index` is passed to `pip`. This means that
`pip`
would fail to locate build time dependencies needed for the packages and
would
just not work. In `whl_library` we setup `PYTHONPATH` to have some build
dependencies available (like `setuptools`) and we could use them during
building from `sdist` and to do so we need to add `--no-build-isolation`
flag.
However, for some cases we need to also add other build-time
dependencies (e.g.
`flit_core`) so that the building of the wheel in the `repository_rule`
context
is successfuly. Removing `--no-index` allows `pip` to silently fetch the
needed
build dependencies from PyPI if they are missing and continue with the
build.

This is not a perfect solution, but it does unblock users to use the
`sdist`
distributions with the experimental feature enabled by using
`experimental_index_url` (see #260 for tracking of the completion).

Fixes #2118
Fixes #2152

---------

Co-authored-by: aignas <[email protected]>
github-merge-queue bot pushed a commit that referenced this issue Oct 7, 2024
With this change we set the default value of `--python_version` when
the `python.toolchain` is used in `bzlmod` and we generate the
appropriate config settings based on the registered toolchains and
given overrides by the root module.

This means that we expect the `--python_version` to be always set to
a non-empty value under `bzlmod` and we can cleanup code which was
handling `//conditions:default` case. This also means that we can
in theory drop the requirement for `python_version` in `pip.parse`
and just setup dependencies for all packages that we find in the
`requirements.txt` file and move on. This is left as future work
by myself or anyone willing to contribute. We can also start reusing
the same `whl_library` instance for multi-platform packages because
the python version flag is always set - this will simplify the layout
and makes the feature non-experimental anymore under bzlmod.

Summary:
* Add `@pythons_hub` to the `WORKSPACE` with dummy data to make
  pythons_hub work.
* Add `MINOR_MAPPING` and `PYTHON_VERSIONS` to pythons_hub to
  generate the config settings.
* Remove handling of the default version in `pypi` code under bzlmod.

Work towards #2081, #260, #1708

---------

Co-authored-by: Richard Levasseur <[email protected]>
aignas added a commit to aignas/rules_python that referenced this issue Oct 11, 2024
With this change we can support ibazel for building our docs again
because we will just not have any sdists that are causing issues. This
limits the scope to only supporting whls at this time, but for the
time being it is the best solution.

Fixes bazelbuild#2223
Work towards bazelbuild#260
github-merge-queue bot pushed a commit that referenced this issue Oct 11, 2024
With this change we can support ibazel for building our docs again
because we will just not have any sdists that are causing issues. This
limits the scope to only supporting whls at this time, but for the
time being it is the best solution.

Fixes #2223
Work towards #260
aignas added a commit to aignas/rules_python that referenced this issue Nov 2, 2024
Before this change the `extra_pip_args` would not be passed down the
chain if `experimental_index_url` is set. This adds a test and fixes the
bug.

Work towards bazelbuild#260.
github-merge-queue bot pushed a commit that referenced this issue Nov 3, 2024
Before this change the `extra_pip_args` would not be passed down the
chain if `experimental_index_url` is set. This adds a test and fixes the
bug.

Work towards #260
github-merge-queue bot pushed a commit that referenced this issue Nov 5, 2024
…repos (#2369)

Before this change, it was impossible for users to use the targets
created with `additive_build_content` whl annotation unless they relied
on the implementation detail of the naming of the spoke repositories and
had `use_repo` statements in their `MODULE.bazel` files importing the
spoke repos.

With #2325 in the works, users will have to change their `use_repo`
statements, which is going to be disruptive. In order to offer them an
alternative for not relying on the names of the spokes, there has to be
a way to expose the extra targets created and this PR implements a
method. Incidentally, the same would have happened if we wanted to
stabilize the #260 work and mark `experimental_index_url` as
non-experimental anymore.

I was hoping we could autodetect them by parsing the build content
ourselves in the `pip` extension, but it turned out to be extremely
tricky and I figured that it was better to have an API rather than not
have it.

Whilst at it, also relax the naming requirements for the
`whl_modifications` attribute.

Fixes #2187
aignas added a commit to aignas/rules_python that referenced this issue Nov 7, 2024
…azelbuild#2325)

With this change we finally generate the same lock file within the
legacy code `pip.parse` code path and it allows to slowly transition to
using the new code path as much as possible without user doing anything.

This moves the selection of the host-compatible lock file from the
extension evaluation to the build phase - note, we will generate extra
repositories here which might not work on the host platform, however, if
the users are consuming the `whl_library` repos through the hub repo
only, then everything should work. A known issue is that it may break
`bazel query` and in these usecases it is advisable to use `cquery`
until we have `sdist` cross-building from source fully working.

Summary:
- feat: reuse the `render_pkg_aliases` for when filename is not known
  but platform is known
- feat: support generating the extra config settings required
- feat: `get_whl_flag_versions` now generates extra args for the rules
- feat: make lock file generation the same irrespective of the host
  platform
- test: add an extra test with multiple requirements files
- feat: support cross-platform builds using `download_only = True` in
  legacy setups

Note, that users depending on the naming of the whl libraries will need
to start using `extra_hub_aliases` attribute instead to keep their
setups not relying on this implementation detail.

Fixes bazelbuild#2268
Work towards bazelbuild#260

---------

Co-authored-by: Richard Levasseur <[email protected]>
github-merge-queue bot pushed a commit that referenced this issue Nov 13, 2024
This just cleans up the code and moves more logic from the
repository_rule
(i.e. generation of `BUILD.bazel` files) to loading time (macro
evaluation).
This makes the unit testing easier and I plan to also move the code that
is
generating config setting names from filenames to this new macro, but
wanted to
submit this PR to reduce the review chunks.

Summary:
- Add a new `pkg_aliases` macro.
- Move logic and tests for creating WORKSPACE aliases.
- Move logic and tests bzlmod aliases.
- Move logic and tests bzlmod aliases with groups.
- Add a test for extra alias creation.
- Use `whl_alias` in `pypi` extension integration tests.
- Improve the serialization of `whl_alias` for passing to the pypi hub
repo.

Related to #260, #2386, #2337, #2319 - hopefully cleaning the code up
will make
it easier to address those feature requests later.

---------

Co-authored-by: Richard Levasseur <[email protected]>
github-merge-queue bot pushed a commit that referenced this issue Dec 24, 2024
The warning is somewhat non-actionable and the sources can be
inspected via the MODULE.bazel.lock file if needed. This makes it
easier to make this option a default at some point.

At the same time cleanup the code since we are not using the
`get_index_urls` to print the warning.

Work towards #260
aignas added a commit to aignas/rules_python that referenced this issue Dec 25, 2024
- Move the `whl_library` creation into a separate function and remove
  the `TODO` note.
- Move the creation of the `get_index_urls` functions into outer
  `parse_modules` function and simplify the reproducible extension
  setting logic.
- Remove the `prefix` parameter from the `*repo_name` functions.
- Add an extra error message, for ensuring that invariants are met.

Work towards bazelbuild#260
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests