rapidsai / legate-boost

GBM implementation on Legate
https://rapidsai.github.io/legate-boost/
Apache License 2.0
8 stars 6 forks source link

run mypy via pre-commit #118

Closed jameslamb closed 2 months ago

jameslamb commented 2 months ago

Contributes to #115.

Proposes running mypy via pre-commit, for the following reasons:

As part of this, this PR also proposes running pre-commit checks in CI before the other building and testing, on a standard GitHub-hosted ubuntu-latest runner, to avoid occupying an NVIDIA-hosted GPU runner for a build that's doomed to fail because of linter errors.

Notes for Reviewers

Running these checks in the rapidsai/ci-conda:latest image and via a script named ci/check_style.sh mirrors what is done across most RAPIDS libraries. That'll make it easier to continue integrating CI tools from RAPIDS.

RAMitchell commented 2 months ago

@trivialfis did this for a reason - maybe he can explain why.

trivialfis commented 2 months ago

The pre-commit hook runs in an isolated environment, which doesn't include legate and other dependencies. As a result, the type checking is not rigorous.

jameslamb commented 2 months ago

The pre-commit hook runs in an isolated environment, which doesn't include legate and other dependencies. As a result, the type checking is not rigorous.

Blegh you're right, I misunderstood how the isolation for mypy worked. We could get pretty good coverage of most dependencies by adding them to the additional_dependencies list that pre-commit provides... but not legate-core or cunumeric, because they don't publish wheels today.

We can close this, sorry for the noise and thanks for correcting my understanding.

RAMitchell commented 2 months ago

Just had a quick read about this issue: https://tech.quantco.com/blog/conda-in-pre-commit

Looks like it is possible now with (too much) effort to use a conda environment with pre-commit. Maybe in the future this will be easier.

jameslamb commented 2 months ago

ooo very interesting, hadn't seen that! Thanks for sharing.