add shellcheck, update pre-commit hooks #738

jameslamb · 2025-03-05T21:18:27Z

Contributes to #667

Expands testing to improve release confidence in these images.

adds new pre-commit hooks:
- rapidsai/verify-copyright = keep copyright notices up to date
- shellcheck = catch issues and enforce consistency in shell scripts
updates all other pre-commit hooks
switches from a manual run: to pre-commit/[email protected] in CI, for faster runs of pre-commit and simpler CI configuration

jameslamb · 2025-03-05T22:25:40Z

The arm64 builds that install cuvs-bench-cpu are failing because of #739

That'll be fixed (and CI unblocked here) once rapidsai/cuvs#750 is merged

jameslamb · 2025-03-05T22:32:25Z

ci/release/update-version.sh

@@ -26,7 +24,7 @@ function sed_runner() {
    sed -i.bak ''"$1"'' $2 && rm -f ${2}.bak
 }

-for FILE in $(find . -name Dockerfile); do
+find . -name Dockerfile | while IFS= read -r -d '' FILE; do


Fixes this from shellcheck:

SC2044 (warning): For loops over find output are fragile. Use find -exec or a while read loop.

jameslamb · 2025-03-05T22:32:40Z

context/test_notebooks.py

-        for notebook in nb_errors.keys():
-            if nb_errors[notebook]:
-                print(f'Errors during {notebook}')
+        for notebook_id, errors in nb_errors.items():


fixes this from ruff:

context/test_notebooks.py:200:9: PLC0206 Extracting value from dictionary without calling `.items()

jameslamb · 2025-03-07T16:59:11Z

context/test_notebooks.py

@@ -35,6 +37,8 @@
    'cuspatial/ZipCodes_Stops_PiP_cuSpatial.ipynb',
    # context on this being skipped: https://github.com/rapidsai/docker/issues/726
    'cuspatial/trajectory_clustering.ipynb',
+    # context on this being skipped: https://github.com/rapidsai/docker/issues/740
+    'cuspatial/Taxi_Dropoff_Reverse_Geocoding.ipynb',


proposing this to unblock CI, see #740

@isVoid I'll start a separate offline thread about our strategy for these notebooks.

jameslamb · 2025-03-07T19:57:39Z

Seeing the one arm test job here fail with this error from cudf:

Jitify fatal error: Uninitialized

(build link)

I think that's related to other ongoing jitify issues cudf has been experiencing on arm the last few days (don't have a great tracking issue, but things that led to PRs like rapidsai/cudf#18175).

jameslamb · 2025-03-07T20:00:01Z

ci/create-cuvs-multiarch-manifest.sh

 set -eEuo pipefail

 common_path="$(dirname "$(realpath "$0")")/common.sh"
 # shellcheck source=common.sh
 source "$common_path"

 cuvs_bench_source_tags=()
-cuvs_bench_datasets_source_tags=()
+# cuvs_bench_datasets_source_tags=()


This and other things I'm commenting out in this file are unused as long as this other code is commented out:

docker/ci/create-cuvs-multiarch-manifest.sh

Lines 27 to 28 in 5b40771

# check_tag_exists "$CUVS_BENCH_DATASETS_IMAGE_REPO" "$full_cuvs_bench_datasets_tag"

# cuvs_bench_datasets_source_tags+=("${org}/${CUVS_BENCH_DATASETS_IMAGE_REPO}:$full_cuvs_bench_datasets_tag")

as it was in #725

The PR mentioned in #725 as a patch was closed, but there is a comment that indicates this might have been fixed in 25.02, though it's unclear, see #723 (review)

Is there an issue or PR in progress tracking this? that we can reference so when it's fixed we can uncomment this back?

Thanks, good point. I just pushed ca8832f, which adds a comment to the issue tracking the cuVS dataset stuff: #724

ncclementi

LGTM, I left a comment about the cuvs commented code. I wasn't able to find an issue or anything suggesting we should fix this, but I think ideally we should leave comment in code referencing an issue on why are we commenting this out.

ncclementi · 2025-03-10T16:09:43Z

ci/create-cuvs-multiarch-manifest.sh

 set -eEuo pipefail

 common_path="$(dirname "$(realpath "$0")")/common.sh"
 # shellcheck source=common.sh
 source "$common_path"

 cuvs_bench_source_tags=()
-cuvs_bench_datasets_source_tags=()
+# cuvs_bench_datasets_source_tags=()


The PR mentioned in #725 as a patch was closed, but there is a comment that indicates this might have been fixed in 25.02, though it's unclear, see #723 (review)

Is there an issue or PR in progress tracking this? that we can reference so when it's fixed we can uncomment this back?

bdice

A few nits, otherwise LGTM.

bdice · 2025-03-18T19:50:00Z

.github/workflows/build-test-publish-images.yml

Should we pin to SHAs of these actions?

If we want to switch from tags to SHAs, I think that'd be better done as a RAPIDS-wide thing and not in the scope of this PR.

I'd like to not have to worry, for this PR, about questions I have about that, such as how that'd affect auto-updates from dependabot / renovate.

I've started a separate (private) discussion about switching to commit SHAs RAPIDS-wide. Let's defer the decision on that to the outcome of that discussion.

I'm going to merge this with these pinned to tags, as other actions in this repo's CI already are.

ci/release/update-version.sh

context/entrypoint.sh

context/test_notebooks.py

KyleFromNVIDIA

Looks good, just a few minor questions and changes.

context/entrypoint.sh

.github/workflows/build-test-publish-images.yml

.pre-commit-config.yaml

Co-authored-by: Bradley Dice <[email protected]>

.pre-commit-config.yaml

jameslamb · 2025-03-19T21:35:30Z

/merge

add shellcheck, update pre-commit hooks

56ad592

jameslamb added improvement non-breaking labels Mar 5, 2025

jameslamb commented Mar 5, 2025

View reviewed changes

jameslamb mentioned this pull request Mar 7, 2025

[BUG] cuspatial/Taxi_Dropoff_Reverse_Geocoding.ipynb failing (RAPIDS 25.04) #740

Open

skip cuspatial/Taxi_Dropoff_Reverse_Geocoding.ipynb

a3711b5

jameslamb commented Mar 7, 2025

View reviewed changes

jameslamb changed the title ~~WIP: add shellcheck, update pre-commit hooks~~ add shellcheck, update pre-commit hooks Mar 7, 2025

jameslamb requested review from jacobtomlinson, raydouglass and ncclementi March 7, 2025 20:00

jameslamb marked this pull request as ready for review March 7, 2025 20:00

jameslamb requested a review from a team as a code owner March 7, 2025 20:00

ncclementi reviewed Mar 10, 2025

View reviewed changes

jameslamb mentioned this pull request Mar 10, 2025

Bump nightly check limit rapidsai/cudf#18213

Merged

3 tasks

jameslamb added 3 commits March 13, 2025 15:16

Merge branch 'branch-25.04' into more-repo-checks

b2c2765

Merge branch 'branch-25.04' into more-repo-checks

62b6c9c

add link to relevant issue

ca8832f

bdice approved these changes Mar 18, 2025

View reviewed changes

KyleFromNVIDIA requested changes Mar 18, 2025

View reviewed changes

context/entrypoint.sh Show resolved Hide resolved

.github/workflows/build-test-publish-images.yml Outdated Show resolved Hide resolved

.pre-commit-config.yaml Show resolved Hide resolved

jameslamb and others added 4 commits March 18, 2025 14:55

Apply suggestions from code review

601c1db

Co-authored-by: Bradley Dice <[email protected]>

Update context/test_notebooks.py

3f0b9c7

Co-authored-by: Bradley Dice <[email protected]>

put name back

a2af74b

update copyright file list

03e0bc8

jameslamb requested a review from KyleFromNVIDIA March 18, 2025 20:15

KyleFromNVIDIA approved these changes Mar 18, 2025

View reviewed changes

.pre-commit-config.yaml Show resolved Hide resolved

rapids-bot bot merged commit e6f193d into rapidsai:branch-25.04 Mar 19, 2025
62 checks passed

jameslamb deleted the more-repo-checks branch March 19, 2025 22:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add shellcheck, update pre-commit hooks #738

add shellcheck, update pre-commit hooks #738

jameslamb commented Mar 5, 2025 •

edited

Loading

jameslamb commented Mar 5, 2025

jameslamb Mar 5, 2025

jameslamb Mar 5, 2025

jameslamb Mar 7, 2025

jameslamb commented Mar 7, 2025

jameslamb Mar 7, 2025

ncclementi Mar 10, 2025

jameslamb Mar 18, 2025

ncclementi left a comment

ncclementi Mar 10, 2025

bdice left a comment

bdice Mar 18, 2025

jameslamb Mar 18, 2025

jameslamb Mar 19, 2025

KyleFromNVIDIA left a comment

jameslamb commented Mar 19, 2025

	# check_tag_exists "$CUVS_BENCH_DATASETS_IMAGE_REPO" "$full_cuvs_bench_datasets_tag"
	# cuvs_bench_datasets_source_tags+=("${org}/${CUVS_BENCH_DATASETS_IMAGE_REPO}:$full_cuvs_bench_datasets_tag")

add shellcheck, update pre-commit hooks #738

add shellcheck, update pre-commit hooks #738

Conversation

jameslamb commented Mar 5, 2025 • edited Loading

jameslamb commented Mar 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameslamb commented Mar 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncclementi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KyleFromNVIDIA left a comment

Choose a reason for hiding this comment

jameslamb commented Mar 19, 2025

jameslamb commented Mar 5, 2025 •

edited

Loading