Skip to content

Commit ddb36fd

Browse files
jameslambjameslamb
authored
fix return-before-assignment bug in notebook test script, add pre-commit, drop cuspatial notebooks (#690)
CI has been failing here for a few weeks, as a result of some failing `cuspatial` notebooks. As described in rapidsai/cuspatial#1406, I observed those notebooks failing like this: ```text cannot access local variable 'execution_time' where it is not associated with a value ``` Turns out that that was coming from the `test_notebooks.py` script in this repo. It calls `return execution_time` unconditionally, even though it's possible for that variable to not exist. This fixes that, and proposes adding a `pre-commit` configuration and corresponding CI job, to help catch such things earlier in the future. This also proposes skipping some `cuspatial` notebooks, to get CI working. I'd hoped to address that in rapidsai/cuspatial#1407, but most `cuspatial` maintainers are busy or unavailable this week. I think we should get CI working here again and deal with the `cuspatial` notebooks as a separate concern. ## Notes for Reviewers Any other changes you see in files here are the result of running `pre-commit run --all-files` (with some manual fixes applied to fix `ruff` warnings). Authors: - James Lamb (https://github.com/jameslamb) Approvers: - Ray Douglass (https://github.com/raydouglass) - https://github.com/jakirkham - AJ Schmidt (https://github.com/ajschmidt8) URL: #690
1 parent a1e5d98 commit ddb36fd

File tree

7 files changed

+76
-11
lines changed

7 files changed

+76
-11
lines changed

‎.github/ISSUE_TEMPLATE/bug_report.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ A clear and concise description of what you expected to happen.
2121
- Method of cuDF install: [conda, Docker, or from source]
2222
- If method of install is [Docker], provide `docker pull` & `docker run` commands used
2323
- Please run and attach the output of the `cudf/print_env.sh` script to gather relevant environment details
24-
24+
2525

2626
**Additional context**
2727
Add any other context about the problem here.

‎.github/workflows/pr.yml‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,20 @@ concurrency:
1010
cancel-in-progress: true
1111

1212
jobs:
13+
checks:
14+
runs-on: ubuntu-latest
15+
steps:
16+
- name: Checkout
17+
uses: actions/checkout@v4
18+
- uses: actions/setup-python@v5
19+
with:
20+
python-version: "3.10"
21+
- name: Run pre-commit
22+
run: |
23+
pip install pre-commit
24+
pre-commit run --all-files
1325
docker:
26+
needs: [checks]
1427
uses: ./.github/workflows/build-test-publish-images.yml
1528
with:
1629
build_type: pull-request

‎.pre-commit-config.yaml‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
repos:
3+
- repo: https://github.com/pre-commit/pre-commit-hooks
4+
rev: v4.6.0
5+
hooks:
6+
- id: end-of-file-fixer
7+
- id: trailing-whitespace
8+
- repo: https://github.com/astral-sh/ruff-pre-commit
9+
rev: v0.5.2
10+
hooks:
11+
- id: ruff
12+
args: ["--config", "pyproject.toml"]

‎context/test_notebooks.py‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,7 @@
55
import sys
66
import timeit
77
from typing import Iterable
8-
import nbconvert
98
import nbformat
10-
from datetime import datetime
119
from nbconvert.preprocessors import ExecutePreprocessor
1210
import yaml
1311

@@ -29,12 +27,15 @@
2927
# following nbs are marked as skipped
3028
'cugraph/algorithms/layout/Force-Atlas2.ipynb',
3129
'cuspatial/binary_predicates.ipynb',
32-
'cuspatial/cuproj_benchmark.ipynb'
30+
'cuspatial/cuproj_benchmark.ipynb',
31+
# context on these being skipped: https://github.com/rapidsai/cuspatial/pull/1407
32+
'cuspatial/cuspatial_api_examples.ipynb',
33+
'cuspatial/nyc_taxi_years_correlation.ipynb'
3334
]
3435

3536

3637
def get_notebooks(directory: str) -> Iterable[str]:
37-
for root, dirs, files in os.walk(directory):
38+
for root, _, files in os.walk(directory):
3839
for file in files:
3940
if (
4041
file.endswith(".ipynb")
@@ -71,12 +72,12 @@ def test_notebook(notebook_file, executed_nb_file):
7172

7273
# use nbconvert to run the notebook natively
7374
ep = ExecutePreprocessor(timeout=600, kernel_name="python3", allow_errors=True)
75+
task_init = timeit.default_timer()
7476
try:
75-
task_init = timeit.default_timer()
76-
nb, nb_resources = ep.preprocess(nb, {"metadata": {"path": ""}})
77-
execution_time = timeit.default_timer() - task_init
77+
nb, _ = ep.preprocess(nb, {"metadata": {"path": ""}})
7878
except Exception as e:
7979
errors.append(e)
80+
execution_time = timeit.default_timer() - task_init
8081

8182
with open(executed_nb_file, "w", encoding="utf-8") as f:
8283
nbformat.write(nb, f)
@@ -152,7 +153,7 @@ def test_notebook(notebook_file, executed_nb_file):
152153
print(f"Input must be a directory. Got: {ns.input}")
153154
sys.exit(1)
154155

155-
notebooks = sorted(list(get_notebooks(ns.input)))
156+
notebooks = sorted(get_notebooks(ns.input))
156157
print(f"{len(notebooks)} Notebooks to be tested:")
157158
for notebook in notebooks:
158159
print(notebook)

‎pyproject.toml‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
[tool.ruff]
2+
target-version = "py310"
3+
4+
[tool.ruff.lint]
5+
ignore = [
6+
# (flake8) line too long
7+
"E501",
8+
# (pylint) too many branches
9+
"PLR0912",
10+
]
11+
select = [
12+
# flake8-builtins
13+
"A",
14+
# flake8-bugbear
15+
"B",
16+
# flake8-comprehensions
17+
"C4",
18+
# pycodestyle
19+
"E",
20+
# eradicate
21+
"ERA",
22+
# pyflakes
23+
"F",
24+
# flynt
25+
"FLY",
26+
# perflint
27+
"PERF",
28+
# pygrep-hooks
29+
"PGH",
30+
# pylint
31+
"PL",
32+
# flake8-pyi
33+
"PYI",
34+
# flake8-return
35+
"RET",
36+
# ruff-exclusive checks
37+
"RUF",
38+
# flake8-bandit
39+
"S",
40+
]

‎raft-ann-bench/README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
This folder contains the dockerfiles for generating GPU and CPU RAFT ANN benchmark images.
44

5-
This images are meant to enable end users of RAFT's ANN algorithms to easily run and reproduce benchmarks and comparisons between RAFT and third party libraries.
5+
These images are meant to enable end users of RAFT's ANN algorithms to easily run and reproduce benchmarks and comparisons between RAFT and third party libraries.
66

77
# Image types:
88

‎raft-ann-bench/cpu/Dockerfile‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,4 +60,3 @@ RUN /home/rapids/raftannbench/get_datasets.sh
6060
CMD ["--dataset fashion-mnist-784-euclidean", "", "--algorithms hnswlib"]
6161

6262
ENTRYPOINT ["/bin/bash", "/data/scripts/run_benchmark_preloaded_datasets.sh"]
63-

0 commit comments

Comments
 (0)