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

py_library enforces Python 2 for some reason #1446

Closed
abergmeier opened this issue Jun 24, 2016 · 20 comments
Closed

py_library enforces Python 2 for some reason #1446

abergmeier opened this issue Jun 24, 2016 · 20 comments
Assignees
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Rules-Python Native rules for Python type: bug

Comments

@abergmeier
Copy link
Contributor

We are not interested in anything lower than Python 3 so we set all our srcs_version to PY3 fpr py_* rules. Still when Analyzing, Bazel complains:

Rule '@foo//:python_foo' need to be converted to Python 2 (not yet implemented).

The error message is quite unhelpful because it does not explain why it "needs" to be converted nor which thing is not yet implemented.

@abergmeier
Copy link
Contributor Author

Workaround seems to be --force_python=PY3 and --host_force_python=PY3 in command line.

@dslomov dslomov added type: documentation (cleanup) P2 We'll consider working on this in future. (Assignee optional) labels Jun 27, 2016
@dslomov
Copy link
Contributor

dslomov commented Jun 27, 2016

Need more helpful error message here

@ulfjack
Copy link
Contributor

ulfjack commented Jul 26, 2016

I don't think py_library enforces anything. :-)

The srcs_version attribute is mainly intended to support migration from python 2 to python 3. The error message is confusing because Bazel thinks you want it to auto-convert the code from python 3 down to python 2, which is the default output.

I think you could just remove all the srcs_version, and it should work just fine.

@abergmeier-dsfishlabs
Copy link
Contributor

@ulfjack Removing the srcs_version I get:

Converting to Python 3: foo.py failed

with it calling bazel-out/host/bin/external/bazel_tools/tools/python/2to3, which is pretty useless since foo.py is a python3 script already.

@ulfjack
Copy link
Contributor

ulfjack commented Jul 27, 2016

I'm not sure I follow - can you give me more details on how to reproduce the issue?

@abergmeier-dsfishlabs
Copy link
Contributor

@ulfjack I would really love to have a complete description how the different python conversion/flags/attributes are used and in which combination they should produce which output.
Is there anyone at Google who could describe what the logic behind all that is?

@ulfjack
Copy link
Contributor

ulfjack commented Aug 10, 2016

We've done some work to support a migration from Python 2 to Python 3, and as part of that, the rules can convert both 2to3 and 3to2 (IIRC) if you have appropriate tools. However, I'm not sure why Blaze thinks that those conversions should be triggered. Unfortunately, that code is ~3-4 years old, and nobody working on Bazel right now has the necessary context.

@abergmeier-dsfishlabs
Copy link
Contributor

@ulfjack I think the implementation is flawed. See 2. in #1580. AFAIU it cannot work correctly this way.
The rest of this "weirdness" then stems from doucmentation issue (1.).

@ulfjack
Copy link
Contributor

ulfjack commented Aug 10, 2016

If by flawed you mean "it doesn't work for all use cases", then I agree.

Where should we start fixing the python rules? What's the biggest pain point?

@abergmeier-dsfishlabs
Copy link
Contributor

  • Before anything else I think there should be unit tests, which test all the different python combinations
  • I think the workaround currently could be to introduce a flag for disabling the up/downgrading of python files completely (given that nobody seems to be sure how and when it works) at all or alternatively fix the code that it handles correctly.
  • Then of course fix documentation.
  • Fix referencing python2 at all. Might be safe for macOS but not for Ubuntu 16.04+ or Windows.

@BrendanDrewZippy
Copy link

I have also run into this problem. The documentation gives the expectation that if you write python 3 code and set srcs_version='PY3' in your BUILD, everything should "just work". This is not the case.

I think I'm up-to-date. bazel info gives:

INFO: Reading 'startup' options from /Users/bdrew/devel/zippy/.bazelrc: --host_jvm_args=-Xmx2500m --host_jvm_args=-Xms2500m --batch
bazel-bin: /private/var/tmp/_bazel_bdrew/36485b236a991e0e17b0caecf9bb47ca/execroot/zippy/bazel-out/darwin_x86_64-fastbuild/bin
bazel-genfiles: /private/var/tmp/_bazel_bdrew/36485b236a991e0e17b0caecf9bb47ca/execroot/zippy/bazel-out/darwin_x86_64-fastbuild/genfiles
bazel-testlogs: /private/var/tmp/_bazel_bdrew/36485b236a991e0e17b0caecf9bb47ca/execroot/zippy/bazel-out/darwin_x86_64-fastbuild/testlogs
character-encoding: file.encoding = ISO-8859-1, defaultCharset = ISO-8859-1
command_log: /private/var/tmp/_bazel_bdrew/36485b236a991e0e17b0caecf9bb47ca/command.log
committed-heap-size: 2512MB
execution_root: /private/var/tmp/_bazel_bdrew/36485b236a991e0e17b0caecf9bb47ca/execroot/zippy
gc-count: 3
gc-time: 93ms
install_base: /var/tmp/_bazel_bdrew/install/db3d7840db9ac45fa322fb3d55e6626c
java-home: /Library/Java/JavaVirtualMachines/jdk1.8.0_121.jdk/Contents/Home/jre
java-runtime: Java(TM) SE Runtime Environment (build 1.8.0_121-b13) by Oracle Corporation
java-vm: Java HotSpot(TM) 64-Bit Server VM (build 25.121-b13, mixed mode) by Oracle Corporation
max-heap-size: 2512MB
message_log: /private/var/tmp/_bazel_bdrew/36485b236a991e0e17b0caecf9bb47ca/message.log
output_base: /private/var/tmp/_bazel_bdrew/36485b236a991e0e17b0caecf9bb47ca
output_path: /private/var/tmp/_bazel_bdrew/36485b236a991e0e17b0caecf9bb47ca/execroot/zippy/bazel-out
package_path: %workspace%
release: release 0.5.1-homebrew
server_pid: 26710
used-heap-size: 297MB
workspace: /Users/bdrew/devel/zippy

When I try to build this target:

py_library(
    name = "mercurylib",
    srcs = glob(["lib/**/*.py"]),
    srcs_version = "PY3",
    deps = [
        "//applications/mercury/proto:py_mercury",
        "@com_github_google_protobuf//:protobuf_python",
    ],
)

bazel tells me:

bazel build //applications/mercury:mercurylib
INFO: Reading 'startup' options from /Users/bdrew/devel/zippy/.bazelrc: --host_jvm_args=-Xmx2500m --host_jvm_args=-Xms2500m --batch
ERROR: /Users/bdrew/devel/zippy/applications/mercury/BUILD:13:1: in py_library rule //applications/mercury:mercurylib: Rule '//applications/mercury:mercurylib' need to be converted to Python 2 (not yet implemented).
ERROR: Analysis of target '//applications/mercury:mercurylib' failed; build aborted.

which is, shall we say, counterintuitive.

@brandjon brandjon self-assigned this Sep 6, 2018
@brandjon
Copy link
Member

brandjon commented Sep 6, 2018

I've been looking at Python 2 vs 3 versioning issues lately and expect to have a simple fix shortly for this kind of spurious and annoying error. Let me recap what's happening here.

The purpose of srcs_version is to annotate what major versions of Python your src files are compatible with. Today this attribute is not used to actually select what mode to build in. It'll just raise an error if you try to build it in the wrong mode.

The Python mode is controlled by --force_python (and I guess its --host... counterpart), or if that's not set, by the default_python_version attribute of a py_binary or py_test rule. Once the mode is set, it doesn't change for any further upstream targets that are depended on.

If you have a srcs_version=PY3 or PY3ONLY library, and you build a default_python_version=PY3 binary that depends on it, that'll satisfy the check. But if you build the library directly, rather than the binary, then the default_python_version attr on the binary isn't processed and the PY2 default is used instead. That's why errors occur in this situation.

The solution is to also set the python mode, if not set already, based on srcs_version if default_python_version is not applicable (as is the case for a library target). (Note that this fix does not imply that Bazel will select a python3 interpreter to use out-of-the-box. That's a separate problem.)

The original variant of the error message in this thread is obsolete. I believe it was indicating that since the Python mode was 2 but srcs_version said 3, it wanted to run 3to2 on the srcs, and 3to2 conversion was not yet implemented. Then the plan changed to not run 3to2 at all, and PY3 and PY3ONLY were made synonyms. source

bazel-io pushed a commit that referenced this issue Sep 10, 2018
This is a precursor to fixing #1446.

RELNOTES: None
PiperOrigin-RevId: 212267168
bazel-io pushed a commit that referenced this issue Sep 11, 2018
This is needed to open source unit tests for Python rules.

Work towards #1446 and other Python-related items.

RELNOTES: None
PiperOrigin-RevId: 212474035
bazel-io pushed a commit that referenced this issue Sep 14, 2018
The moved tests used to cover binary/test and now also cover library.

Also added some test cases for srcs_version.

Work toward #1446.

RELNOTES: None
PiperOrigin-RevId: 212999264
@brandjon
Copy link
Member

Update: This work was delayed by a little yak shaving to open source some unit tests for our Python rules. It's now blocked on the fact that my fix breaks some users of bazel cquery/aquery, which is experimental. We're looking at whether the underlying issue with cquery/aquery can be fixed so it's not a breakage, and if not, whether it's ok to just continue anyway. I won't be able to work on this until mid next week anyway, hopefully the blocker will be resolved by then.

@Globegitter
Copy link

@brandjon any update on this? We are kind of holding off with some of our development just until having the py2/py3 support fixed as it sounded it would not be far away. But if it is still a few weeks/months away we might have to start finding/using a different solution.

@brandjon
Copy link
Member

When you say you're blocked on having py2/3 support fixed, which particular issues do you mean? I'm using this thread to track only the specific issue that you can't directly (i.e. on the command line) build a srcs_version = PY3 library without also passing --force_python=PY3.

I just made an index of PY2/3-related issues at #6444, most of which are targeted for this quarter.

For the bug described by this thread, here's an update. The blocker with cquery/aquery was fixed. The new blocker we're facing is that we're worried that fixing this bug will break py3 targets that have py2 data dependencies. This in turn requires fixing #6441. We may also need an incompatible change flag, but that's debatable because Python 3 support is marked "experimental". I'm anticipating resuming work on the blocker and this issue next week.

@brandjon
Copy link
Member

Another blocker, this time more serious: We're finding that there are action conflicts between targets built in the PY2 mode, and targets built in the "I haven't decided what python version I'm using yet" mode.

That is, if you declare --force_python=PY2 or default_python_version="PY2", that's different from not declaring anything at all, even though the global default is Python 2. One example where the difference matters is if you have some transitive dependency that does a select() on "force_python", say to control what sources you're building with. Then the two modes give different results, hence the conflict when we try to write the results to the same path.

One solution is to make a separate output root to distinguish the "I haven't decided" mode from PY2 mode, the way we already have the "-py3" output root for PY3 mode.

But another solution I'm investigating is doing away with the "I haven't decided" mode altogether. The more I think about it, the more I think this model may be simpler and avoid redundant build computations.

@gpshead
Copy link

gpshead commented Oct 23, 2018

Doing away with "I haven't decided" mode sounds like the right thing.

@samuela
Copy link
Contributor

samuela commented Oct 24, 2018

In that case there would be three modes: PY2, PY3, and PY2AND3, but no "unset", correct? Then what mode would py_library default to? Like if you built a pytype_library target would it typecheck in python 2, python 3, or both? It seems that PY2AND3 is the most natural/expected choice but that's likely a breaking change for many users. And what happens when Python 4 rolls around? etc...

It seems to me that this is one big constraint solving problem. The only twist here is that some build rules change at "runtime" via select(). But perhaps a constraint solving attack could be taken here. After all it seems that using select() is a bit of a hack in the first place. For example, each target could have a set of constraints that it specifies based on its own contents, and relative to the constraints of its dependencies. Then bazel bulid would simply solve this constraint problem to identify all the configurations that are supported by each build target. Then just build all of those.

@brandjon
Copy link
Member

PY2AND3 is a srcs_version value indicating compatibility with PY2 and PY3. It is not a value for the python mode (default_python_version / --force_python).

We have no plans to make the Python major version mode handle anything besides PY 2 vs PY 3. I believe the Python community has decided that (a hypothetical) PY 4 will be significant for its name rather than for its breaking changes, so it's unlikely we'll see the need to manage compatibility to the same extent as for 2 vs 3.

The more general problem you describe is tackled to some extent by the platforms/toolchains framework. We don't use it for Python 2 vs 3 because at the moment the framework is for platform-based constraints rather than version-based constraints, though that may change in the future.

If you want to generalize, the next issue may be minor version numbers, where you might want to say some py_library contains source that only runs on an interpreter > x.y. But this is a slippery slope: you could also be concerned about patch release versions, more expressive constraint logic (excluding one-off bad versions), constraints on the interpreter family (CPython, PyPy, etc.), and so on.

At the moment we are designing for the common use case of dealing with a dual PY2 / PY3 code base including an active migration from 2 to 3. By the time we address the other cases the platform/toolchain framework may be more of an obvious answer.

bazel-io pushed a commit that referenced this issue Jan 4, 2019
The py provider is currently just a struct manipulated by native code. Eventually it should be a full-fledged Provider, but in the meantime this moves its accessors to a separate utility class.

This is preparation for adding new provider fields to address #6583 and #1446. The cleanup is also a small step toward #7010.

RELNOTES: None
PiperOrigin-RevId: 227771462
bazel-io pushed a commit that referenced this issue Jan 15, 2019
PyProvider manages how Java code creates and accesses the fields of the legacy "py" struct provider (which we'll migrate away from soonish). This CL refactors default value logic into PyProvider, and adds a builder so it's easier to construct. It also slightly refactors some shared-library logic in PyCommon. More cleanup (hopefully) to come.

Work toward #7010, yak shaving for #6583 and #1446.

RELNOTES: None
PiperOrigin-RevId: 229401739
bazel-io pushed a commit that referenced this issue Jan 15, 2019
Simplify computing the transitive sources and transitive-without-local sources by factoring a method or two and separating out the computation of `convertedFiles`.

Work toward #7010, yak shaving for #6583 and #1446.

RELNOTES: None
PiperOrigin-RevId: 229410930
bazel-io pushed a commit that referenced this issue Jan 15, 2019
PyCommon's initialization and validation helpers are renamed and moved to near the top of the class, followed by simple accessors. Initialization methods are made static so there are no longer any implicit dependencies on which fields get initialized first.

Python import paths are now computed in the PyCommon constructor in order to follow the pattern of other provider fields. This requires passing the PythonSemantics to the constructor, so while I was at it I made PyCommon keep a reference to the semantics. I was a little concerned about this promoting increased coupling between PyCommon and PythonSemantics, but given that both classes serve essentially the same purpose I'm not too worried.

Work toward #7010, yak shaving for #6583 and #1446.

RELNOTES: None
PiperOrigin-RevId: 229425980
bazel-io pushed a commit that referenced this issue Jan 18, 2019
When --experimental_allow_python_version_transitions is enabled, srcs_version will now be checked by py_binary/py_test instead of at every py_library. This allows building multiple py_library targets in the same invocation (e.g. via bazel build //pkg/...) regardless of their srcs_versions constraints.

The check occurs at analysis time, but any failure is reported at execution time. This allows a configured target to be created for diagnostic purposes (i.e. finding the bad transitive dependency). I've updated our analysis-time tests to account for this possibility by asserting that this deferred failure does not occur.

The "py" provider now has fields `has_py2_only_sources` and `has_py3_only_sources`. Will add a couple tests for accessing these from Starlark code in a follow-up CL.

Work towards #6583 and #1446.

RELNOTES: None
PiperOrigin-RevId: 229986854
bazel-io pushed a commit that referenced this issue Jan 18, 2019
The rewritten test is non-Bazel specific and directly checks the sandwich use case.

There's a slight loss in coverage that will be addressed with #7054.

Work towards #1446.

RELNOTES: None
PiperOrigin-RevId: 230002328
bazel-io pushed a commit that referenced this issue Jan 25, 2019
We need this aspect because under the new Python version semantics, incompatibilities in srcs_version are caught by py_binary rather than at the py_library where the mismatch occurred. The workflow is for users to see the validation error and follow a URL to documentation (to come in a later CL) instructing them to run this aspect to locate the bad dependencies.

The aspect propagates to deps and uses both srcs_version and the py provider fields to try to determine the top-most targets that require Python 2 or Python 3. It's possible this idiom of "find the topmost dependencies satisfying property X" is reusable and could be factored into Skylib (perhaps once this code is moved out of core Bazel and into rules_python).

Incidentally removed python_version_srcs filegroup in favor of just using srcs.

Work towards #6583 and #1446.

RELNOTES: None
PiperOrigin-RevId: 230813240
weixiao-huang pushed a commit to weixiao-huang/bazel that referenced this issue Jan 31, 2019
PyProvider manages how Java code creates and accesses the fields of the legacy "py" struct provider (which we'll migrate away from soonish). This CL refactors default value logic into PyProvider, and adds a builder so it's easier to construct. It also slightly refactors some shared-library logic in PyCommon. More cleanup (hopefully) to come.

Work toward bazelbuild#7010, yak shaving for bazelbuild#6583 and bazelbuild#1446.

RELNOTES: None
PiperOrigin-RevId: 229401739
weixiao-huang pushed a commit to weixiao-huang/bazel that referenced this issue Jan 31, 2019
Simplify computing the transitive sources and transitive-without-local sources by factoring a method or two and separating out the computation of `convertedFiles`.

Work toward bazelbuild#7010, yak shaving for bazelbuild#6583 and bazelbuild#1446.

RELNOTES: None
PiperOrigin-RevId: 229410930
weixiao-huang pushed a commit to weixiao-huang/bazel that referenced this issue Jan 31, 2019
PyCommon's initialization and validation helpers are renamed and moved to near the top of the class, followed by simple accessors. Initialization methods are made static so there are no longer any implicit dependencies on which fields get initialized first.

Python import paths are now computed in the PyCommon constructor in order to follow the pattern of other provider fields. This requires passing the PythonSemantics to the constructor, so while I was at it I made PyCommon keep a reference to the semantics. I was a little concerned about this promoting increased coupling between PyCommon and PythonSemantics, but given that both classes serve essentially the same purpose I'm not too worried.

Work toward bazelbuild#7010, yak shaving for bazelbuild#6583 and bazelbuild#1446.

RELNOTES: None
PiperOrigin-RevId: 229425980
weixiao-huang pushed a commit to weixiao-huang/bazel that referenced this issue Jan 31, 2019
When --experimental_allow_python_version_transitions is enabled, srcs_version will now be checked by py_binary/py_test instead of at every py_library. This allows building multiple py_library targets in the same invocation (e.g. via bazel build //pkg/...) regardless of their srcs_versions constraints.

The check occurs at analysis time, but any failure is reported at execution time. This allows a configured target to be created for diagnostic purposes (i.e. finding the bad transitive dependency). I've updated our analysis-time tests to account for this possibility by asserting that this deferred failure does not occur.

The "py" provider now has fields `has_py2_only_sources` and `has_py3_only_sources`. Will add a couple tests for accessing these from Starlark code in a follow-up CL.

Work towards bazelbuild#6583 and bazelbuild#1446.

RELNOTES: None
PiperOrigin-RevId: 229986854
weixiao-huang pushed a commit to weixiao-huang/bazel that referenced this issue Jan 31, 2019
The rewritten test is non-Bazel specific and directly checks the sandwich use case.

There's a slight loss in coverage that will be addressed with bazelbuild#7054.

Work towards bazelbuild#1446.

RELNOTES: None
PiperOrigin-RevId: 230002328
weixiao-huang pushed a commit to weixiao-huang/bazel that referenced this issue Jan 31, 2019
We need this aspect because under the new Python version semantics, incompatibilities in srcs_version are caught by py_binary rather than at the py_library where the mismatch occurred. The workflow is for users to see the validation error and follow a URL to documentation (to come in a later CL) instructing them to run this aspect to locate the bad dependencies.

The aspect propagates to deps and uses both srcs_version and the py provider fields to try to determine the top-most targets that require Python 2 or Python 3. It's possible this idiom of "find the topmost dependencies satisfying property X" is reusable and could be factored into Skylib (perhaps once this code is moved out of core Bazel and into rules_python).

Incidentally removed python_version_srcs filegroup in favor of just using srcs.

Work towards bazelbuild#6583 and bazelbuild#1446.

RELNOTES: None
PiperOrigin-RevId: 230813240
@brandjon
Copy link
Member

brandjon commented Apr 1, 2019

--incompatible_allow_python_version_transitions is flipped in Bazel 0.25.

@brandjon brandjon closed this as completed Apr 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Rules-Python Native rules for Python type: bug
Projects
None yet
Development

No branches or pull requests

9 participants