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

[WIP DO NOT MERGE] update to work with Python 3.10. #5

Closed
wants to merge 5 commits into from

Conversation

ethanluoyc
Copy link

No description provided.

@ethanluoyc
Copy link
Author

@kenjitoyama I just updated the PR and have tested this to compile for Python 3.10. This should be good enough for a start.

The main issues around updating the dependencies should be sorted, but there are some things which are just quite hard to do it with hacks on my side. Specifically, I am not entirely sure what's the approach to work with python packaging on top of bazel right now, so the PR currently has some code that is a bit coupled with py310. Let me unpack. I added some comments to the diff so perhaps you can help me take a look?

.gitignore Outdated
Copy link
Author

Choose a reason for hiding this comment

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

Added for gitignore sanity.

Copy link
Author

Choose a reason for hiding this comment

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

Only updated for the single Dockerfile, maybe we can discuss what base to use for the allversions Dockerfile as well.


# Override gcc/g++.
RUN update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-8 60 --slave /usr/bin/g++ g++ /usr/bin/g++-8
RUN update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-9 60 --slave /usr/bin/g++ g++ /usr/bin/g++-9
Copy link
Author

Choose a reason for hiding this comment

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

gcc-9 as per Tensorflow 2.12.0

https://www.tensorflow.org/install/source


package(default_visibility = ["//visibility:public"])

# This rule adds a convenient way to update the requirements file.
compile_pip_requirements(
Copy link
Author

Choose a reason for hiding this comment

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

If we use requirement, then it expects a compiled dependency (generated below).

This is a hack! I couldn't find a good way to create requirements locks with multiple Python versions.

Copy link
Author

Choose a reason for hiding this comment

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

I think the rules_python pip repository does not work well with repositories that have native extensions

@@ -82,10 +82,21 @@ cc_test(
"@com_google_absl//absl/status",
Copy link
Author

Choose a reason for hiding this comment

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

Oops I forgot to clean thses up, will follow-up.

docker/Dockerfile Outdated Show resolved Hide resolved
@ethanluoyc
Copy link
Author

ethanluoyc commented Jun 22, 2023

@kenjitoyama
I got rid of all requirements as it was easier to first install these into the build environment in OSS. This is what launchpad and reverb do as well. I am not sure how much the BUILD.bazel shares with blaze (I guess not much) but this might be something you would like to fix internally. In launchpad and reverb I think there is some build_def.bzl sharing with blaze.

The cross_language_test fails when running with bazel test because for some reason the way it passed the runfile environment variables removes PATH information from the subprocess call which causes the py_writer to resolve to the python under /usr/bin/python. When running outside the container, this causes the tests to fail.

I think the code mostly works so we can start thinking about cleaning it up?

@kenjitoyama
Copy link
Collaborator

Hi @ethanluoyc , sorry for the delay I was OOO.

What's the current state now? Does this build the 3.10 wheels successfully?

Thanks,

Daniel

@ethanluoyc
Copy link
Author

Hi @ethanluoyc , sorry for the delay I was OOO.

What's the current state now? Does this build the 3.10 wheels successfully?

Thanks,

Daniel

Hi!
Yes, it should work.

copybara-service bot pushed a commit that referenced this pull request 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
@kenjitoyama
Copy link
Collaborator

This work has been absorbed into 1c0d8f2.

@kenjitoyama kenjitoyama closed this Jul 6, 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.

2 participants