Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ci] update some linting versions #6598

Merged
merged 4 commits into from
Aug 15, 2024
Merged

[ci] update some linting versions #6598

merged 4 commits into from
Aug 15, 2024

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Aug 9, 2024

Contributes to #3763.

Updates some linting versions.

  • newest versions of all conda install'd stuff for the lint and check-docs tests
  • newest pre-commit hook versions from pre-commit autoupdate

Happy to say the newest version of ruff caught a bug... a Python test that could accidentally not raise an exception because of a missing raise statement! This fixes that as well.

@@ -53,7 +53,7 @@ def _create_data(task, n_samples=100, n_features=4):
elif task == "multiclass-classification":
centers = 3
else:
ValueError(f"Unknown classification task '{task}'")
raise ValueError(f"Unknown classification task '{task}'")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From ruff:

tests/python_package_test/test_sklearn.py:56:13: PLW0133 Missing `raise` statement on exception
   |
54 |             centers = 3
55 |         else:
56 |             ValueError(f"Unknown classification task '{task}'")
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW0133
57 |         X, y = make_blobs(n_samples=n_samples, n_features=n_features, centers=centers, random_state=42)
58 |         g = None
   |
   = help: Add `raise` keyword

Found 1 error.

OS_NAME: 'linux'
PYTHON_VERSION: '3.12'

jobs:
test:
name: ${{ matrix.task }}
runs-on: ubuntu-22.04
runs-on: ubuntu-latest
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will prevent needing to update this OS version in the future. I don't think there's any reason this job needs to be on 22.04 specifically.

@@ -17,13 +17,14 @@ concurrency:

env:
COMPILER: 'gcc'
MAKEFLAGS: '-j4'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While looking at this configuration, I realized that the check-r-docs job is installing a bunch of R packages on Linux. CRAN doesn't provide Linux binaries, so that means they're all being compiled from source.

Parallelizing compilation should speed that job up, as we do with the other R jobs:

.ci/test.sh Outdated Show resolved Hide resolved
@jameslamb jameslamb changed the title WIP: [ci] update some linting versions [ci] update some linting versions Aug 9, 2024
@jameslamb jameslamb marked this pull request as ready for review August 9, 2024 03:11
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jameslamb jameslamb merged commit 5e4ab24 into master Aug 15, 2024
44 checks passed
@jameslamb jameslamb deleted the ci/new-linter-versions branch August 15, 2024 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants