Skip to content

Conversation

@scal444
Copy link
Collaborator

@scal444 scal444 commented Nov 10, 2025

No description provided.

@scal444
Copy link
Collaborator Author

scal444 commented Nov 10, 2025

/build

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Overview

Greptile Summary

Migrated CI infrastructure from GitLab Runner to Jenkins with Blossom integration for PR automation. The new pipeline runs three parallel jobs: a QA pipeline (format checking, linting, static analysis) and two build/test jobs for CUDA 12.6 (oldest supported) and CUDA 13.0 (newest supported) with different Python/RDKit version combinations.

Key Changes:

  • Replaced GitLab-based CI with Jenkins pipeline using NVIDIA Blossom GitHub integration
  • Added parallel testing across CUDA 12.6 (Ubuntu 22.04) and CUDA 13.0 (Ubuntu 24.04) containers
  • Created setup_conda_env.sh to install Miniforge and configure Python/RDKit environments
  • Created run_qa.sh to consolidate QA checks (clang-format, cmake-format, clang-tidy, cppcheck, iwyu)
  • Archived old Jenkinsfile as old_JENkinsfile for reference

Critical Issue:
The two build/test stages (build_test_oldest_supported and build_test_newest_supported) contain placeholder comments indicating incomplete implementation, but they report SUCCESS status despite not running any actual build or test logic. This creates a false sense of passing CI.

Confidence Score: 2/5

  • Not safe to merge - two critical test stages report false success without running tests
  • The pipeline has placeholder logic in two of three parallel stages that report SUCCESS without executing any build or test commands. This means the CI will falsely report passing tests when no validation has occurred. Only the QA pipeline appears functional.
  • Jenkinsfile requires immediate attention - lines 76 and 92 need actual build/test implementation before merging

Important Files Changed

File Analysis

Filename Score Overview
Jenkinsfile 2/5 New Jenkins CI pipeline with parallel testing across CUDA 12/13, but two build stages have placeholder logic that report SUCCESS without running any tests
admin/run_qa.sh 4/5 New QA script that runs formatting, linting, and static analysis tools; missing newline at EOF
admin/setup_conda_env.sh 3/5 New script to setup conda environment with specific Python/RDKit versions; installs conda unconditionally which may be inefficient

Sequence Diagram

sequenceDiagram
    participant GH as GitHub PR
    participant Jenkins as Jenkins Pipeline
    participant CUDA12 as CUDA 12.6 Container
    participant CUDA13 as CUDA 13.0 Container
    
    GH->>Jenkins: Trigger PR Build
    Jenkins->>Jenkins: Get GitHub Token
    Jenkins->>Jenkins: Checkout PR Code
    
    par Parallel Testing
        Jenkins->>CUDA13: QA Pipeline
        CUDA13->>CUDA13: setup_conda_env.sh (Python 3.13, RDKit 2025.3.1)
        CUDA13->>CUDA13: run_qa.sh (format, lint, tidy, cppcheck, iwyu)
        CUDA13->>Jenkins: Report QA Status
    and
        Jenkins->>CUDA12: Build Test (Oldest)
        CUDA12->>CUDA12: setup_conda_env.sh (Python 3.10, RDKit 2024.9.6)
        Note over CUDA12: Build/Test Logic Missing (TODO)
        CUDA12->>Jenkins: Report Status
    and
        Jenkins->>CUDA13: Build Test (Newest)
        CUDA13->>CUDA13: setup_conda_env.sh (Python 3.13, RDKit 2025.3.1)
        Note over CUDA13: Build/Test Logic Missing (TODO)
        CUDA13->>Jenkins: Report Status
    end
    
    Jenkins->>Jenkins: Upload Logs to GitHub
    Jenkins->>GH: Update Commit Status
Loading

5 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

println "Setting up conda environment: Python 3.10, RDKit 2024.9.6"
sh 'cd ${WORKSPACE} && bash admin/setup_conda_env.sh 3.10 2024.9.6'
println "Running build and test on oldest supported CUDA"
// Build and test logic for CUDA 12.6 will be added here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Pipeline reports success despite having no actual build/test logic - the comment indicates work is incomplete but the stage still marks as SUCCESS

println "Setting up conda environment: Python 3.13, RDKit 2025.3.1"
sh 'cd ${WORKSPACE} && bash admin/setup_conda_env.sh 3.13 2025.3.1'
println "Running build and test on newest supported CUDA"
// Build and test logic for CUDA 13.0 will be added here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Pipeline reports success despite having no actual build/test logic - the comment indicates work is incomplete but the stage still marks as SUCCESS

bash admin/run_cmake_format_lint.sh -d
bash admin/run_clang_tidy.sh
bash admin/run_cppcheck.sh
bash admin/run_iwyu.sh No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syntax: Missing newline at end of file (POSIX requirement)

Suggested change
bash admin/run_iwyu.sh
bash admin/run_iwyu.sh

Comment on lines +9 to +12
mkdir -p /var/tmp && wget -q -nc --no-check-certificate -P /var/tmp https://github.com/conda-forge/miniforge/releases/download/25.3.0-3/Miniforge3-25.3.0-3-Linux-x86_64.sh && \
bash /var/tmp/Miniforge3-25.3.0-3-Linux-x86_64.sh -b -p /usr/local/anaconda && \
/usr/local/anaconda/bin/conda init && \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Installs conda unconditionally every run without checking if it already exists, which wastes time and may cause conflicts

@scal444
Copy link
Collaborator Author

scal444 commented Nov 25, 2025

/build

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 25, 2025

Greptile Overview

Greptile Summary

Migrates CI infrastructure from GitLab to Jenkins with parallel CUDA 12/13 builds and integrated QA pipeline. The Jenkinsfile sets up conda environments with different Python/RDKit versions and runs build/test stages, though the QA pipeline execution is currently commented out (line 94).

Key changes:

  • New Jenkins pipeline with parallel testing across CUDA 12.6 (oldest) and CUDA 13.0 (newest) supported versions
  • Conda environment setup script supporting configurable Python and RDKit versions
  • QA orchestration scripts for clang-format, clang-tidy, cppcheck, and IWYU
  • Removed old GitLab CI runner script (admin/run_ci.sh)
  • Minor test cleanup: removed unused parameter from makeAdjacency function

Observations:

  • QA pipeline stage reports success without actually running checks (line 94 commented out)
  • Previous review concerns about missing dependencies and environment variables appear to still be present but are mitigated by the QA stage being disabled

Confidence Score: 3/5

  • Safe for experimental CI testing but QA pipeline is incomplete
  • The PR title indicates this is experimental ("CI test 4"), and the build/test stages appear functional. However, the QA pipeline is commented out and would fail if enabled due to unresolved dependency and environment issues from previous reviews. For experimental CI setup this is acceptable, but production readiness requires addressing the commented-out QA stage.
  • Jenkinsfile requires attention - QA pipeline is disabled and needs completion before production use

Important Files Changed

File Analysis

Filename Score Overview
Jenkinsfile 3/5 New Jenkins CI pipeline with parallel CUDA 12/13 builds and QA stage (currently commented out)
admin/setup_conda_env.sh 4/5 Conda environment setup script for different Python/RDKit versions
admin/run_qa.sh 4/5 QA orchestration script for running format checks, linting, and static analysis tools
admin/install_qa_dependencies.sh 4/5 Installs clang-17 toolchain and cppcheck 2.14 for QA pipeline
admin/run_cmake_format_lint.sh 3/5 Changed from git-based root detection to pwd (may break when run from subdirs)

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7 files reviewed, 5 comments

Edit Code Review Agent Settings | Greptile

Jenkinsfile Outdated
sh 'cd ${WORKSPACE} && bash admin/setup_conda_env.sh 3.10 2024.9.6'
println "Running build and test on oldest supported CUDA"
// Build and test logic for CUDA 12.6 will be added here
mkdir /build && cd /build && cmake ${WORKSPACE} && make -j && ctest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syntax: Missing sh wrapper - bare shell command will fail

Suggested change
mkdir /build && cd /build && cmake ${WORKSPACE} && make -j && ctest
sh 'mkdir /build && cd /build && cmake ${WORKSPACE} && make -j && ctest'

@@ -0,0 +1,9 @@
#!/bin/bash

source /usr/local/anaconda/etc/profile.d/conda.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Sourcing conda without activating makes no change - set -ex immediately follows, but conda environment is never activated

Comment on lines +5 to +9
bash admin/run_clang_format.sh -d
bash admin/run_cmake_format_lint.sh -d
bash admin/run_clang_tidy.sh
bash admin/run_cppcheck.sh
bash admin/run_iwyu.sh No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: QA tools (clang-format-17, clang-tidy-17, cppcheck 2.14) are not installed - these scripts will fail without admin/install_qa_dependencies.sh being called first

Jenkinsfile Outdated
Comment on lines 58 to 60
sh 'cd ${WORKSPACE} && bash admin/setup_conda_env.sh 3.13 2025.3.1'
println "Running QA pipeline"
sh 'cd ${WORKSPACE} && bash admin/run_qa.sh'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: QA pipeline will fail - run_qa.sh depends on clang-17 and cppcheck 2.14, but install_qa_dependencies.sh is never called

. /usr/local/anaconda/etc/profile.d/conda.sh && \
conda activate base && \
conda config --add channels conda-forge --add channels nvidia --add channels pytorch && \
conda install -y python=$PYTHON_VERSION rdkit=$RDKIT_VERSION libboost-devel libboost-headers libboost-python-devel librdkit-dev pytest pandas psutil cmake cmakelang scikit-build=0.18 eigen No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Missing RDKIT_PATH export - QA scripts expect this variable (used in run_clang_tidy.sh:46, run_cppcheck.sh:37, run_iwyu.sh:26)

@scal444
Copy link
Collaborator Author

scal444 commented Nov 25, 2025

/build

@scal444
Copy link
Collaborator Author

scal444 commented Nov 25, 2025

/build

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

Jenkinsfile Outdated
sh 'cd ${WORKSPACE} && bash admin/setup_conda_env.sh 3.10 2024.9.6'
println "Running build and test on oldest supported CUDA"
// Build and test logic for CUDA 12.6 will be added here
mkdir /build && cd /build && cmake ${WORKSPACE} && make -j && ctest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syntax: Missing sh wrapper - this bare command will cause pipeline failure

Suggested change
mkdir /build && cd /build && cmake ${WORKSPACE} && make -j && ctest
sh 'mkdir /build && cd /build && cmake ${WORKSPACE} && make -j && ctest'

. /usr/local/anaconda/etc/profile.d/conda.sh && \
conda activate base && \
conda config --add channels conda-forge --add channels nvidia --add channels pytorch && \
conda install -q -y python=$PYTHON_VERSION rdkit=$RDKIT_VERSION libboost-devel libboost-headers libboost-python-devel librdkit-dev pytest pandas psutil cmake cmakelang scikit-build=0.18 eigen No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Missing RDKIT_PATH export - QA scripts (run_clang_tidy.sh:46, run_cppcheck.sh:37, run_iwyu.sh:26) require this variable to locate RDKit headers

Suggested change
conda install -q -y python=$PYTHON_VERSION rdkit=$RDKIT_VERSION libboost-devel libboost-headers libboost-python-devel librdkit-dev pytest pandas psutil cmake cmakelang scikit-build=0.18 eigen
conda install -q -y python=$PYTHON_VERSION rdkit=$RDKIT_VERSION libboost-devel libboost-headers libboost-python-devel librdkit-dev pytest pandas psutil cmake cmakelang scikit-build=0.18 eigen && \
export RDKIT_PATH=/usr/local/anaconda

Jenkinsfile Outdated
Comment on lines 58 to 60
sh 'cd ${WORKSPACE} && bash admin/setup_conda_env.sh 3.13 2025.3.1'
println "Running QA pipeline"
sh 'cd ${WORKSPACE} && bash admin/run_qa.sh'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: QA pipeline will fail - run_qa.sh requires clang-17 and cppcheck 2.14, but install_qa_dependencies.sh is never called

Suggested change
sh 'cd ${WORKSPACE} && bash admin/setup_conda_env.sh 3.13 2025.3.1'
println "Running QA pipeline"
sh 'cd ${WORKSPACE} && bash admin/run_qa.sh'
println "Setting up conda environment: Python 3.13, RDKit 2025.3.1"
sh 'cd ${WORKSPACE} && bash admin/setup_conda_env.sh 3.13 2025.3.1'
println "Installing QA dependencies"
sh 'cd ${WORKSPACE} && bash admin/install_qa_dependencies.sh'
println "Running QA pipeline"
sh 'cd ${WORKSPACE} && bash admin/run_qa.sh'

@scal444
Copy link
Collaborator Author

scal444 commented Nov 25, 2025

/build

@scal444
Copy link
Collaborator Author

scal444 commented Nov 25, 2025

/build

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

done

ROOT_DIR=$(git rev-parse --show-toplevel)
ROOT_DIR=$(pwd)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Using pwd instead of git rev-parse --show-toplevel breaks when script is called from subdirectories - run_qa.sh calls this without changing to repo root first

mkdir -p build && cd build && cmake -DCMAKE_INSTALL_PREFIX=/usr/local -DUSE_MATCHCOMPILER=ON -DCMAKE_BUILD_TYPE=release .. && \
cmake --build $(pwd) --target all -- -j$(nproc) && \
cmake --build $(pwd) --target install -- -j$(nproc) && \
rm -rf /var/tmp/cppcheck No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syntax: Missing newline at end of file (POSIX requirement)

sh 'cd ${WORKSPACE} && bash admin/setup_conda_env.sh 3.10 2024.9.6'
println "Running build and test on oldest supported CUDA"
// Build and test logic for CUDA 12.6 will be added here
sh '. /usr/local/anaconda/etc/profile.d/conda.sh && conda activate base && mkdir /build && cd /build && cmake ${WORKSPACE} && make -j && ctest'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: mkdir /build will fail on second run - directory already exists and mkdir without -p flag errors out

sh 'cd ${WORKSPACE} && bash admin/setup_conda_env.sh 3.13 2025.3.1'
println "Running build and test on newest supported CUDA"
// Build and test logic for CUDA 13.0 will be added here
sh '. /usr/local/anaconda/etc/profile.d/conda.sh && conda activate base && mkdir /build && cd /build && cmake ${WORKSPACE} && make -j && ctest'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: mkdir /build will fail on second run - directory already exists and mkdir without -p flag errors out

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@scal444
Copy link
Collaborator Author

scal444 commented Nov 25, 2025

/build

7 similar comments
@scal444
Copy link
Collaborator Author

scal444 commented Nov 25, 2025

/build

@scal444
Copy link
Collaborator Author

scal444 commented Nov 25, 2025

/build

@scal444
Copy link
Collaborator Author

scal444 commented Nov 25, 2025

/build

@scal444
Copy link
Collaborator Author

scal444 commented Nov 26, 2025

/build

@scal444
Copy link
Collaborator Author

scal444 commented Nov 26, 2025

/build

@scal444
Copy link
Collaborator Author

scal444 commented Dec 3, 2025

/build

@scal444
Copy link
Collaborator Author

scal444 commented Dec 3, 2025

/build

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant