Skip to content

Conversation

oxidase
Copy link
Contributor

@oxidase oxidase commented Dec 22, 2022

Fixes #930 the following errors with bzlmod modules:

ERROR: An error occurred during the fetch of repository 'rules_python~0.16.1~pip~pip':
   Traceback (most recent call last):
	File "/home/miha/.cache/bazel/_bazel_miha/bd93af10788cff1331d75ed739998a3c/external/rules_python~0.16.1/python/pip_install/pip_repository.bzl", line 335, column 63, in _pip_repository_impl
		environment = _create_repository_execution_environment(rctx),
	File "/home/miha/.cache/bazel/_bazel_miha/bd93af10788cff1331d75ed739998a3c/external/rules_python~0.16.1/python/pip_install/pip_repository.bzl", line 210, column 47, in _create_repository_execution_environment
		cppflags.extend(_get_toolchain_unix_cflags(rctx))
	File "/home/miha/.cache/bazel/_bazel_miha/bd93af10788cff1331d75ed739998a3c/external/rules_python~0.16.1/python/pip_install/pip_repository.bzl", line 115, column 37, in _get_toolchain_unix_cflags
		if not is_standalone_interpreter(rctx, rctx.attr.python_interpreter_target):
	File "/home/miha/.cache/bazel/_bazel_miha/bd93af10788cff1331d75ed739998a3c/external/rules_python~0.16.1/python/repositories.bzl", line 64, column 22, in is_standalone_interpreter
		rctx.path(Label("@{}//:WORKSPACE".format(rctx.attr.python_interpreter_target.workspace_name))).dirname,
Error in path: Unable to load package for @[unknown repo 'rules_python~0.16.1~python~python3_10_x86_64-unknown-linux-gnu' requested from @rules_python~0.16.1]//:WORKSPACE: The repository '@[unknown repo 'rules_python~0.16.1~python~python3_10_x86_64-unknown-linux-gnu' requested from @rules_python~0.16.1]' could not be resolved: No repository visible as '@rules_python~0.16.1~python~python3_10_x86_64-unknown-linux-gnu' from repository '@rules_python~0.16.1'

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

The PR correctly references python interpreter target labels as @@rules_python~0.16.1~python~python3_10_x86_64-unknown-linux-gnu//:WORKSPACE and allows to use

use_repo(python, "python3_10_x86_64-unknown-linux-gnu")

pip.parse(
    name = "pip",
    requirements_lock = "//:requirements.txt",
    python_interpreter_target = "@python3_10_x86_64-unknown-linux-gnu//:bin/python3",
)


## Does this PR introduce a breaking change?

- [ ] Yes
- [X] No


<!-- If this PR contains a breaking change, please describe the impact and migration path for existing applications below. -->


## Other information

@oxidase oxidase force-pushed the fix/python_interpreter_target branch 2 times, most recently from 829206c to ba30b71 Compare December 23, 2022 22:22
Copy link
Member

@f0rmiga f0rmiga left a comment

Choose a reason for hiding this comment

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

Could you update the bzlmod example to include this, please?

@oxidase oxidase force-pushed the fix/python_interpreter_target branch from 6951d96 to a4be55b Compare December 24, 2022 12:58
@oxidase oxidase requested a review from rickeylev as a code owner December 24, 2022 12:58
@oxidase oxidase force-pushed the fix/python_interpreter_target branch from a4be55b to 5d42fb6 Compare December 24, 2022 13:00
@oxidase
Copy link
Contributor Author

oxidase commented Dec 24, 2022

@f0rmiga I pushed a WIP commit with the updated example but it won't pass CI as there is no way to get execution platform at this stage. It would be also wrong to build wheels using an execution platform unless it is guaranteed that it is similar to target one.

With the following code

def register_pip_repositories(toolchains):
    for toolchain in toolchains:
        pip_name = "pip_%s" % toolchain
        print (pip_name)
        use_repo(python, toolchain)
        pip.parse(
            name = pip_name,
            requirements_lock = "//:requirements_lock.txt",
            requirements_windows = "//:requirements_windows.txt",
            python_interpreter_target = "@%s//:bin/python3" % toolchain,
        )
        use_repo(pip, pip_name)

register_pip_repositories([
    "python3_9_x86_64-apple-darwin",
    "python3_9_x86_64-unknown-linux-gnu"
])

I stuck with a problem how to conditionally load one of

load("@pip_python3_9_x86_64-apple-darwin//:requirements.bzl", "requirement")
load("@pip_python3_9_x86_64-unknown-linux-gnu//:requirements.bzl", "requirement")

Would it be ok to not update the example?

Frankly, python_interpreter_target attribute is a hack that can not be fixed without changing whl_library from a repository rule to a rule where a resolved python toolchain can be used.

@acr92
Copy link
Contributor

acr92 commented Dec 30, 2022

Seems to fail on Windows and OS X for the example, because of the hardcoded dependency on Linux.

Any neat way to handle this? I dug through the code and saw that for the pre-bzlmod system there was a "interpreter" field exposed.

Would be fantastic to get this PR fixed up and merged.

@groodt
Copy link
Collaborator

groodt commented Dec 30, 2022

To correctly use toolchains for the external PyPI dependencies, I believe we need to do the following:

  • Move away from the overuse of repository rules in the current implementation
  • Convert the requirement helpers into simpler macros without a reference to the repositories
  • Require bzl lockfiles for all target platforms

Would folks be happy if we moved to a "binary only" / "download only" implementation for bzlmod? It's fairly straightforward to migrate whl_library away from a repository rule if we don't need to use pip to build wheels.

ERROR: An error occurred during the fetch of repository 'rules_python~0.16.1~pip~pip':
   Traceback (most recent call last):
	File "/home/miha/.cache/bazel/_bazel_miha/bd93af10788cff1331d75ed739998a3c/external/rules_python~0.16.1/python/pip_install/pip_repository.bzl", line 335, column 63, in _pip_repository_impl
		environment = _create_repository_execution_environment(rctx),
	File "/home/miha/.cache/bazel/_bazel_miha/bd93af10788cff1331d75ed739998a3c/external/rules_python~0.16.1/python/pip_install/pip_repository.bzl", line 210, column 47, in _create_repository_execution_environment
		cppflags.extend(_get_toolchain_unix_cflags(rctx))
	File "/home/miha/.cache/bazel/_bazel_miha/bd93af10788cff1331d75ed739998a3c/external/rules_python~0.16.1/python/pip_install/pip_repository.bzl", line 115, column 37, in _get_toolchain_unix_cflags
		if not is_standalone_interpreter(rctx, rctx.attr.python_interpreter_target):
	File "/home/miha/.cache/bazel/_bazel_miha/bd93af10788cff1331d75ed739998a3c/external/rules_python~0.16.1/python/repositories.bzl", line 64, column 22, in is_standalone_interpreter
		rctx.path(Label("@{}//:WORKSPACE".format(rctx.attr.python_interpreter_target.workspace_name))).dirname,
Error in path: Unable to load package for @[unknown repo 'rules_python~0.16.1~python~python3_10_x86_64-unknown-linux-gnu' requested from @rules_python~0.16.1]//:WORKSPACE: The repository '@[unknown repo 'rules_python~0.16.1~python~python3_10_x86_64-unknown-linux-gnu' requested from @rules_python~0.16.1]' could not be resolved: No repository visible as '@rules_python~0.16.1~python~python3_10_x86_64-unknown-linux-gnu' from repository '@rules_python~0.16.1'
@oxidase oxidase force-pushed the fix/python_interpreter_target branch from 5d42fb6 to f70e0d2 Compare January 2, 2023 10:00
@oxidase
Copy link
Contributor Author

oxidase commented Jan 2, 2023

@acr92

Seems to fail on Windows and OS X for the example, because of the hardcoded dependency on Linux.

I have removed WIP commit, so the PR is unblocked.

Any neat way to handle this?

I don't see a way to correctly solve the issue. The repository_rule context has no resolved toolchains and the rule context does not allow a variable list of outputs that can be produced by "toolchain/python setup.py ...".

I dug through the code and saw that for the pre-bzlmod system there was a "interpreter" field exposed.

python_interpreter is a string attribute to reference an execution platform python and python_interpreter_target is a label attribute to reference a single external repository python runtime. The former attribute is non-hermetic and fails if an execution platform python runtime does not match toolchain one. The latter attribute is a workaround that fails in a case of multiple registered python toolchains.

@groodt

if we don't need to use pip to build wheels.

This is exactly my use case that requires building wheels for the following requirements.txt

torch==1.13.0+cu117

mmcv==2.0.0rc3
mmcls==1.0.0rc4
mmsegmentation==1.0.0rc1

as mmcv setup.py script checks if torch is installed to build the extensions module. My use case also requires #942 as dependencies have to be installed before mmcv:

pip.parse(
    name = "pip-mmcv",
    requirements_lock = "//:requirements_mmcv.txt",
    python_interpreter_target = "@python3_10_x86_64-unknown-linux-gnu//:bin/python3",
    extra_pip_args = [
        "--verbose",
    ],
    environment = {
        "MAX_JOBS": "12",
        "SETUPTOOLS_USE_DISTUTILS": "stdlib",
    },
    quiet = False,
    deps = [
        "@pip_torch//:site-packages/__init__.py",
        "@pip_typing_extensions//:site-packages/__init__.py",
    ],
    timeout = 10000,
)

Using the "download only" implementation will still require building mmcv with some torch version outside the bazel environment.

@oxidase oxidase requested review from f0rmiga and removed request for hrfuller and rickeylev January 2, 2023 12:39
Copy link
Member

@f0rmiga f0rmiga left a comment

Choose a reason for hiding this comment

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

Even though there are other solutions to the problem we envision, this PR solves the immediate issue while not going in the wrong direction.

@f0rmiga f0rmiga merged commit 5f166c1 into bazel-contrib:main Jan 23, 2023
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

Successfully merging this pull request may close these issues.

pip.parse extension does not allow setting the hermetic python interpreter

4 participants