felipepessoto commented on code in PR #12778: URL: https://github.com/apache/gluten/pull/12778#discussion_r3787133590
########## .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: Applied, though the premise needed checking first: `pip3` **is** present in `apache/gluten:centos-9-jdk8` (`pip 21.3.1 from /usr/lib/python3.9/site-packages`, the CentOS 9 distro RPM). It arrives transitively rather than through an explicit `dnf install`, so nothing is broken today. The underlying point stands, though. Two of the script's steps depend on `pip3`, and if it ever stopped being pulled in the failure would be quiet: a warning scrolls past at create time and `dev/format-cpp-code.sh` only breaks much later. Added a guard in the same "install what the image lacks" style the script already uses for JDK 17 and clang-format. ########## .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: The diagnosis is right, but the suggested fix would not have worked, so I fixed it a different way. `postCreateCommand` runs only at container creation, and `~/.bashrc` is not on a persisted volume (only `/root/.ccache` and `/root/.m2` are). On a rebuild the file is therefore already fresh from the image and the marker is absent, so rewriting the block on every post-create run would change nothing in exactly the case where it runs. Where staleness actually bites is a Codespaces machine-type change, which is a stop/start: the container filesystem survives, `postCreateCommand` does **not** re-run, and a baked-in constant persists. Resizing 32 GB down to 16 GB would keep 6 jobs on a machine sized for 2 — the OOM risk you describe. Fixed by making the value dynamic rather than baked in. `post-create.sh` now installs a `gluten-num-threads` helper, and `~/.bashrc` contains: ```bash export NUM_THREADS=${NUM_THREADS:-$(gluten-num-threads)} ``` It is recomputed per shell, so it follows the machine with no re-run needed, the formula lives in one place, and an explicit `NUM_THREADS` still wins. Verified across sizes: 16 GB gives 2 jobs, 32 GB gives 6, 62 GB gives 13, and smaller machines clamp to 1. -- 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]
