[ROCm] ci build and dockerfile changes #161
Conversation
Ruturaj4
commented
Nov 26, 2024
•
Ruturaj4
commented
- Install system GCC libraries
- Remove docker image
- apt update
Ruturaj4
added
the
open-upstream
|
Feature branch from main is ready. Create a new PR%20Install%20system%20GCC%20libraries%0D%0A2)%20Remove%20docker%20image%0D%0A3)%20apt%20update%0A%0ACreated%20from%3A%20rocm%2Fjax%23161) destined for upstream? |
charleshofer
added
open-upstream
|
Feature branch from main is ready. Create a new PR%20Install%20system%20GCC%20libraries%0D%0A2)%20Remove%20docker%20image%0D%0A3)%20apt%20update%0A%0ACreated%20from%3A%20rocm%2Fjax%23161) destined for upstream? |
charleshofer
added
open-upstream
|
Feature branch from main is ready. Create a new PR destined for upstream? |
Ruturaj4
force-pushed
the
ci_install_sys_libs
branch
from
f76b97a to
d2e058e
Compare
Ruturaj4
added
open-upstream
|
Feature branch from main is ready. Create a new PR destined for upstream? |
| @@ -47,6 +47,13 @@ def dist_wheels( | |||
| # create manylinux image with requested ROCm installed | |||
| image = "jax-manylinux_2_28_x86_64_rocm%s" % rocm_version.replace(".", "") | |||
|
|
|||
| # Try removing the Docker image. | |||
| try: | |||
There was a problem hiding this comment.
I don't think we should do this. I was talking to Matt, and he said that the original intent of this script was to run it in Jenkins where the image has already been removed and Docker has been pruned. When you're not running in Jenkins (and on the GitHub Actions self-hosted runner), docker build should be smart enough to pick up the new image.
| @@ -4,6 +4,9 @@ ARG ROCM_VERSION=6.1.1 | |||
| ARG ROCM_BUILD_JOB | |||
| ARG ROCM_BUILD_NUM | |||
|
|
|||
| # Install system GCC and C++ libraries. | |||
| RUN yum install -y gcc-c++.x86_64 | |||
There was a problem hiding this comment.
I know this is here to fix the problem where Clang can't find the standard C++ libraries, but this is definitely something we shouldn't be doing (at least long-term). GCC 13, which is the GCC version for manylinux_2_28, is already installed as part of the base image. If this pulls in, say, GCC 14 stuff, this may put us out of manylinux compliance.
I suspect there's another solution here that involves telling Clang to check standard C++ libraries wherever GCC 13 is keeping them.
@mrodden Any thoughts on where that might be?
| @@ -75,6 +75,9 @@ def install_packages(self, package_specs): | |||
| if self.pkgbin == "apt": | |||
| env["DEBIAN_FRONTEND"] = "noninteractive" | |||
|
|
|||
| # Update indexes. | |||
| subprocess.check_call(["apt-get", "update"]) | |||
There was a problem hiding this comment.
Just for my education, what is this line for?
