Copilot commented on code in PR #12778: URL: https://github.com/apache/gluten/pull/12778#discussion_r3787097204
########## .devcontainer/post-create.sh: ########## @@ -0,0 +1,134 @@ +#!/usr/bin/env bash + +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Dev Container postCreateCommand. +# +# This only prepares the environment; it never builds Velox. A Velox build takes +# tens of minutes to several hours, and a postCreateCommand that long stalls +# container creation and leaves a half-built tree behind when the editor +# disconnects or Codespaces times out. Run the build yourself once the container +# is up -- the command is printed at the end of this script. + +set -uo pipefail + +NUM_THREADS_MARKER='# >>> gluten dev container num_threads >>>' + +warn() { echo "WARNING: $*" >&2; } + +echo "Preparing the Gluten dev container..." + +# Spark 4.0/4.1 and the UDF tests need JDK 17, which this JDK 8 image lacks. +# Both JDKs can coexist: JAVA_HOME still points at JDK 8 for the default build. +if [ ! -d /usr/lib/jvm/java-17-openjdk ]; then + echo "Installing JDK 17 alongside JDK 8 (needed for Spark 4.x)..." + dnf install -y --setopt=install_weak_deps=False java-17-openjdk-devel >/dev/null || + warn "could not install JDK 17; Spark 4.x builds will not work until it is installed." +fi + +# dev/format-cpp-code.sh requires a binary literally named clang-format-15, and +# tries to install it with apt, which does not exist on CentOS. +if ! command -v clang-format-15 >/dev/null 2>&1; then + echo "Installing clang-format 15..." + if pip3 install --quiet --retries 1 clang-format==15.0.7; then + CLANG_FORMAT=$(command -v clang-format) + if [ -n "$CLANG_FORMAT" ]; then + ln -sf "$CLANG_FORMAT" /usr/local/bin/clang-format-15 + fi + else + warn "could not install clang-format 15; ./dev/format-cpp-code.sh will not run." + fi +fi + +# dev/check.py and .github/workflows/util/license-header.py import regex. +if ! python3 -c "import regex" >/dev/null 2>&1; then + echo "Installing the regex module..." + pip3 install --quiet --retries 1 regex || + warn "could not install the regex module; ./dev/check.py will not run." +fi Review Comment: `post-create.sh` relies on `pip3` to install `clang-format==15.0.7` and `regex`, but the CentOS 9 dynamic-build image Dockerfile doesn’t install `python3-pip`. If `pip3` is missing, these installs will fail and the script will only warn, leaving `clang-format-15` and `regex` unavailable (breaking `dev/format-cpp-code.sh` and `dev/check.py`). Consider ensuring `pip3` is present via `dnf install python3-pip` before any pip-based installs. ########## .devcontainer/post-create.sh: ########## @@ -0,0 +1,134 @@ +#!/usr/bin/env bash + +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Dev Container postCreateCommand. +# +# This only prepares the environment; it never builds Velox. A Velox build takes +# tens of minutes to several hours, and a postCreateCommand that long stalls +# container creation and leaves a half-built tree behind when the editor +# disconnects or Codespaces times out. Run the build yourself once the container +# is up -- the command is printed at the end of this script. + +set -uo pipefail + +NUM_THREADS_MARKER='# >>> gluten dev container num_threads >>>' + +warn() { echo "WARNING: $*" >&2; } + +echo "Preparing the Gluten dev container..." + +# Spark 4.0/4.1 and the UDF tests need JDK 17, which this JDK 8 image lacks. +# Both JDKs can coexist: JAVA_HOME still points at JDK 8 for the default build. +if [ ! -d /usr/lib/jvm/java-17-openjdk ]; then + echo "Installing JDK 17 alongside JDK 8 (needed for Spark 4.x)..." + dnf install -y --setopt=install_weak_deps=False java-17-openjdk-devel >/dev/null || + warn "could not install JDK 17; Spark 4.x builds will not work until it is installed." +fi + +# dev/format-cpp-code.sh requires a binary literally named clang-format-15, and +# tries to install it with apt, which does not exist on CentOS. +if ! command -v clang-format-15 >/dev/null 2>&1; then + echo "Installing clang-format 15..." + if pip3 install --quiet --retries 1 clang-format==15.0.7; then + CLANG_FORMAT=$(command -v clang-format) + if [ -n "$CLANG_FORMAT" ]; then + ln -sf "$CLANG_FORMAT" /usr/local/bin/clang-format-15 + fi + else + warn "could not install clang-format 15; ./dev/format-cpp-code.sh will not run." + fi +fi + +# dev/check.py and .github/workflows/util/license-header.py import regex. +if ! python3 -c "import regex" >/dev/null 2>&1; then + echo "Installing the regex module..." + pip3 install --quiet --retries 1 regex || + warn "could not install the regex module; ./dev/check.py will not run." +fi + +# Cap build parallelism by memory, not just by core count. +# dev/builddeps-veloxbe.sh defaults NUM_THREADS to "nproc --ignore=2", which +# ignores memory entirely. Velox's heavier translation units peak at roughly +# 3.5 GB of resident memory each, so on a machine with many cores relative to +# its RAM the default oversubscribes memory badly: on 32 cores / 62 GB it asks +# for 30 jobs, about 100 GB, and the OOM killer takes down the build or the +# whole container. Reserve a few GB for the editor, Maven and the OS, allow +# about 4 GB per job, and never exceed the CPU-based default. +CPU_THREADS=$(nproc --ignore=2) +[ "$CPU_THREADS" -lt 1 ] && CPU_THREADS=1 +MEM_GB=$(awk '/^MemTotal:/ {printf "%d", $2 / 1024 / 1024}' /proc/meminfo 2>/dev/null) +MEM_THREADS=$(( (${MEM_GB:-0} - 8) / 4 )) +[ "$MEM_THREADS" -lt 1 ] && MEM_THREADS=1 +if [ "$MEM_THREADS" -lt "$CPU_THREADS" ]; then + NUM_THREADS=$MEM_THREADS +else + NUM_THREADS=$CPU_THREADS +fi + +# Export it so a plain "./dev/buildbundle-veloxbe.sh", as documented in +# docs/get-started/Velox.md, is memory-safe too and not just the command printed +# below. Written to ~/.bashrc rather than devcontainer.json's containerEnv +# because the value depends on the machine the container was placed on. +if ! grep -qF "$NUM_THREADS_MARKER" "$HOME/.bashrc" 2>/dev/null; then + cat >>"$HOME/.bashrc" <<EOF + +$NUM_THREADS_MARKER +# Velox compiles need ~4 GB per job; the build scripts size NUM_THREADS from the +# core count alone, which the OOM killer punishes on core-rich machines. +export NUM_THREADS=\${NUM_THREADS:-$NUM_THREADS} +# <<< gluten dev container num_threads <<< +EOF +fi Review Comment: `NUM_THREADS` is only written to `~/.bashrc` once (guarded by the marker). If the container is rebuilt on a different machine size (common with Codespaces), the already-written value can become stale and reintroduce OOM risk. It’s safer to delete any existing marker block and rewrite it on every post-create run so it always matches the current machine’s memory. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
