Repository navigation
Dockerfiles: Update S3 SDK version - #1387
Conversation
There is a bug in the currently used SDK 1.11.581 where the CrtCallback uses data after it was freed. SDK version 1.11.760 fixes this. Update dockerfiles to use latest version. Signed-off-by: Adit Ranadive <aranadive@nvidia.com>
|
👋 Hi aranadive! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
|
/build |
📝 WalkthroughWalkthroughThree Dockerfile files are updated to change the AWS SDK C++ Git branch reference from 1.11.581 to 1.11.760. All other build steps and cloning parameters remain unchanged. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
benchmark/nixlbench/contrib/Dockerfile (1)
156-161: 🧹 Nitpick | 🔵 TrivialRefactor this build step to avoid
cdinRUNand align with Hadolint DL3003 guidance.The current pattern uses
cdinside aRUNchain, which Hadolint flags asDL3003. Switch to CMake's-S/-Bflags pluscmake --build/--installto keep the step deterministic and maintainable, consistent with the Azure SDK build pattern already used in this Dockerfile (lines 169–173).♻️ Proposed refactor
RUN git clone --recurse-submodules --depth 1 --shallow-submodules https://github.com/aws/aws-sdk-cpp.git --branch 1.11.760 && \ - mkdir sdk_build && \ - cd sdk_build && \ - cmake ../aws-sdk-cpp/ -DCMAKE_BUILD_TYPE=Release -DBUILD_ONLY="s3;s3-crt" -DENABLE_TESTING=OFF -DCMAKE_INSTALL_PREFIX=/usr/local && \ - make -j && \ - make install + cmake -S aws-sdk-cpp -B sdk_build -DCMAKE_BUILD_TYPE=Release -DBUILD_ONLY="s3;s3-crt" -DENABLE_TESTING=OFF -DCMAKE_INSTALL_PREFIX=/usr/local && \ + cmake --build sdk_build -j${NPROC:-$(nproc)} && \ + cmake --install sdk_build🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/nixlbench/contrib/Dockerfile` around lines 156 - 161, Refactor the Dockerfile RUN step that builds aws-sdk-cpp to avoid using cd by using CMake's source/build directories and its --build/--install verbs: replace the chained commands that clone aws-sdk-cpp and then "mkdir sdk_build && cd sdk_build && cmake ../aws-sdk-cpp/ ..." with a sequence that clones the repo (same URL and --branch 1.11.760 and submodule flags) and then invokes cmake -S aws-sdk-cpp -B sdk_build -DCMAKE_BUILD_TYPE=Release -DBUILD_ONLY="s3;s3-crt" -DENABLE_TESTING=OFF -DCMAKE_INSTALL_PREFIX=/usr/local followed by cmake --build sdk_build -- -j and cmake --install sdk_build; keep the same build options (BUILD_ONLY/ENABLE_TESTING/CMAKE_INSTALL_PREFIX) and ensure the directory names (aws-sdk-cpp, sdk_build) match the existing diff so Hadolint DL3003 is resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contrib/Dockerfile`:
- Around line 98-101: The Dockerfile RUN chain uses shell cd to switch into
aws_sdk_build which triggers DL3003; replace the cd-based build with explicit
CMake source and build directory flags instead: create the build directory
(aws_sdk_build) via mkdir and invoke cmake pointing to ../aws-sdk-cpp as the
source and the build dir as the current working directory using -S and -B (or
pass the build dir as the first argument) so you call cmake -S aws-sdk-cpp -B
aws_sdk_build ... (matching existing flags -DCMAKE_BUILD_TYPE=Release
-DBUILD_ONLY="s3;s3-crt" -DENABLE_TESTING=OFF
-DCMAKE_INSTALL_PREFIX=/usr/local), then run make -C aws_sdk_build
-j${NPROC:-$(nproc)} and make -C aws_sdk_build install; adjust the git clone and
directory names (aws-sdk-cpp, aws_sdk_build) accordingly to avoid using cd.
---
Outside diff comments:
In `@benchmark/nixlbench/contrib/Dockerfile`:
- Around line 156-161: Refactor the Dockerfile RUN step that builds aws-sdk-cpp
to avoid using cd by using CMake's source/build directories and its
--build/--install verbs: replace the chained commands that clone aws-sdk-cpp and
then "mkdir sdk_build && cd sdk_build && cmake ../aws-sdk-cpp/ ..." with a
sequence that clones the repo (same URL and --branch 1.11.760 and submodule
flags) and then invokes cmake -S aws-sdk-cpp -B sdk_build
-DCMAKE_BUILD_TYPE=Release -DBUILD_ONLY="s3;s3-crt" -DENABLE_TESTING=OFF
-DCMAKE_INSTALL_PREFIX=/usr/local followed by cmake --build sdk_build -- -j and
cmake --install sdk_build; keep the same build options
(BUILD_ONLY/ENABLE_TESTING/CMAKE_INSTALL_PREFIX) and ensure the directory names
(aws-sdk-cpp, sdk_build) match the existing diff so Hadolint DL3003 is resolved.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
benchmark/nixlbench/contrib/Dockerfilecontrib/Dockerfilecontrib/Dockerfile.manylinux
There is a bug in the currently used SDK 1.11.581 where the CrtCallback uses data after it was freed. SDK version 1.11.760 fixes this. Update dockerfiles to use latest version. Signed-off-by: Adit Ranadive <aranadive@nvidia.com>
There is a bug in the currently used SDK 1.11.581 where the CrtCallback uses data after it was freed. SDK version 1.11.760 fixes this. Update dockerfiles to use latest version.
See aws/aws-sdk-cpp#3643.