[VL] Use the dynamic-link image for the dev container - #12778
Conversation
There was a problem hiding this comment.
Pull request overview
Updates Gluten’s Velox dev container to use the dynamically linked CentOS 9 image and moves setup work into a lightweight post-create script, so container creation is fast and doesn’t trigger a long native build. Documentation is rewritten to match the new workflow and provide correct build/test commands and sizing guidance.
Changes:
- Switch
.devcontainer/devcontainer.jsonfrom the static-link vcpkg image toapache/gluten:centos-9-jdk8, add host requirements, mounts, GCC toolset env, and watcher excludes. - Add
.devcontainer/post-create.shto install missing dev tools (JDK 17, clang-format 15, regex), size/exportNUM_THREADS, and print recommended commands. - Rewrite
docs/developers/dev-container.mdto document the new image choice, non-automatic native build, and correct build/test invocations.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| docs/developers/dev-container.md | Rewrites dev-container docs to match the dynamic-link image workflow and correct commands/constraints. |
| .devcontainer/post-create.sh | New post-create setup script for tool installation, NUM_THREADS sizing, and guidance output. |
| .devcontainer/devcontainer.json | Switches devcontainer to dynamic image and adds env/mounts/hostRequirements and editor excludes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ca8918e to
cf01f2e
Compare
@felipepessoto, according to my understanding, vcpkg maintains its local binary cache per port, so it usually skips building unchanged dependencies and only rebuilds modified ones. Exceptional cases do happen, for instance, after a GCC upgrade, cached binaries won't match the output of the new toolchain, but this is relatively rare. |
Yes. We noted that vcpkg is a bit sensitive to the system environment. The problem is that once the environment is changed, vcpkg silently rebuilds the packages but doesn't show the reason. The only way is to try a fresh image and then add the script one by one. |
postCreateCommand ran dev/ci-velox-buildstatic-centos-9.sh on top of apache/gluten:vcpkg-centos-9. That is a CI packaging script, and running it at container creation is a poor fit: - It sets NUM_THREADS=2, which is right for a CI runner and wrong for a workstation: the whole Velox build is pinned to 2-way parallelism no matter how many cores the container has. This is the main reason it runs for hours. - It also enables S3, GCS, HDFS, ABFS and static linking, which serve release packaging rather than day-to-day development. - The static link step is memory hungry; Velox.md recommends 64 GB. - vcpkg caches per port, so a moved checkout normally rebuilds only what it changed, but the ABI hash also covers the toolchain: an environment change invalidates every port at once and vcpkg does not say why. In a postCreateCommand that looks like a container hanging with no output. - A postCreateCommand that runs for hours stalls container creation and leaves a half-built tree behind whenever the editor disconnects or a Codespace times out. Static linking exists to produce portable release jars, which a developer never needs. Switch to apache/gluten:centos-9-jdk8, the dynamically linked image the cpp/UDF test job already runs on: it ships Velox's third-party libraries and Arrow under /usr/local, a pre-warmed Maven repository, and the Spark distributions the unit tests need under /opt/shims. The vcpkg image never runs install-spark-resources.sh, so gluten-ut cannot run there at all, and it ships JDK 17 only. The native build is no longer automatic. postCreateCommand now runs .devcontainer/post-create.sh, which: - installs JDK 17 alongside JDK 8, needed by Spark 4.x and by the Spark unit tests, which CI runs on JDK 17; - installs clang-format 15, which dev/format-cpp-code.sh requires under that exact name and tries to install with apt, and the regex module dev/check.py imports; - sizes NUM_THREADS from memory rather than cores, and exports it; - prints the build and test commands to run. The NUM_THREADS sizing is what keeps a build from being OOM-killed. The build scripts default it to "nproc --ignore=2" while Velox's heavier translation units peak at roughly 3.5 GB resident each, so on a 32-core, 62 GB container the default asks for 30 jobs and about 100 GB, and the OOM killer takes down the build and the container with it. Reserving a few GB and allowing ~4 GB per job gives 13 jobs there. The value is recomputed per shell so it follows a resized Codespace. devcontainer.json also: - puts GCC 12 on PATH through remoteEnv, since the image's default GCC 11 cannot compile Velox's C++20 sources and the build scripts do not enable the toolset themselves; - declares hostRequirements, where the 64 GB storage floor is what rules out machine types too small for the image plus the Velox build tree; - keeps the ccache and the Maven repository in named volumes so they survive a rebuild; - excludes the build trees from the editor's file watcher, which otherwise exhausts inotify handles once a build has produced more than 10 GB of artifacts. docs/developers/dev-container.md covers all of it, including that the test command must not use "-pl gluten-ut": gluten-ut is an aggregator POM, "-pl" does not select its children, and the build then finishes in seconds with no suite in the reactor. Generated-by: GitHub Copilot CLI claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
cf01f2e to
3992572
Compare
|
@philo-he @FelixYBW you are both right, and my wording in the description was wrong. Thanks for catching it. I had described vcpkg's caching as all-or-nothing against the manifest. It isn't: the ABI hash is computed per port, and What is left after dropping that claim still argues for the change, and I think it is a better argument than the one I made:
Independently of vcpkg, Static linking is still the right default for release jars, and the doc keeps instructions for switching back to the vcpkg image when someone needs to reproduce a static-link problem. The comparison table now says the cache works per port and flags the toolchain caveat, rather than claiming a full rebuild. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.devcontainer/post-create.sh:58
- After
pip3 install clang-format==15.0.7succeeds, the script assumesclang-formatis now discoverable on PATH and silently does nothing if it is not (leavingclang-format-15missing with no warning). It would be safer to warn whenclang-formatcannot be found after a successful install, so failures don’t show up much later when./dev/format-cpp-code.shis run.
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
|
@felipepessoto would you do a check if we could adopt to use multiple dev containers here? |
@zhouyuan I checked it, and multiple configurations are supported. Alternatives can be placed under Multiple configurations do not change why this PR replaces the current default. The current configuration uses the static-link vcpkg image and runs a CI packaging script during For this PR, I suggest keeping the validated Velox dynamic-link configuration as the default. If a static-link alternative is needed, it should be redesigned for development rather than preserve the current configuration unchanged:
I have not evaluated ClickHouse or GPU configurations because I am not familiar enough with those backends to define and validate their environments reliably. Those configurations would need input and ownership from contributors who work on those backends. |
What changes are proposed in this pull request?
Follow-up to #12125, which added the Dev Container. As configured there, the container runs
dev/ci-velox-buildstatic-centos-9.shas itspostCreateCommandon top ofapache/gluten:vcpkg-centos-9. That is a CI packaging script, and running it at container creation is a poor fit:export NUM_THREADS=2. That is right for a CI runner and wrong for a workstation: it pins the whole Velox build to 2-way parallelism no matter how many cores the container has. This is the main reason it runs for hours.--enable_s3 --enable_gcs --enable_hdfs --enable_abfsand static linking, which serve release packaging rather than day-to-day development.Velox.mdrecommends 64 GB.postCreateCommandthat looks like a container hanging with no output.postCreateCommandthat runs for hours stalls container creation and leaves a half-built tree behind whenever the editor disconnects or a Codespace times out.Static linking exists to produce portable release jars, which a developer never needs. This switches to
apache/gluten:centos-9-jdk8, the dynamically linked image the cpp/UDF test job already runs on: it ships Velox's third-party libraries and Arrow under/usr/local, a pre-warmed Maven repository, and the Spark distributions the unit tests need under/opt/shims. The vcpkg image never runsinstall-spark-resources.sh, sogluten-utcannot run there at all, and it ships JDK 17 only.The native build is no longer automatic.
postCreateCommandnow runs.devcontainer/post-create.sh, which:dev/format-cpp-code.shrequires under that exact name and tries to install withapt, and theregexmoduledev/check.pyimports;NUM_THREADSfrom memory rather than cores, and exports it;The
NUM_THREADSsizing is what keeps a build from being OOM-killed. The build scripts default it tonproc --ignore=2while Velox's heavier translation units peak at roughly 3.5 GB resident each, so on a 32-core, 62 GB container the default asks for 30 jobs and about 100 GB, and the OOM killer takes down the build and the container with it. Reserving a few GB and allowing ~4 GB per job gives 13 jobs there. The value is recomputed per shell, so it follows a resized Codespace.devcontainer.jsonalso:PATHthroughremoteEnv, since the image's default GCC 11 cannot compile Velox's C++20 sources and the build scripts do not enable the toolset themselves;hostRequirements, where the 64 GB storage floor is what rules out machine types too small for the image plus the Velox build tree;docs/developers/dev-container.mdis rewritten to match, including a warning that the unit test command must not use-pl gluten-ut:gluten-utis an aggregator POM,-pldoes not select its children, and the build then finishes in seconds with no suite in the reactor.How was this patch tested?
Dev Container configuration and documentation only; no product code is touched.
Validated from inside the container that the documented commands work end to end:
cpp/build,ep/build-velox/build/velox_ep/_buildandep/_ep, using the documented command:BUILD SUCCESS, producingcpp/build/releases/libgluten.so,libvelox.soandpackage/target/gluten-package-1.8.0-SNAPSHOT-3.5.jar.cd cpp/build && ctest: 5675 of 5675 tests passed, confirming--build_tests=ONproduces working test binaries.Gluten Unit Test Spark35in the Maven reactor, where the previous-pl gluten-utform selected only the aggregator POM and ran no suite.Was this patch authored or co-authored using generative AI tooling?
Generated-by: GitHub Copilot CLI claude-opus-5