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

Python 3.10 Wheel #4

Closed
JesseFarebro opened this issue Aug 23, 2022 · 17 comments
Closed

Python 3.10 Wheel #4

JesseFarebro opened this issue Aug 23, 2022 · 17 comments
Labels
enhancement New feature or request

Comments

@JesseFarebro
Copy link

Hi!

Would it be possible to publish Python 3.10 wheels? Other DeepMind projects are already publishing for 3.10 and it would be really nice to have envlogger.

Thanks!

@kenjitoyama
Copy link
Collaborator

Hi @JesseFarebro, yeah, we should definitely also publish 3.10 wheels. I'm really busy this week, but it shouldn't be a major problem I think (at least I don't see any obvious technical issues blocking it). Cheers!

@kenjitoyama kenjitoyama added the enhancement New feature or request label Aug 23, 2022
@ethanluoyc
Copy link

@kenjitoyama Hey is there any update on this?

@kenjitoyama
Copy link
Collaborator

Hi! Sorry for the massive delay, I forgot about this. I'll take a look.... the annoying part is ensuring that it plays nice with TFDS's glibc version, protobuf's version, pybind11_protobuf's version and libstdc++'s version. I'll take a look at this and update this bug.

@ethanluoyc
Copy link

ethanluoyc commented Jun 20, 2023

@kenjitoyama
Thanks, I actually did some digging. I think there are a few updates that need to be done for the third-party packages. This probably needs some coordination with riegeli's dependencies as well as I believe that the master branch currently breaks with TF 2.12.0 (the latest release version).

Not sure if it's at all helpful, here are some things which I found to be useful when attempting to create PR (which I didn't manage to finish because coordinating a few repos outside google3 is painful and bazel doesn't quite work with my pyenv setup.) Updating some dependencies get me to compile envlogger with py3.10. I did this with Bazel 6.1.0 instead of <4 as I thought maybe we can update the bazel version so that's it's not too out-of-sync.

# Probably a good idea to be in sync with the TF release?
http_archive(
    name = "com_google_absl",
    sha256 = "54707f411cb62a26a776dad5fd60829098c181700edcd022ea5c2ca49e9b7ef1",
    strip_prefix = "abseil-cpp-20220623.1",
    urls = ["https://github.com/abseil/abseil-cpp/archive/refs/tags/20220623.1.zip"],  # 2022-06-23
)

# This seems to be the version of protobuf as of TF 2.12.0
http_archive(
    name = "com_google_protobuf",
    # patch_args = ["-p1"],
    # patches = ["//third_party:protobuf.patch"],
    # sha256 = "cfcba2df10feec52a84208693937c17a4b5df7775e1635c1e3baffc487b24c9b",
    sha256 = "f66073dee0bc159157b0bd7f502d7d1ee0bc76b3c1eac9836927511bdc4b3fc1",
    strip_prefix = "protobuf-3.21.9",
    urls = ["https://github.com/protocolbuffers/protobuf/archive/v3.21.9.zip"], 
)
load("@com_google_protobuf//:protobuf_deps.bzl", "protobuf_deps")
protobuf_deps()

http_archive(
    name = "com_google_googletest",
    # sha256 = "ff7a82736e158c077e76188232eac77913a15dac0b22508c390ab3f88e6d6d86",
    sha256 = "81964fe578e9bd7c94dfdb09c8e4d6e6759e19967e397dbea48d1c10e45d0df2",
    # strip_prefix = "googletest-b6cd405286ed8635ece71c72f118e659f4ade3fb",
    strip_prefix = "googletest-release-1.12.1",
    urls = [
        # "https://storage.googleapis.com/mirror.tensorflow.org/github.com/google/googletest/archive/b6cd405286ed8635ece71c72f118e659f4ade3fb.zip",
        # "https://github.com/google/googletest/archive/b6cd405286ed8635ece71c72f118e659f4ade3fb.zip",
        "https://github.com/google/googletest/archive/refs/tags/release-1.12.1.tar.gz"
    ],
)

http_archive(
    name = "pybind11_bazel",
    sha256 = "b72c5b44135b90d1ffaba51e08240be0b91707ac60bea08bb4d84b47316211bb",
    strip_prefix = "pybind11_bazel-b162c7c88a253e3f6b673df0c621aca27596ce6b",
    urls = ["https://github.com/pybind/pybind11_bazel/archive/b162c7c88a253e3f6b673df0c621aca27596ce6b.zip"],
)

# We still require the pybind library.
http_archive(
    name = "pybind11",
    build_file = "@pybind11_bazel//:pybind11.BUILD",
    strip_prefix = "pybind11-2.10.4",
    urls = ["https://github.com/pybind/pybind11/archive/v2.10.4.tar.gz"],
)

load("@pybind11_bazel//:python_configure.bzl", "python_configure")

Let me know if I could be of more help! I want to say thanks again for the effort of maintaining this as it is a really useful library to have working with the other DM RL libraries.

@kenjitoyama
Copy link
Collaborator

Hi @ethanluoyc! Yeah, it won't be so easy. Coordinating all of these deps is super annoying.

I think our best bet is to start from ubuntu:20.04 (which is the image used by tensorflow), upgrade deps like the protobuf lib, pybind11 etc and then try to get it to build.

@ethanluoyc
Copy link

@kenjitoyama Yeah that was needed for py310. I thought maybe working with the TF build image https://hub.docker.com/r/tensorflow/build is another way given we want ABI compatibility with TF.

Totally agree. I think in general tooling around python packages built by bazel is a mess. Same goes for launchpad and reverb which all have separate ways for managing the complexity.

@kenjitoyama
Copy link
Collaborator

I'm already getting some errors trying to build a docker image with 3.10:

Traceback (most recent call last):
  File "/usr/lib/python3.10/runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/lib/python3.10/runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "/usr/lib/python3/dist-packages/pip/__main__.py", line 16, in <module>
    from pip._internal.cli.main import main as _main  # isort:skip # noqa
  File "/usr/lib/python3/dist-packages/pip/_internal/cli/main.py", line 10, in <module>
    from pip._internal.cli.autocompletion import autocomplete
  File "/usr/lib/python3/dist-packages/pip/_internal/cli/autocompletion.py", line 9, in <module>
    from pip._internal.cli.main_parser import create_main_parser
  File "/usr/lib/python3/dist-packages/pip/_internal/cli/main_parser.py", line 7, in <module>
    from pip._internal.cli import cmdoptions
  File "/usr/lib/python3/dist-packages/pip/_internal/cli/cmdoptions.py", line 19, in <module>
    from distutils.util import strtobool
ModuleNotFoundError: No module named 'distutils.util'

pip is still using the deprecated distutils module. :(

@ethanluoyc, how far did you get in the process?

@kenjitoyama
Copy link
Collaborator

pip's error should be fixed with pypa/pip@6739f56, but the version in ubuntu:20.04 is older than that.

@ethanluoyc
Copy link

ethanluoyc commented Jun 20, 2023

@kenjitoyama I created a PR which captures what I have right now. No guarantees that it would work and I also wasn't using the docker image :)

Roughly the updates I applied.

  1. Update bazel to 6.1.0 (cause 3 is old right)
  2. Update riegelli to master (there are a few more commits since the last time I pull but don't matter)
  3. Update a few dependencies (mainly protobuf, absl, googletest, pybind). For protobuf I don't find the patches to be needed anymore but I could be wrong. I did not check the if the patches needed by riegeli are up-to-date.
  4. Update the BUILD files (riegelli deprecates the :base target so I removed those). I removed the pypi requirements for my setup as there were some issues with my pyenv setup but preinstalled them but it should not be necessary in your case.
  5. The bazelrc is mainly for my local config, no need for that.
  6. I added some debugging statements to cross_language_test because bazel picks up the wrong python in PATH but you don't need that.

I remember getting this to work with most tests and the failing tests I got are mostly due to bazel not picking up the correct python.

@ethanluoyc
Copy link

Maybe once we figure this out we can think about setting up some GitHub CI? I can certainly help with that.

@kenjitoyama
Copy link
Collaborator

Hi @ethanluoyc ! Thanks for the PR, it's helpful to see your work. Are you able to build the wheels for Python 3.10? The command would be something like python3 envlogger/setup.py bdist_wheel --plat manylinux2014_x86_64. After building the wheels you can try them yourself to see if everything works as expected.

WRT Github CI we already have an internal CI system that run tests on the docker image (using Dockerfile as the "source"). I'm not opposed to having a GitHub Action that complements that system though, and in fact that's what we do with AndroidEnv and it works really well. Having the docker image nailed first will make any such CI attempts 10x easier.

@ethanluoyc
Copy link

Yeah I will give that a try. Would you want me to create an entire PR so you can review the changes and run the internal tests or you would want to update things on your end (I was hoping it was easier updating on your end but I could be wrong :D)

@kenjitoyama
Copy link
Collaborator

I can update the things on our end once we find a golden configuration. ;)

@ethanluoyc
Copy link

Cool. I will let you know of any update.

@ethanluoyc
Copy link

@kenjitoyama made some attempts

@kenjitoyama
Copy link
Collaborator

Hi @ethanluoyc , I gave up on trying to use pip_parse() in Bazel and have instead did what you did by just installing the pip dependencies directly into the system. I've got everything working, except the cross language test which I didn't look into (it's not a huge issue though since we do test that internally to make sure everything is working) and that we can take a look later since it's unrelated to bazel, pip and Python.

The new PyPi packages are available here: https://pypi.org/project/envlogger/1.1/

I'll prepare an internal change and hopefully it'll be submitted by the end of this week.

Thanks a lot for the investigation!

Daniel

@ethanluoyc
Copy link

@kenjitoyama Good to know!

I have encountered the issue with cross-language test and it's got to do with py_test not finding the correct python interpreter in bazel with a non-hermetic python toolchain configuration. (you need to pass the host PATH to bazel test in e.g. bazelrc for test to get the correct interpreter) I have some hacks which can fix that issue but I think it's too much hassle to test that OSS as long as you have it working internally.

copybara-service bot pushed a commit that referenced this issue Jul 6, 2023
This change modifies `Dockerfile` and `WORKSPACE.bazel` to build for Python
`3.10` instead of `3.8`, besides upgrading a lot of of the underlying
dependencies. All the `BUILD.bazel` files have also been updated to reflect
this. The `Dockerfile` now also takes a build `ARG` which allows us to remove
`Dockerfile.allpyversions`.

The `pip` dependencies are now managed in a different way and are installed in
the system by `pip` instead of via `bazel`'s `pip_install()` (deprecated) /
`pip_parse()`. This makes our code a lot simpler for now, but we may revisit
this in the future.

Huge thanks to Yicheng Luo <ethanluoyc@gmail.com> (ethanluoyc@) for debugging a
lot of the issues here, especially with `bazel` and `pip`. Please see #5 and #4.

This fixes issue #4.

The new PyPi release is available at https://pypi.org/project/envlogger/1.1/.

PiperOrigin-RevId: 546115546
Change-Id: I49bb7a35b029e6b925ec4fc2c029a93972a0fe5c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants