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

0.5.3 no longer allows to enforce Python3 #3517

Closed
abergmeier-dsfishlabs opened this issue Aug 7, 2017 · 23 comments
Closed

0.5.3 no longer allows to enforce Python3 #3517

abergmeier-dsfishlabs opened this issue Aug 7, 2017 · 23 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-dsfishlabs
Copy link
Contributor

Description of the problem / feature request / question:

I have a python script, that worked fine in 0.5.2.

Switching to 0.5.3 running

import concurrent.futures

gives me:

ImportError: No module named concurrent.futures

Environment info

  • Operating System: Ubuntu 16.04.2
  • Bazel version (output of bazel info release): 0.5.3
@abergmeier-dsfishlabs
Copy link
Contributor Author

Printing paths before the error I get:

/<project>/fusion/python/gof3m/collect
/<bazelroot>/execroot/com_dsfishlabs_tools/bazel-out/local-py3-fastbuild/bin/fusion/python/collect.runfiles
/<bazelroot>/execroot/com_dsfishlabs_tools/bazel-out/local-py3-fastbuild/bin/fusion/python/collect.runfiles/com_dsfishlabs_tools
/usr/lib/python2.7
/usr/lib/python2.7/plat-x86_64-linux-gnu
/usr/lib/python2.7/lib-tk
/usr/lib/python2.7/lib-old
/usr/lib/python2.7/lib-dynload
/home/andreas/.local/lib/python2.7/site-packages
/usr/local/lib/python2.7/dist-packages
/usr/lib/python2.7/dist-packages
/usr/lib/python2.7/dist-packages/PILcompat
/usr/lib/python2.7/dist-packages/gtk-2.0
/usr/lib/python2.7/dist-packages/ubuntu-sso-client

which is obviously wrong since I enforced Python 3:

py_binary(
    ...
    default_python_version = "PY3",
    srcs_version = "PY3ONLY",

Regression is in 0.5.4rc2, too 👎

@abergmeier-dsfishlabs abergmeier-dsfishlabs changed the title 0.5.3 does not import from standard library 0.5.3 no longer allows to enforce Python3 Aug 7, 2017
@buchgr
Copy link
Contributor

buchgr commented Aug 7, 2017

There was some breaking change in 0.5.3, that unfortunately didn't make it to the release notes. I think you need to specify --python3_path in the .bazelrc file now? It might be a different issue that I am thinking of.

@meteorcloudy might know more?

@abergmeier-dsfishlabs
Copy link
Contributor Author

@buchgr To quote the python3_path docs:

Local path to the Python3 executable. Deprecated, please use python_path or python_top instead.

So I guess you mean python_path?

@buchgr buchgr self-assigned this Aug 8, 2017
@buchgr
Copy link
Contributor

buchgr commented Aug 8, 2017

So apparently, those attributes don't work anymore and you now need to specify --python_path to point to your python executable.

I am in contact with relevant people... sorry :(

@hlopko hlopko added category: rules > python P2 We'll consider working on this in future. (Assignee optional) under investigation labels Aug 9, 2017
@buchgr
Copy link
Contributor

buchgr commented Aug 11, 2017

It was an intentional breakage, that unfortunately did not make it into the release notes. We will provide a blog post and updated documentation soon.

@abergmeier-dsfishlabs
Copy link
Contributor Author

Can you perhaps write a bit more here.
I now tried to use py_runtime in combination with PY3ONLY and PY3 and it seems like it does not properly link the library into the sandbox!?

@buchgr
Copy link
Contributor

buchgr commented Aug 16, 2017

My understanding is that starting with 0.5.3, bazel no longer knows or cares about the python version. You just point it to a python runtime and it will use it. The default_python_version argument and PY3ONLY and PY3 don't have any effect.

@abergmeier-dsfishlabs
Copy link
Contributor Author

abergmeier-dsfishlabs commented Aug 16, 2017

My understanding is that starting with 0.5.3, bazel no longer knows or cares about the python version

Sadly that is wrong.

The default_python_version argument and PY3ONLY and PY3 don't have any effect.

That may be correct. Please either remove them or add tests to validate every permutation. Still there is --force_python=PY3 and --host_force_python=PY3, which still trigger 2to3 and fun in 0.5.3.

Also it seems like py_runtime is not getting properly added to the sandbox.

Can you please make the dev in charge of Python write up how it is "supposed" to work now and I will file bugs then.

@abergmeier-dsfishlabs
Copy link
Contributor Author

My understanding is that starting with 0.5.3, bazel no longer knows or cares about the python version

Is it then no longer possible to have both Python2 and Python3 in one repo?

@duggelz
Copy link

duggelz commented Sep 19, 2017

Any movement on this?

@AustinSchuh
Copy link
Contributor

Is it then no longer possible to have both Python2 and Python3 in one repo?

I think I just ran into that when attempting to upgrade from 0.5.0 to 0.6.0 yesterday. We've got mixed python2 and python3 binaries in our build. I'd love to upgrade to python3 entirely, but not all of the libraries we use support python3 yet.

@buchgr
Copy link
Contributor

buchgr commented Dec 4, 2017

@lberki can tell you more about plans for python maybe?

@joshburkart
Copy link

At the very minimum, I need to be able to have a single codebase with both Python 2 and Python 3 code, and then manually specify in the individual test/binary which interpreter to use. Is this possible somehow? A temporary workaround would be fine. A global flag is unsuitable, since I need to be able to do e.g. bazel test //....

(This is in particular necessitated by Google-developed Python 2-only libraries like Apache Beam.)

@buchgr
Copy link
Contributor

buchgr commented Mar 12, 2018

@lberki can you take this please?

@abergmeier
Copy link
Contributor

Sooo soonish, we will be a year later and still no explanation?

@buchgr
Copy link
Contributor

buchgr commented Jul 4, 2018

friendly ping @lberki :)

@AdamDorwart
Copy link

AdamDorwart commented Aug 29, 2018

I just wanted to offer a temporary workaround that we're using to allow multiple python runtimes in a single workspace. You can set your py_runtime to a decorator script that inspects the shebang on the first line of the script to invoke a particular interpreter. You could make this a simple shell script which someone had suggested in another thread I can't seem to find.

If you're using distroless images (py_image or py3_image) using a shell script might be an issue so I wrote a statically linked go application. This is pretty heavy handed but not so bad if you're already using go in your workspace. Hopefully either one of these options is viable until someone gets this sorted out.

.bazelrc

build --python_top=//:python-2-or-3

BUILD

load(
    "@io_bazel_rules_go//go:def.bzl",
    "go_binary",
)

go_binary(
    name = "python_decorator",
    srcs = ["python.go"],
    gc_linkopts = [
        "-linkmode",
        "external",
        "-extldflags",
        "-static",
    ],
)

py_runtime(
    name = "python-2-or-3",
    files = [],
    interpreter = ":python_decorator",
)

python.go

package main

import (
	"bufio"
	"log"
	"os"
	"strings"
	"syscall"
)

func python2Binary() string {
	return "/usr/bin/python"
}

func python3Binary() string {
	return "/usr/bin/python3"
}

func main() {
	if len(os.Args) < 2 {
		log.Fatal("No python script provided!")
	}
	file, err := os.Open(os.Args[1])
	if err != nil {
		log.Fatal(err)
	}
	defer file.Close()

	scanner := bufio.NewScanner(file)
	scanner.Scan()
	firstLine := scanner.Text()
	if err := scanner.Err(); err != nil {
		log.Fatal(err)
	}

	switch {
	case strings.Contains(firstLine, "python3"):
		if err := syscall.Exec(python3Binary(), append([]string{python3Binary()}, os.Args[1:]...), os.Environ()); err != nil {
			log.Fatal(err)
		}
	default:
		if err := syscall.Exec(python2Binary(), append([]string{python2Binary()}, os.Args[1:]...), os.Environ()); err != nil {
			log.Fatal(err)
		}
	}

}

@buchgr
Copy link
Contributor

buchgr commented Aug 29, 2018

Thanks @AdamDorwart !

@rickeylev
Copy link
Contributor

Another way of doing it is using a select statement on the py_runtime rule.
(the below using interpreter_path, but the same should be possible with interpreter)
(I gave this a quick try, seems to work)

It still requires a custom arg to bazel; imho bazel should have it configured like this by default. Maybe an easy fix is to just add the config_setting() and select() I mention below directly to whatever Bazel's default py_runtime definition is.

Also, some caveats:

  1. I think host mode can only use 1 python version. i.e., if you have two genrules, one that needs py2 and one that needs py3, it won't work.
  2. I think the transition to py3 is a bit "one way", i.e., once its in py3 mode, bazel can't switch back to py2 mode. So if you do something complicated like a py2 binary that puts a py3 binary in data, and then that py3 binary puts a py2 binary in data, then it'll probably break in some way.
  3. Since its a custom py_runtime, you have to pass the extra flag to all your builds.
config_setting(
    name = "py3config",
    values = {"force_python": "PY3"}
)

py_runtime(
    name = "myruntime",
    interpreter_path = select({
        ":py3config": "/usr/bin/python3",
         "//conditions:default": "/usr/bin/python"
         }),
    files = []
    )

Also, just for posterity and to reconfirm what someone said up thread: Reading through the Bazel source, it looks like only --python_path and py_runtime are used to figure out the binary to use to run the python target (i started with how it populates the template script and went backwards from there). I'm guessing this is because Bazel expects people to use the select() way to select different values depending on the configuration.

@mfarrugi
Copy link

@AdamDorwart I believe this is the bash script you were referencing:

if head -n 1 "$1" | grep -q python3; then
  exec "$BASE_PATH"/usr/bin/python3 "$@"
else
  exec "$BASE_PATH"/usr/bin/python2 "$@"
fi

from https://groups.google.com/forum/#!topic/bazel-discuss/nVQ48R94S_8

@brandjon
Copy link
Member

Note that the shebang-parsing way will choose the version based on the actual source file contents, while the select() way (and the eventual fix to Bazel's Python rules) will choose based on the version selected in the build (default_python_version attr / --force_python).

@rickeylev's right about the transition being one way, but I can imagine changing it so that it's only one way through deps, and gets reset when it crosses a data attribute.

The host mode issue is also a problem because it means you can't use Python 3 tools in your genrules (or if you invert it with --host_force_python=PY3, then you can't use Python 2 tools). That one's harder to fix, so if you really need multiple python versions in your host config you may want to stick with indirecting through a script.

@brandjon
Copy link
Member

I'm closing this issue and opening several more directed ones to cover the points raised in this thread.

For a more detailed index of Python 2/3 issues, see tracking bug #6444, most of which is targeted for this quarter.

@brandjon
Copy link
Member

brandjon commented Feb 5, 2019

Note that select()-ing on "force_python" is no longer supported / will be disallowed. As of release 0.23, the workaround in #3517 (comment) should be replaced by the modified one given in #4815 (comment).

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