Add support for Centos9 Stream + GCC12 #9903

majetideepak · 2024-05-23T06:51:54Z

Resolves #9879

netlify · 2024-05-23T06:52:08Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`7da4072`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6667316247c40a00084db500

assignUser

I think we need to clean up scripts/ at some point ^^ The docker.yml workflow will need to be updated as well
LGTM so far, let's test it in CI like you suggested!

assignUser · 2024-05-28T02:40:11Z

scripts/centos-9-stream.dockerfile

Is anything using this, a quick grep didn't show anything? If not we should remove it.

scripts/setup-centos9.sh

assignUser · 2024-05-28T02:41:37Z

scripts/circleci-container.dockfile

This is also defunct imo and should be removed :)

czentgr

Thank you for these changes!
I have only a few comments.

czentgr · 2024-05-29T13:03:16Z

scripts/setup-centos9.sh

+function install_cuda {
+  # See https://developer.nvidia.com/cuda-downloads
+  dnf config-manager --add-repo https://developer.download.nvidia.com/compute/cuda/repos/rhel8/x86_64/cuda-rhel8.repo
+  yum install -y cuda-nvcc-$(echo $1 | tr '.' '-') cuda-cudart-devel-$(echo $1 | tr '.' '-')


As discussed the path on line 190 needs update to RHEL9.
https://developer.download.nvidia.com/compute/cuda/repos/rhel9/x86_64/cuda-rhel9.repo

Should probably use dnf. Also this doesn't work with how the install functions are called. $1 is never set. $1 is representing a version in the form major.minor (e.g. 12.5) which gets converted to major-minor (e.g. 12-5) which is the suffix for the packages. We don't have to fix it here but without a change to the file this function cannot be used as input to the script.

In the github workflow file, I see that this is invoked along with the version.
I don't see why we can't include the CUDA version in this script via an ENV variable.
I will followup on this separately.

czentgr · 2024-05-29T13:04:56Z

scripts/setup-centos9.sh

+function install_velox_deps_from_dnf {
+  dnf_install libevent-devel \
+    openssl-devel re2-devel libzstd-devel lz4-devel double-conversion-devel \
+    libdwarf-devel curl-devel libicu-devel bison flex libsodium-devel zlib-devel


As discussed, lets add elfutils-libelf-devel. This is needed to ensure that folly is able to turn on the symbolization for the stacktraces. It isn't present by default.

czentgr · 2024-05-29T13:10:37Z

scripts/setup-centos9.sh

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# This script documents setting up a Centos8 host for Velox


Typo: Centos8 -> Centos9.

czentgr · 2024-05-29T13:11:27Z

scripts/setup-centos9.sh

+#     Use "n" to never wipe directories.
+#
+# You can also run individual functions below by specifying them as arguments:
+# $ scripts/setup-centos8.sh install_googletest install_fmt


Typo: centos8 -> centos9 or centos.

majetideepak · 2024-05-30T06:50:51Z

@assignUser CUDA 11.8 is not supported with GCC 12.2.1.
According to https://stackoverflow.com/questions/6622454/cuda-incompatible-with-my-gcc-version, we need to use CUDA > 12.1.
Do you know if it is okay to upgrade the CUDA version?

assignUser · 2024-05-31T18:59:38Z

Do you know if it is okay to upgrade the CUDA version?

I think there was some discussion about this in the PR adding cuda to adapters but I don't know any specifics #9335

wanglinsong · 2024-06-03T17:06:06Z

We also need to upgrade the base image for the java build.

assignUser · 2024-06-04T02:37:34Z

This should fix the abseil issue (it needs to be built with C++17 so absl::string_view == std::stringview):

diff --git a/scripts/setup-adapters.sh b/scripts/setup-adapters.sh
index 7a487e877..679729f99 100755
--- a/scripts/setup-adapters.sh
+++ b/scripts/setup-adapters.sh
@@ -61,7 +61,7 @@ function install_gcs-sdk-cpp {
   github_checkout abseil/abseil-cpp 20240116.2 --depth 1
   cmake_install \
     -DABSL_BUILD_TESTING=OFF \
-    -DCMAKE_CXX_STANDARD=14 \
+    -DCMAKE_CXX_STANDARD=17 \
     -DABSL_PROPAGATE_CXX_STD=ON \
     -DABSL_ENABLE_INSTALL=ON

majetideepak · 2024-06-04T04:53:50Z

it needs to be built with C++17

@assignUser thanks for the pointer. I was searching for ABSL_USE_STRINGVIEW. I had a hunch that the CXX standard was different.

kgpai · 2024-06-04T17:53:09Z

@majetideepak any reason to keep it in draft ?

majetideepak · 2024-06-04T18:32:13Z

@kgpai Multiple issues came up and are now fixed.
The last issue is the test failures

majetideepak · 2024-06-04T18:33:29Z

In Linux release with adapters

The following tests FAILED:
	328 - velox_gcsfile_test (Failed)
	330 - velox_hive_connector_test (Failed)

majetideepak · 2024-06-04T18:39:51Z

I see /opt/miniforge/envs/adapters/bin/python3: No module named testbench for velox_gcsfile_test failures.
@assignUser, @tigrux any clue?

assignUser · 2024-06-04T18:42:43Z

It's a test dependency for gcs and should be installed in the env of the docker file. I can take a look

tigrux · 2024-06-04T18:47:14Z

I see /opt/miniforge/envs/adapters/bin/python3: No module named testbench for velox_gcsfile_test failures. @assignUser, @tigrux any clue?

I see it is installed in:
https://github.com/facebookincubator/velox/blob/main/scripts/adapters.dockerfile#L34

However I don't really know how to add it to the image used in the current PR.

majetideepak · 2024-06-04T18:50:43Z

However I don't really know how to add it to the image used in the current PR.

@tigrux, I added a new adapters-cpp target in the docker-compose file in this PR. I used that to build the adapters image. The docker-compose build invokes the adapters.dockerfile.

assignUser · 2024-06-05T01:22:28Z

@majetideepak I just rebuild the adapters image with the docker file from this PR and it does install testbench in the environment:

[root@48ce3479dcd7 /]# mamba run -n adapters /bin/bash -c "pip list"
Package                      Version
---------------------------- --------
certifi                      2024.6.2
charset-normalizer           3.3.2
click                        8.1.7
crc32c                       2.3
Flask                        2.2.4
googleapis-common-protos     1.59.0
googleapis-storage-testbench 0.33.0
[...]

But when I run the same command in the tmp container used in ci nothing is installed:

[root@6502d6f7f833 /]# mamba run -n adapters /bin/bash -c "pip list"
Package    Version
---------- -------
pip        24.0
setuptools 70.0.0
wheel      0.43.0

You probably need to just rebuild and push your container?

majetideepak · 2024-06-05T05:01:46Z

@assignUser thanks! I was using podman to build the image in a background shell. I now see the following log

WARN[0923] SHELL is not supported for OCI image format, [mamba run -n adapters /bin/bash -c] will be ignored. Must use `docker` format 
--> 5bc97a7b2e85
STEP 13/15: ENV HADOOP_HOME=/usr/local/hadoop     HADOOP_ROOT_LOGGER="WARN,DRFA"     LC_ALL=C     LIBHDFS3_CONF=/velox/scripts/hdfs-client.xml     PATH=/usr/local/hadoop/bin:${PATH}
WARN[0936] SHELL is not supported for OCI image format, [mamba run -n adapters /bin/bash -c] will be ignored. Must use `docker` format 
--> db9e7d78910f
STEP 14/15: ENTRYPOINT ["/bin/bash", "-c", "source /opt/rh/gcc-toolset-12/enable && exec \"$@\"", "--"]
WARN[0936] SHELL is not supported for OCI image format, [mamba run -n adapters /bin/bash -c] will be ignored. Must use `docker` format 
--> d71f700d8e86
STEP 15/15: CMD ["/bin/bash"]
COMMIT ghcr.io/facebookincubator/velox-dev:adapters
WARN[0936] SHELL is not supported for OCI image format, [mamba run -n adapters /bin/bash -c] will be ignored. Must use `docker` format 
--> 56d9ef7c366a

majetideepak · 2024-06-05T15:05:20Z

There is one last test failure velox_hive_connector_test which happens only in the release build. I am looking into that.

majetideepak · 2024-06-06T03:08:30Z

I opened a separate PR for the last commit #10079

majetideepak · 2024-06-06T03:53:48Z

All tests are now passing.

Summary: I noticed in #9903 that the upload errors due to missing credentials. This PR adds a guard that allows the job to finish ✔️ even from a fork. It will still run the upload (for testing) when the PR is from within the main repo. Pull Request resolved: #10080 Reviewed By: bikramSingh91 Differential Revision: D58244591 Pulled By: kevinwilfong fbshipit-source-id: e75bf84292ad046dfc85be9f1a3b813af474cf11

Summary: Pass by value to avoid strict aliasing issues for REAL and DOUBLE types. We are seeing the HivePartitionFunctionTest failing with GCC12 here #9903 Pull Request resolved: #10079 Reviewed By: Yuhta Differential Revision: D58294082 Pulled By: kevinwilfong fbshipit-source-id: 765af472a35d1f8f2b619d5c58ee4399a8359ee2

assignUser

Thanks!

kgpai

Looks good. Can mark for merge after fix of nit.

kgpai · 2024-06-10T16:22:40Z

.github/workflows/build-metrics.yml

@@ -37,7 +37,7 @@ jobs:
    name: Linux ${{ matrix.type }} with adapters
    if: ${{ github.repository == 'facebookincubator/velox' }}
    runs-on: ${{ matrix.runner }}
-    container: ghcr.io/facebookincubator/velox-dev:adapters
+    container: docker.io/majetideepak4/velox:adapters9


nit: I presume this was for testing

facebook-github-bot · 2024-06-12T21:54:41Z

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-06-12T23:12:52Z

@kgpai merged this pull request in f9f6d82.

Summary: Resolves facebookincubator#9879 Pull Request resolved: facebookincubator#9903 Reviewed By: kevinwilfong Differential Revision: D58491851 Pulled By: kgpai fbshipit-source-id: 5edbb6c9b5eff40adc0888291a9107c078d1125e

conbench-facebook · 2024-06-12T23:37:33Z

Conbench analyzed the 1 benchmark run on commit f9f6d822.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…bator#10080) Summary: I noticed in facebookincubator#9903 that the upload errors due to missing credentials. This PR adds a guard that allows the job to finish ✔️ even from a fork. It will still run the upload (for testing) when the PR is from within the main repo. Pull Request resolved: facebookincubator#10080 Reviewed By: bikramSingh91 Differential Revision: D58244591 Pulled By: kevinwilfong fbshipit-source-id: e75bf84292ad046dfc85be9f1a3b813af474cf11

Summary: Pass by value to avoid strict aliasing issues for REAL and DOUBLE types. We are seeing the HivePartitionFunctionTest failing with GCC12 here facebookincubator#9903 Pull Request resolved: facebookincubator#10079 Reviewed By: Yuhta Differential Revision: D58294082 Pulled By: kevinwilfong fbshipit-source-id: 765af472a35d1f8f2b619d5c58ee4399a8359ee2

Summary: Resolves facebookincubator#9879 Pull Request resolved: facebookincubator#9903 Reviewed By: kevinwilfong Differential Revision: D58491851 Pulled By: kgpai fbshipit-source-id: 5edbb6c9b5eff40adc0888291a9107c078d1125e

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 23, 2024

assignUser reviewed May 28, 2024

View reviewed changes

czentgr reviewed May 29, 2024

View reviewed changes

majetideepak force-pushed the centos9-support branch from 1aa2734 to 14f8e14 Compare May 29, 2024 13:37

majetideepak force-pushed the centos9-support branch from bdbb779 to 3ca9cf9 Compare May 30, 2024 07:19

czentgr mentioned this pull request Jun 3, 2024

[native] Update dockerfiles to use Centos9 prestodb/presto#22909

Merged

6 tasks

majetideepak force-pushed the centos9-support branch from c483cc6 to 37cb6dd Compare June 4, 2024 01:32

assignUser closed this Jun 4, 2024

assignUser reopened this Jun 4, 2024

majetideepak force-pushed the centos9-support branch from 11ecd48 to 78fae2c Compare June 4, 2024 04:51

majetideepak mentioned this pull request Jun 4, 2024

Linux release with adapters CI fails with "Failed to download metadata for repo 'appstream'" #10016

Closed

majetideepak force-pushed the centos9-support branch from ab742e3 to 0389831 Compare June 5, 2024 05:28

majetideepak force-pushed the centos9-support branch from 0389831 to ee88f95 Compare June 6, 2024 02:57

majetideepak mentioned this pull request Jun 6, 2024

Fix hashOne functions in HivePartitionFunction #10079

Closed

majetideepak marked this pull request as ready for review June 6, 2024 03:51

assignUser mentioned this pull request Jun 6, 2024

Guard metrics upload and report generation for fork PRs #10080

Closed

majetideepak force-pushed the centos9-support branch from ee88f95 to f737f37 Compare June 6, 2024 12:34

assignUser approved these changes Jun 7, 2024

View reviewed changes

majetideepak force-pushed the centos9-support branch from f737f37 to a5d847a Compare June 8, 2024 01:13

czentgr approved these changes Jun 10, 2024

View reviewed changes

kgpai reviewed Jun 10, 2024

View reviewed changes

Add support for Centos9 Stream + GCC12

52b3e97

majetideepak force-pushed the centos9-support branch from a5d847a to ff9351c Compare June 10, 2024 16:56

revert docker repo links

7da4072

majetideepak force-pushed the centos9-support branch from ff9351c to 7da4072 Compare June 10, 2024 17:01

kevinwilfong mentioned this pull request Jun 11, 2024

[Velox] Fix some gaps in clang format #10127

Closed

kgpai approved these changes Jun 12, 2024

View reviewed changes

kgpai added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Jun 12, 2024

facebook-github-bot closed this in f9f6d82 Jun 12, 2024

facebook-github-bot added the Merged label Jun 12, 2024

majetideepak deleted the centos9-support branch June 21, 2024 06:54

Add support for Centos9 Stream + GCC12 #9903

Add support for Centos9 Stream + GCC12 #9903

Conversation

majetideepak commented May 23, 2024 • edited Loading

netlify bot commented May 23, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

assignUser left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czentgr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majetideepak commented May 30, 2024

assignUser commented May 31, 2024

wanglinsong commented Jun 3, 2024

assignUser commented Jun 4, 2024

majetideepak commented Jun 4, 2024 • edited Loading

kgpai commented Jun 4, 2024

majetideepak commented Jun 4, 2024

majetideepak commented Jun 4, 2024

majetideepak commented Jun 4, 2024

assignUser commented Jun 4, 2024

tigrux commented Jun 4, 2024

majetideepak commented Jun 4, 2024

assignUser commented Jun 5, 2024

majetideepak commented Jun 5, 2024

majetideepak commented Jun 5, 2024

majetideepak commented Jun 6, 2024

majetideepak commented Jun 6, 2024

assignUser left a comment

Choose a reason for hiding this comment

kgpai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 12, 2024

facebook-github-bot commented Jun 12, 2024

conbench-facebook bot commented Jun 12, 2024

majetideepak commented May 23, 2024 •

edited

Loading

netlify bot commented May 23, 2024 •

edited

Loading

assignUser left a comment •

edited

Loading

majetideepak commented Jun 4, 2024 •

edited

Loading