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

run mypy via pre-commit #118

Closed
wants to merge 2 commits into from
Closed

run mypy via pre-commit #118

wants to merge 2 commits into from

Conversation

jameslamb
Copy link
Member

Contributes to #115.

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

  • reduce duplication of mypy configuration
  • remove a dependency from the [test] extra for the package
  • standardizes mypy version across all the place where those checks are run (to limit "it works on my computer, why is CI broken" types of situations)

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.

@jameslamb jameslamb added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Jul 17, 2024
@jameslamb jameslamb changed the title WIP: run mypy via pre-commit run mypy via pre-commit Jul 17, 2024
@jameslamb jameslamb marked this pull request as ready for review July 17, 2024 19:14
@RAMitchell
Copy link
Contributor

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

@trivialfis
Copy link
Member

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
Copy link
Member Author

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.

@jameslamb jameslamb closed this Jul 17, 2024
@jameslamb jameslamb deleted the mypy branch July 17, 2024 21:13
@RAMitchell
Copy link
Contributor

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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants