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

incompatible_use_python_toolchains: The Python runtime is obtained from a toolchain rather than a flag #7899

Open
brandjon opened this issue Mar 29, 2019 · 24 comments

Comments

Projects
None yet
6 participants
@brandjon
Copy link
Member

commented Mar 29, 2019

Flag: --incompatible_use_python_toolchains
Available since: 0.25
Will be flipped in: 0.27
Feature tracking issue: #7375

<==== If your targets are running under the wrong Python version... ====>

A common source of migration problems occurs when a py_binary needs Python 2, but incorrectly declares itself (possibly by default) as having python_version = "PY3". Previously, bug #4815 could mask this problem by allowing the target to still run under Python 2. But flipping this flag fixes that bug and reveals the underlying version mismatch. (The same scenario can also happen in reverse, with a target that needs Python 3 but declares itself as python_version = "PY2".)

To fix this, add python_version = "PY2" (or "PY3") to your target. If your target is used in the host configuration, you may need to globally set --host_force_python=PY2 for your build (see #6443) until the host configuration can be replaced by the execution configuration.

<==== If you're seeing problems with the default Python toolchain... ====>

Specifically, an error like this:

Error: The default python toolchain (@bazel_tools//tools/python:autodetecting_toolchain) was unable to locate a suitable Python interpreter on the target platform at execution time. Please register an appropriate Python toolchain. [...]
Failure reason: Cannot locate 'python3' or 'python' on the target platform's PATH, which is: [...]

Determine whether you have python2, python3, and/or python on your PATH. If you can't add them or it seems like that's not working, try defining and registering your own Python toolchain as descrived below. On Mac systems, see #7414: The autodetecting toolchain experienced a bug for mac that's fixed as of Bazel 0.27.

Motivation

For background on toolchains, see here.

Previously, the Python runtime (i.e., the interpreter used to execute py_binary and py_test targets) could only be controlled globally, and required passing flags like --python_top to the bazel invocation. This is out-of-step with our ambitions for flagless builds and remote-execution-friendly toolchains. Using the toolchain mechanism means that each Python target can automatically select an appropriate runtime based on what target platform it is being built for.

Change

Enabling this flag triggers the following changes.

  1. Executable Python targets will retrieve their runtime from the new Python toolchain.

  2. It is forbidden to set any of the legacy flags --python_top, --python2_path, or --python3_path. Note that the last two of those are already no-ops. It is also strongly discouraged to set --python_path, but this flag will be removed in a later cleanup due to #7901.

  3. The python_version attribute of the py_runtime rule becomes mandatory. It must be either "PY2" or "PY3", indicating which kind of runtime it is describing.

For builds that rely on a Python interpreter installed on the system, it is recommended that users (or platform rule authors) ensure that each platform has an appropriate Python toolchain definition.

If no Python toolchain is explicitly registered, on non-Windows platforms there is a new default toolchain that automatically detects and executes an interpreter (of the appropriate version) from PATH. This resolves longstanding issue #4815. A Windows version of this toolchain will come later (#7844).

Migration

If you were relying on --python_top, and you want your whole build to continue to use the py_runtime you were pointing it to, you just need to follow the steps below to define a py_runtime_pair and toolchain, and register this toolchain in your workspace. So long as you don't add any platform constraints that would prevent your toolchain from matching, it will take precedence over the default toolchain described above.

If you were relying on --python_path, and you want your whole build to use the interpreter located at the absolute path you were passing in this flag, the steps are the same, except you also have to define a new py_runtime with the interpreter_path attribute set to that path.

Otherwise, if you were only relying on the default behavior that resolved python from PATH, just enjoy the new default behavior, which is:

  1. First try python2 or python3 (depending on the target's version)
  2. Then fall back on python if not found
  3. Fail-fast if the interpreter that is found doesn't match the target's major Python version (PY2 or PY3), as per the python -V flag.

On Windows the default behavior is currently unchanged (#7844).

Example toolchain definition:

# In your BUILD file...
load("@bazel_tools//tools/python/toolchain.bzl", "py_runtime_pair")
py_runtime(
    name = "my_py2_runtime",
    interpreter_path = "/system/python2",
    python_version = "PY2",
)
py_runtime(
    name = "my_py3_runtime",
    interpreter_path = "/system/python3",
    python_version = "PY3",
)
py_runtime_pair(
    name = "my_py_runtime_pair",
    py2_runtime = ":my_py2_runtime",
    py3_runtime = ":my_py3_runtime",
)
toolchain(
    name = "my_toolchain",
    target_compatible_with = [...],  # optional platform constraints
    toolchain = ":my_py_runtime_pair",
    toolchain_type = "@bazel_tools//tools/python:toolchain_type",
)
# In your WORKSPACE...
register_toolchains("//my_pkg:my_toolchain")

Of course, you can define and register many different toolchains and use platform constraints to restrict them to appropriate target platforms. It is recommended to use the constraint settings @bazel_tools//tools/python:py2_interpreter_path and [...]:py3_interpreter_path as the namespaces for constraints about where a platform's Python interpreters are located.

The new toolchain-related rules and default toolchain are implemented in Starlark under @bazel_tools. Their source code and documentation strings can be read here.

@brandjon brandjon self-assigned this Mar 29, 2019

bazel-io pushed a commit that referenced this issue Mar 30, 2019

Introduce --incompatible_use_python_toolchains
This renames --experimental_use_python_toolchains to --incompatible. It also adds the behavior to the flag that

  1) py_runtime's python_version attribute becomes manadatory, and

  2) the --python_top flag is disallowed (it is superseded by toolchains), as well as --python2_path and --python3_path (they were already no-ops). --python_path is strongly discouraged but not yet banned due to an existing use on Windows (#7901).

Feature issue is #7375, incompatible change migration issue is #7899.

RELNOTES[INC]: Introduced --incompatible_use_python_toolchains, which supersedes --python_top/--python_path. See #7899 and #7375 for more information.

PiperOrigin-RevId: 241134532
@nlopezgi

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

I'm trying this flag for rules_k8s in the context of bazelbuild/rules_k8s#305.
If I run a build with an explicit --python_top my build / test succeeds (bazelbuild/rules_k8s#306)
However if I use the new --incompatible_use_python_toolchains I get the following error:

ERROR: .../src/rules_k8s/fork/k8s/BUILD:58:1: Building par file //k8s:resolver.par failed (Exit 1) compiler.par failed: error executing command bazel-out/host/bin/external/subpar/compiler/compiler.par --manifest_file bazel-out/k8-fastbuild/bin/k8s/resolver.par_SOURCES --outputpar bazel-out/k8-fastbuild/bin/k8s/resolver.par --stub_file ... (remaining 4 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
src/main/tools/linux-sandbox-pid1.cc:427: "execvp(bazel-out/host/bin/external/subpar/compiler/compiler.par, 0x1830290)": No such file or directory

Note I did not experience this type of error when trying this flag out with rules_docker (bazelbuild/rules_docker#787)

See also bazelbuild/rules_k8s#305 for context

emusand added a commit to emusand/bazel that referenced this issue Apr 16, 2019

Introduce --incompatible_use_python_toolchains
This renames --experimental_use_python_toolchains to --incompatible. It also adds the behavior to the flag that

  1) py_runtime's python_version attribute becomes manadatory, and

  2) the --python_top flag is disallowed (it is superseded by toolchains), as well as --python2_path and --python3_path (they were already no-ops). --python_path is strongly discouraged but not yet banned due to an existing use on Windows (bazelbuild#7901).

Feature issue is bazelbuild#7375, incompatible change migration issue is bazelbuild#7899.

RELNOTES[INC]: Introduced --incompatible_use_python_toolchains, which supersedes --python_top/--python_path. See bazelbuild#7899 and bazelbuild#7375 for more information.

PiperOrigin-RevId: 241134532

bazel-io pushed a commit that referenced this issue May 1, 2019

Release 0.25.0 (2019-05-01)
Baseline: 0366246

Cherry picks:

   + 3f7f255:
     Windows: fix native test wrapper's arg. escaping
   + afeb8d0:
     Flip --incompatible_windows_escape_jvm_flags
   + 4299b65:
     Sort DirectoryNode children to ensure validity.
   + 231270c:
     Conditionally use deprecated signature for initWithContentsOfURL
   + 75a3a53:
     Add http_archive entries for testing with various JDK versions.
   + 4a6354a:
     Now that ubuntu1804 uses JDK 11, remove explicit
     ubuntu1804_java11 tests.
   + ae102fb:
     Fix wrong name of ubuntu1804_javabase9 task.
   + 0020a97:
     Remove @executable_path/Frameworks from rpaths
   + 130f86d:
     Download stderr/stdout to a temporary FileOutErr

Incompatible changes:

  - (Starlark rules) The legacy "py" provider can no longer be passed
    to or produced by native Python rules; use
    [PyInfo](https://docs.bazel.build/versions/master/skylark/lib/PyIn
    fo.html) instead. See
    [#7298](#7298) for more
    information.
  - (Python rules) The `default_python_version` attribute of the
    `py_binary` and `py_test` rules has been renamed to
    `python_version`. Also, the `--force_python` flag has been
    renamed to `--python_version`. See
    [#7308](#7308) for more
    information.
  - (Python rules) The python version now changes to whatever version
    is specified in a `py_binary` or `py_test`'s `python_version`
    attribute, instead of being forced to the value set by a command
    line flag. You can temporarily revert this change with
    `--incompatible_allow_python_version_transitions=false`. See
    [#7307](#7307) for more
    information.
  - --incompatible_disable_third_party_license_checking` is enabled
    by default
  - Introduced --incompatible_use_python_toolchains, which supersedes
    --python_top/--python_path. See #7899 and #7375 for more
    information.
  - Python 3 is now the default Python version (for `py_binary` and
    `py_test` targets that don't specify the `python_version`
    attribute). Targets that are built for Python 3 will no longer
    have their output put in a separate `-py3` directory; instead
    there is now a separate `-py2` directory for Python 2 targets.
    See #7359 and #7593 for more information.
  - objc_library resource attributes are now disabled by default.
    Please migrate them to data instead. See
    #7594 for more info.
  - Flip --incompatible_windows_escape_jvm_flags to true. See
    #7486

New features:

  - genrules now support a $(RULEDIR) variable that resolves to the
    directory where the outputs of the rule are put.
  - Added --incompatible_windows_native_test_wrapper flag: enables
    using the Bash-less test wrapper on Windows. (No-op on other
    platforms.)

Important changes:

  - incompatible_use_jdk11_as_host_javabase: makes JDK 11 the default
    --host_javabase for remote jdk
    (#7219)
  - Makes genquery somepath output deterministic.
  - Tristate attributes of native rules now reject True/False (use
    1/0)
  - Rollback of "Tristate attributes of native rules now reject
    True/False (use 1/0)"
  - Tristate attributes of native rules now reject True/False (use
    1/0)
  - Added -incompatible_do_not_split_linking_cmdline flag. See #7670
  - Tristate attributes of native rules now temporarily accept
    True/False again
  - `--incompatible_disable_legacy_crosstool_fields` has been flipped
    (#6861)
    `--incompatible_disable_expand_if_all_available_in_flag_set` has
    been flipped (#7008)
  - `--incompatible_disable_legacy_crosstool_fields` has been flipped
    (#6861)
    `--incompatible_disable_expand_if_all_available_in_flag_set...
    RELNOTES: None.
  - --incompatible_no_transitive_loads is enabled by default.
  - Makes TreeArtifact deterministic.
  - --incompatible_no_transitive_loads is enabled by default.
  - Android NDK C++ toolchain is now configured in Starlark. This
    should be a backwards compatible change, but in case of bugs
    blame unknown commit.
  - `--incompatible_disable_legacy_crosstool_fields` has been flipped
    (#6861)
    `--incompatible_disable_expand_if_all_available_in_flag_set` has
    been flipped (#7008)
  - --incompatible_no_transitive_loads is enabled by default.
  - --incompatible_bzl_disallow_load_after_statement is enabled
  - Added `--incompatible_require_ctx_in_configure_features`, see
    #7793 for details.
  - Flag --incompatible_merge_genfiles_directory is flipped. This
    removes the directory `bazel-genfiles` in favor of `bazel-bin`.
  - previously deprecated flag --experimental_remote_spawn_cache was
    removed
  - `--incompatible_disallow_load_labels_to_cross_package_boundaries`
    is enabled by default
  - Fix an issue where the Android resource processor did not surface
    errors from aapt2 compile and link actions.
  - --incompatible_no_attr_license is enabled by default
  - `--incompatible_disable_crosstool_file` has been flipped
    (#7320)
  - A new flag `--incompatible_string_join_requires_strings` is
    introduced. The sequence argument of `string.join` must contain
    only string elements.
  - --incompatible_symlinked_sandbox_expands_tree_artifacts_in_runfile
    s_tree has been flipped
  - Incompatible flag `--incompatible_disable_legacy_cc_provider` has
    been flipped (see #7036
    for details).
  - Don't drop the analysis cache when the same --define flag is set
    multiple times and the last value is the same (e.g. if the
    current invocation was run with "--define foo=bar" and the
    previous one was run with "--define foo=baz --define foo=bar").
  - The --incompatible_disable_genrule_cc_toolchain_dependency flag
    has been flipped (see
    #6867 for details).
  - Incompatible change
    `--incompatible_remove_cpu_and_compiler_attributes_from_cc_toolcha
    in` has been flipped (see
    #7075 for details).
  - --noexperimental_java_coverage is a no-op flag.
  - --experimental_java_coverage/--incompatible_java_coverage flag was
    removed. See #7425.
  - incompatible_use_toolchain_providers_in_java_common: pass
    JavaToolchainInfo and JavaRuntimeInfo providers to java_common
    APIs instead of configured targets
    (#7186.)
  - --incompatible_remote_symlinks has been flipped. The remote
    caching and execution protocol will now represent symlinks in
    outputs as such. See
    #7917 for more details.
  - Bazel is now ~20MiB smaller, from unbundling the Android rules'
    runtime dependencies.

This release contains contributions from many people at Google, as well as Andreas Herrmann, Andrew Suffield, Andy Scott, Benjamin Peterson, Ed Baunton, George Gensure, Ian McGinnis, Ity Kaul, Jingwen Chen, John Millikin, Keith Smiley, Marwan Tammam, Mike Fourie, Oscar Bonilla, perwestling, petros, Robert Sayre, Ryan Beasley, silvergasp, Stanimir Mladenov, Travis Cline, Vladimir Chebotarev, ??.

@bazel-io bazel-io closed this in bf66dc7 May 1, 2019

@brandjon

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

Unfortunately this broke a large number of downstream projects, mostly because it actually enforces that the Python interpreter has the requested version. Looks like switching the default Python version in Bazel 0.25 from PY2 to PY3 was too easy precisely because it wasn't being enforced at execution time. :(

We'll need to do some downstream fixing before we can flip again.

@brandjon brandjon reopened this May 2, 2019

@bazel-io bazel-io closed this in 4837e11 May 2, 2019

@brandjon brandjon reopened this May 2, 2019

bazel-io pushed a commit that referenced this issue May 2, 2019

Improve test compatibility with Python toolchain flag
This rolls-forward the parts of bf66dc7 that made our tests compatible with --incompatible_use_python_toolchains, without actually flipping the flag. The flag is set to default true within our test setup.

Work toward #7899.

TESTED=Confirmed that it doesn't break mac postsubmit: https://buildkite.com/bazel/bazel-bazel/builds/8065
PiperOrigin-RevId: 246404708

irengrig added a commit to irengrig/bazel that referenced this issue May 3, 2019

Introduce --incompatible_use_python_toolchains
This renames --experimental_use_python_toolchains to --incompatible. It also adds the behavior to the flag that

  1) py_runtime's python_version attribute becomes manadatory, and

  2) the --python_top flag is disallowed (it is superseded by toolchains), as well as --python2_path and --python3_path (they were already no-ops). --python_path is strongly discouraged but not yet banned due to an existing use on Windows (bazelbuild#7901).

Feature issue is bazelbuild#7375, incompatible change migration issue is bazelbuild#7899.

RELNOTES[INC]: Introduced --incompatible_use_python_toolchains, which supersedes --python_top/--python_path. See bazelbuild#7899 and bazelbuild#7375 for more information.

PiperOrigin-RevId: 241134532
@tmc

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

I'm trying this flag for rules_k8s in the context of bazelbuild/rules_k8s#305.
If I run a build with an explicit --python_top my build / test succeeds (bazelbuild/rules_k8s#306)
However if I use the new --incompatible_use_python_toolchains I get the following error:

ERROR: .../src/rules_k8s/fork/k8s/BUILD:58:1: Building par file //k8s:resolver.par failed (Exit 1) compiler.par failed: error executing command bazel-out/host/bin/external/subpar/compiler/compiler.par --manifest_file bazel-out/k8-fastbuild/bin/k8s/resolver.par_SOURCES --outputpar bazel-out/k8-fastbuild/bin/k8s/resolver.par --stub_file ... (remaining 4 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
src/main/tools/linux-sandbox-pid1.cc:427: "execvp(bazel-out/host/bin/external/subpar/compiler/compiler.par, 0x1830290)": No such file or directory

Note I did not experience this type of error when trying this flag out with rules_docker (bazelbuild/rules_docker#787)

See also bazelbuild/rules_k8s#305 for context

FWIW I think this is google/subpar#98

dkelmer added a commit that referenced this issue May 6, 2019

Release 0.25.0 (2019-05-01)
Baseline: 0366246

Cherry picks:

   + 3f7f255:
     Windows: fix native test wrapper's arg. escaping
   + afeb8d0:
     Flip --incompatible_windows_escape_jvm_flags
   + 4299b65:
     Sort DirectoryNode children to ensure validity.
   + 231270c:
     Conditionally use deprecated signature for initWithContentsOfURL
   + 75a3a53:
     Add http_archive entries for testing with various JDK versions.
   + 4a6354a:
     Now that ubuntu1804 uses JDK 11, remove explicit
     ubuntu1804_java11 tests.
   + ae102fb:
     Fix wrong name of ubuntu1804_javabase9 task.
   + 0020a97:
     Remove @executable_path/Frameworks from rpaths
   + 130f86d:
     Download stderr/stdout to a temporary FileOutErr

Incompatible changes:

  - (Starlark rules) The legacy "py" provider can no longer be passed
    to or produced by native Python rules; use
    [PyInfo](https://docs.bazel.build/versions/master/skylark/lib/PyIn
    fo.html) instead. See
    [#7298](#7298) for more
    information.
  - (Python rules) The `default_python_version` attribute of the
    `py_binary` and `py_test` rules has been renamed to
    `python_version`. Also, the `--force_python` flag has been
    renamed to `--python_version`. See
    [#7308](#7308) for more
    information.
  - (Python rules) The python version now changes to whatever version
    is specified in a `py_binary` or `py_test`'s `python_version`
    attribute, instead of being forced to the value set by a command
    line flag. You can temporarily revert this change with
    `--incompatible_allow_python_version_transitions=false`. See
    [#7307](#7307) for more
    information.
  - --incompatible_disable_third_party_license_checking` is enabled
    by default
  - Introduced --incompatible_use_python_toolchains, which supersedes
    --python_top/--python_path. See #7899 and #7375 for more
    information.
  - Python 3 is now the default Python version (for `py_binary` and
    `py_test` targets that don't specify the `python_version`
    attribute). Targets that are built for Python 3 will no longer
    have their output put in a separate `-py3` directory; instead
    there is now a separate `-py2` directory for Python 2 targets.
    See #7359 and #7593 for more information.
  - objc_library resource attributes are now disabled by default.
    Please migrate them to data instead. See
    #7594 for more info.
  - Flip --incompatible_windows_escape_jvm_flags to true. See
    #7486

New features:

  - genrules now support a $(RULEDIR) variable that resolves to the
    directory where the outputs of the rule are put.
  - Added --incompatible_windows_native_test_wrapper flag: enables
    using the Bash-less test wrapper on Windows. (No-op on other
    platforms.)

Important changes:

  - incompatible_use_jdk11_as_host_javabase: makes JDK 11 the default
    --host_javabase for remote jdk
    (#7219)
  - Makes genquery somepath output deterministic.
  - Tristate attributes of native rules now reject True/False (use
    1/0)
  - Rollback of "Tristate attributes of native rules now reject
    True/False (use 1/0)"
  - Tristate attributes of native rules now reject True/False (use
    1/0)
  - Added -incompatible_do_not_split_linking_cmdline flag. See #7670
  - Tristate attributes of native rules now temporarily accept
    True/False again
  - `--incompatible_disable_legacy_crosstool_fields` has been flipped
    (#6861)
    `--incompatible_disable_expand_if_all_available_in_flag_set` has
    been flipped (#7008)
  - `--incompatible_disable_legacy_crosstool_fields` has been flipped
    (#6861)
    `--incompatible_disable_expand_if_all_available_in_flag_set...
    RELNOTES: None.
  - --incompatible_no_transitive_loads is enabled by default.
  - Makes TreeArtifact deterministic.
  - --incompatible_no_transitive_loads is enabled by default.
  - Android NDK C++ toolchain is now configured in Starlark. This
    should be a backwards compatible change, but in case of bugs
    blame unknown commit.
  - `--incompatible_disable_legacy_crosstool_fields` has been flipped
    (#6861)
    `--incompatible_disable_expand_if_all_available_in_flag_set` has
    been flipped (#7008)
  - --incompatible_no_transitive_loads is enabled by default.
  - --incompatible_bzl_disallow_load_after_statement is enabled
  - Added `--incompatible_require_ctx_in_configure_features`, see
    #7793 for details.
  - Flag --incompatible_merge_genfiles_directory is flipped. This
    removes the directory `bazel-genfiles` in favor of `bazel-bin`.
  - previously deprecated flag --experimental_remote_spawn_cache was
    removed
  - `--incompatible_disallow_load_labels_to_cross_package_boundaries`
    is enabled by default
  - Fix an issue where the Android resource processor did not surface
    errors from aapt2 compile and link actions.
  - --incompatible_no_attr_license is enabled by default
  - `--incompatible_disable_crosstool_file` has been flipped
    (#7320)
  - A new flag `--incompatible_string_join_requires_strings` is
    introduced. The sequence argument of `string.join` must contain
    only string elements.
  - --incompatible_symlinked_sandbox_expands_tree_artifacts_in_runfile
    s_tree has been flipped
  - Incompatible flag `--incompatible_disable_legacy_cc_provider` has
    been flipped (see #7036
    for details).
  - Don't drop the analysis cache when the same --define flag is set
    multiple times and the last value is the same (e.g. if the
    current invocation was run with "--define foo=bar" and the
    previous one was run with "--define foo=baz --define foo=bar").
  - The --incompatible_disable_genrule_cc_toolchain_dependency flag
    has been flipped (see
    #6867 for details).
  - Incompatible change
    `--incompatible_remove_cpu_and_compiler_attributes_from_cc_toolcha
    in` has been flipped (see
    #7075 for details).
  - --noexperimental_java_coverage is a no-op flag.
  - --experimental_java_coverage/--incompatible_java_coverage flag was
    removed. See #7425.
  - incompatible_use_toolchain_providers_in_java_common: pass
    JavaToolchainInfo and JavaRuntimeInfo providers to java_common
    APIs instead of configured targets
    (#7186.)
  - --incompatible_remote_symlinks has been flipped. The remote
    caching and execution protocol will now represent symlinks in
    outputs as such. See
    #7917 for more details.
  - Bazel is now ~20MiB smaller, from unbundling the Android rules'
    runtime dependencies.

This release contains contributions from many people at Google, as well as Andreas Herrmann, Andrew Suffield, Andy Scott, Benjamin Peterson, Ed Baunton, George Gensure, Ian McGinnis, Ity Kaul, Jingwen Chen, John Millikin, Keith Smiley, Marwan Tammam, Mike Fourie, Oscar Bonilla, perwestling, petros, Robert Sayre, Ryan Beasley, silvergasp, Stanimir Mladenov, Travis Cline, Vladimir Chebotarev, ??.
@nlopezgi

This comment has been minimized.

Copy link
Member

commented May 8, 2019

Documentation here has a small typo. Currently says:
load("@bazel_tools//tools/python/toolchain.bzl", "py_runtime_pair")
when it should be
load("@bazel_tools//tools/python:toolchain.bzl", "py_runtime_pair")
Note /toolchain.bzl vs :toolchain.bzl

@brandjon

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

Fixed in 331c84b. :)

@brandjon

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

Downstream buildkite run here. Cataloging the failures:

  • Tests that run on mac workers fail because python3 is not on the PATH propagated to actions.

  • Android Testing failures are tracked here

  • Bazel's own failures are android-related so they're probably the same cause as the above.

  • The Bazel Toolchains failure is due to a PY2 host tool running as PY3.

  • The Cloud Robotics Core failure is also a host config issue.

  • Remote execution and Rules jvm external have some android issues.

  • rules_k8s has the same issue as the above host-config stuff with Bazel Toolchains

@brandjon

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

We're now targeting 0.27 for flipping this flag. In the absence of the execution transition feature, all projects that require PY2 host tools should set --host_force_python=PY2.

@drigz

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

all projects that require PY2 host tools should set --host_force_python=PY2.

AFAICT, this includes all projects that use certain parts of rules_docker, eg container_push. It depends on a Python 2-only version of httplib2: https://github.com/bazelbuild/rules_docker/blob/6fc0137cae4936b67c3c05dde1f77c88f59379f6/repositories/repositories.bzl#L89

@nlopezgi If this is flipped, you may need to document rules_docker's requirement of --host_force_python=PY2.

Is there a way code can be compatible with either Python version, eg something like py2or3_library()? Otherwise I don't see how a build could turn off --host_force_python=PY2 without a big-bang upgrade of all Python dependencies.

@nlopezgi

This comment has been minimized.

Copy link
Member

commented May 14, 2019

thanks for looping me. Yes, we will need to update docs to point to use of this flag. For now, we have updated the .bazelrc file in rules_docker (https://github.com/bazelbuild/rules_docker/blob/master/.bazelrc) to include this flag. Once 0.27 is I'll make sure to update docs to point to use of this flag as required.

@brandjon

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

It's unfortunately true that downstream projects will need to pass this flag as well. As for avoiding a big-bang change when turning off the flag, I think the answer to that will be to migrate away from the host configuration once the execution transition is ready.

@drigz

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

It's unfortunately true that downstream projects will need to pass this flag as well.

I'm a little concerned that this is hard to discover. We were lucky to have the issue you filed on our repository to point us to the flag, but others users will see their build fail on 0.27 with something like:

ContainerPushDigest push_foo.digest failed (Exit 1) digester failed: error executing command bazel-out/host/bin/external/containerregistry/digester --config bazel-out/k8-fastbuild/bin/main.0.config --manifest bazel-out/k8-fastbuild/bin/main.0.manifest --digest ... (remaining 21 argument(s) skipped)                                  

Use --sandbox_debug to see verbose messages from the sandbox
Traceback (most recent call last):
  File ".../sandbox/linux-sandbox/14/execroot/__main__/bazel-out/host/bin/external/containerregistry/digester.runfiles/containerregistry/tools/image_digester_.py", line 28, in <module>
    from containerregistry.client.v2_2 import docker_image as v2_2_image
  File ".../sandbox/linux-sandbox/14/execroot/__main__/bazel-out/host/bin/external/containerregistry/digester.runfiles/containerregistry/client/__init__.py", line 23, in <module>
    from containerregistry.client import docker_creds_
  File ".../sandbox/linux-sandbox/14/execroot/__main__/bazel-out/host/bin/external/containerregistry/digester.runfiles/containerregistry/client/docker_creds_.py", line 31, in <module>
    import httplib2
  File ".../sandbox/linux-sandbox/14/execroot/__main__/bazel-out/host/bin/external/containerregistry/digester.runfiles/httplib2/__init__.py", line 28, in <module>
    import email.FeedParser
ModuleNotFoundError: No module named 'email.FeedParser'

If they're lucky, they'll connect "ContainerPushDigest" to rules_docker, check the docs and see the flag mentioned. I wouldn't have done that; I'd probably have tried to update rules_docker and found that it didn't help. I'd have tried to build the rules_docker repo and seen that it worked fine. I might have used bazelisk --migrate with the old bazel version to identify the flag, found this issue and discovered --host_force_python.

I don't have a great idea on how to improve this. Perhaps adding migration instructions to the release notes would help (do people read them?) or perhaps rules_docker could add an assertion that Python 3 is being used, instructing the user to add --host_force_python if not.

@brandjon

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

That's a very good point. I have an idea how we could make this experience nicer.

The py_binary rule can detect at analysis time when it is getting the wrong Python version as a consequence of being used in the host configuration. It can tell this by the fact that its python_version attribute won't match the version stored in the configuration state.

When this situation is detected, we have a few options.

  1. We can fail-fast with an error. But this means that we cannot support PY2 and PY3 py_binarys in the same build, even if the actual user Python code is compatible with both.

  2. We can emit a warning. This may be spammy, but it gives a very good chance that a user looking through a traceback will see the likely cause of their problem.

  3. We could also do either of the above and provide a way to silence the failure/warning on a per-target basis by setting an attribute a certain way, but this would be ugly to implement and eventually cleanup. It also wouldn't work in cases where the host tool is in an upstream repo.

  4. When this situation is detected, we carry on as normal but pass this information to the generated stub script. The stub script will then monitor the user Python code, and if it has a non-zero exit code, emit additional text explaining that the problem may be due to this issue. (This implies a slight performance penalty of changing an os.execv call to subprocess, but that code path would only be activated for host tools with this kind of version mismatch.)

I think option 4 is the most versatile. Note that any solution to this problem will be useful not just for the 0.27 migration, but also as long as we still have Python tools in the host configuration going forward, i.e. as long as the host configuration is still a thing.

@drigz

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

Good ideas! 4 sounds good to me if the complexity is acceptable. The only downside I see is that it will confuse users who see the warning spuriously when something unrelated breaks their genrule.

If the stub can know its python_version, could it fall back to using the system interpreter when the python_version doesn't match? Combined with a warning, this could provide the smoothest transition path, although I can imagine that it has other consequences.

FYI, while I was reading up on python_version I noticed this comment, which you might want to adjust to reflect recent changes:

<p><b>Bug warning:</b> This attribute sets the version for which Bazel builds your target,

We can emit a warning. This may be spammy, but it gives a very good chance that a user looking through a traceback will see the likely cause of their problem.

A fresh build of our project prints out 150 lines of warnings due to #7157 so we could well miss it. Printing the warning in the stub would make it more noticeable.

@brandjon

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

I noticed this comment, which you might want to adjust

Thanks, but unfortunately #4815 is still relevant until this flag is flipped and until we also have an autodetecting Python toolchain on Windows.

A fresh build of our project prints out 150 lines of warnings due to #7157 so we could well miss it.

A good argument to do it in the stub script then. Note that since we'll emit it only after the user code has dumped its stack trace, the relevant message should be closer to where the user is looking in the logs.

@brandjon

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

FYI for anyone depending on subpar, the 2.0.0 release makes it compatible with the default Python toolchain.

@nlopezgi

This comment has been minimized.

Copy link
Member

commented May 15, 2019

I think option 4 is the most versatile. Note that any solution to this problem will be useful not just for the 0.27 migration, but also as long as we still have Python tools in the host configuration going forward, i.e. as long as the host configuration is still a thing.

This option sgtm too. Please let me know how this proceeds so I can document as best as possible in rules_docker docs.
Also, is implementing any of these alternatives considered blocking for 0.27.0?

@brandjon

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

I believe I can implement 4, which should supersede the other choices, and do it in time for 0.27.

bazel-io pushed a commit that referenced this issue May 15, 2019

Annotate android tools/tests as PY2
This is needed to ensure we continue to give these tests/binaries a Python 2 runtime once --incompatible_use_python_toolchains is enabled.

Work toward #7899.

RELNOTES: None
PiperOrigin-RevId: 248391969
@nlopezgi

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Great! thanks!

@brandjon

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

Here are a few other ideas to help make this migration smoother:

  • Provide a recipe for a toolchain that parses the shebang of the main .py file, if it exists, and delegates to that interpreter. Attempt to ensure this toolchain only matches the host platform.

  • Add a hack to the py_binary rule to select (from the toolchain) the runtime whose version matches the value of python_version, rather than the value of the version in the configuration.

Both of these amount to pretty much the same thing: Detect when we're in the host config and getting the wrong version, and pick the right version interpreter in spite of the configuration. But since these workarounds don't actually change the configuration state, any select()s in the Python target or its transitive dependencies will see the wrong Python version, and any srcs_version constraints that require the correct version may cause the build to fail.

I'll note that at no point so far has Bazel ever supported multiple Python versions in the host configuration. So these workarounds would add expressivity that was not there before. The breakages caused by the toolchain flag occur not because you need both versions, but because you need just one, and you're not getting that version anymore because #4815 is being fixed. So I think --host_force_python + better diagnostics should be sufficient for the purposes of flipping this flag.

@brandjon

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

If the stub can know its python_version, could it fall back to using the system interpreter when the python_version doesn't match?

Sorry @drigz, just realized you had suggested a variant of what I later described above. Yes, that's similar to the "choose the right runtime from the toolchain in spite of the configuration" approach. I'm not sure whether that's better or worse than adding a warning upon failure.

I guess I'm a bit concerned that it makes it possible for the build to lumber along in a partially broken state. Sure you get the right interpreter, but your select()s (if you use any) will see the wrong version, and if someone adds a correct PY[2|3]ONLY constraint to the srcs_version of a transitive dependency, you break.

So maybe I'd rather force the user to set --host_force_python, since that also sets the configuration to be consistent with what's actually run.

@drigz

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

No problem. The warning seems reasonable to me.

your select()s (if you use any) will see the wrong version

Do you have an example of a select() for Python version? IIUC this would let rules_docker use the appropriate version of httplib2 depending on the value of --host_force_python.

@brandjon

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

Documentation is in-line with the source here. You can basically just select on the config settings @bazel_tools//tools/python:PY2 and :PY3. (No need for a default case since those two options cover all possibilities.) If you need a select branch with more conjunctive conditions you can define your own config setting using the :python_version target.

@drigz

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

Thanks! I tried to make rules_docker compatible with both, could you have a look? It was a bit harder than I expected but it should unblock the Py3 migration for some users. bazelbuild/rules_docker#843

bazel-io pushed a commit that referenced this issue May 17, 2019

Add a warning to Python host tools built for the wrong version
The Python stub script now emits a warning message when its payload exits abnormally, if the target was built 1) in the host configuration, 2) for the wrong Python version, and 3) with Python toolchains enabled. This is aimed at improving the discoverability of migration issues for #7899, since otherwise the only stderr output would be a Python stack trace.

Work toward #7899.

RELNOTES: All host-configured Python tools that are built for the wrong Python version will now emit a warning message when they exit with non-zero status. See #7899.
PiperOrigin-RevId: 248717898
@keith

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

I experienced an issue using this flag on macOS #8414 that I submitted a fix for #8415

bazel-io pushed a commit that referenced this issue May 21, 2019

Fix the autodetecting Python toolchain on Mac
See bazelbuild/continuous-integration#578 for context. The gist of it is that when PATH is not set (as happens when a binary using this toolchain is used as a tool from Starlark), the shell comes up with its own PATH to run the pywrapper script. However, it does not necessarily export this PATH, causing "which" to misbehave and fail to locate a Python interpreter.

The fix is to add "export PATH" and a regression test.

Fixes bazelbuild/continuous-integration#578, work toward #7899.

RELNOTES: None
PiperOrigin-RevId: 249263849

aehlig added a commit that referenced this issue May 23, 2019

Fix the autodetecting Python toolchain on Mac
See bazelbuild/continuous-integration#578 for context. The gist of it is that when PATH is not set (as happens when a binary using this toolchain is used as a tool from Starlark), the shell comes up with its own PATH to run the pywrapper script. However, it does not necessarily export this PATH, causing "which" to misbehave and fail to locate a Python interpreter.

The fix is to add "export PATH" and a regression test.

Fixes bazelbuild/continuous-integration#578, work toward #7899.

RELNOTES: None
PiperOrigin-RevId: 249263849

aehlig added a commit that referenced this issue May 23, 2019

Fix the autodetecting Python toolchain on Mac
See bazelbuild/continuous-integration#578 for context. The gist of it is that when PATH is not set (as happens when a binary using this toolchain is used as a tool from Starlark), the shell comes up with its own PATH to run the pywrapper script. However, it does not necessarily export this PATH, causing "which" to misbehave and fail to locate a Python interpreter.

The fix is to add "export PATH" and a regression test.

Fixes bazelbuild/continuous-integration#578, work toward #7899.

RELNOTES: None
PiperOrigin-RevId: 249263849
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.