Clean-up + add hygiene checks#463
Conversation
There was a problem hiding this comment.
I can't work out what has changed here, but as far as I can tell, I think this file can go. There are no references to it in any script, and we don't include any other MLPerf source any more.
There was a problem hiding this comment.
It should just be adding a line ending but it's a huge single line of JSON (not sure why we thought it was the right thing to version control). Happy to wipe this though. Now you've pointed it out, I think there's more files to wipe...
There was a problem hiding this comment.
Wiped along with a couple of other files
|
|
||
| # Install uv for quicker package installations (installed to ~/.local/bin with --user) | ||
| RUN python -m pip install --user uv==0.9.29 | ||
| ENV UV_NO_CACHE=1 |
There was a problem hiding this comment.
I understand not wanting to cache in the docker build phase to avoid releasing extra blobs, but I think it may be inconvenient for the user to find that uv doesn't cache (given that it is such a useful feature of uv!). Can we instead set this on each run command (or unset at the end of the build)?
There was a problem hiding this comment.
Hmm, I'm setting the environment variable during the workshop stage. It should be wiped when we call the second FROM, i.e. when we create the user's workspace. Is there something I'm missing?
| python -m pip install --user --no-cache-dir pre-commit==4.5.1 reuse==6.2.0 | ||
| echo "$HOME/.local/bin" >> "$GITHUB_PATH" | ||
|
|
||
| - name: Run whole-repo whitespace check |
There was a problem hiding this comment.
Why can't we just run the whole set in one pre-commit command?
There was a problem hiding this comment.
Because of the reuse annotate hooks; they should only apply to changed files. Otherwise copyright headers of files that haven't actually been touched will be updated. By itself this shouldn't be a problem since we're not committing files in the CI but it will still cause the hygiene CI to fail (erroneously)
jondea
left a comment
jondea
left a comment
There was a problem hiding this comment.
Generally looks great, thanks! I just had some minor comments.
puneetmatharu
force-pushed
the
add-hygiene-checks
branch
from
5f45c97 to
21dda2b
Compare
puneetmatharu
force-pushed
the
add-hygiene-checks
branch
from
b551afb to
cd3f193
Compare
2 participants
PR Summary
Adds a new hygiene CI workflow and expands pre-commit coverage.
.github/workflows/hygiene.ymlto run changed-file pre-commit hooks, safe whole-repo hygiene checks, whole-reporeuse lint..pre-commit-config.yamlwith EOF, line-ending, JSON/YAML, case/symlink, large-file, executable/shebang, andruff-checkhooks, with comments for non-obvious behavior.Dockerfiles to avoidpip/uvcache directories in final images.rufffindings in PyTorch/TensorFlow examples.Note
I propose that we hold off until #458 is merged before we consider merging this. We could potentially even wait until after the 26.06 release.