chore: update pre-commit hooks #528
Conversation
![pre-commit-ci[bot]](https://avatars.githubusercontent.com/in/68672?s=60&v=4){kind=small}
updates: - [github.com/astral-sh/ruff-pre-commit: v0.7.3 → v0.7.4](astral-sh/ruff-pre-commit@v0.7.3...v0.7.4)
|
AttributeError: 'VectorObject2D' object has no attribute 'minmax_depth' |
No, it doesn't. |
|
This is #524. Should be fixed once a new release of awkward is out. |
Then we have a good reason to make a new Awkward release. I'll do that today. In general, I don't think it's a good idea to merge a PR with a failing test, even if we know for sure that it's not failing because of the PR (just because I think that's a slippery slope—what if we're wrong?). Ideally, the head of all repos should be a working system. (There's another school of thought in which the head is not working, and a branch intentionally made for a release is laboriously brought into a working state. But that feels like always having to swim upstream to me.) |
|
If rerunning the CI on the main branch also fails in the same way, and the PR isn't touching anything that affects the failure, it's a bit fuzzy. It's nice to keep all merges green, but if CI is broken and the fix is external, you can't always stop all development. If you are sure you aren't introducing new failures and the failing jobs are limited to special cases (like the notebooks job here), that might be okay. (The Fedora CI in scikit-build-core is always failing due to upstream Fedora breaking their CI every week or so, so I just have to ignore a red x there if it comes from Fedora most of the time!) |
Awesome, thank you!
Noted. I merged this one as it was just a pre-commit version update and the PR did not touch any code files. |
|
That's understandable. It's a fine line to draw between "I know this PR is safe, despite the test failures" and "I really know it's safe." In fact, I had to ignore a deploy-documentation-preview failure to release Awkward (the documentation preview has been giving us trouble recently, but it follows a different path from the documentation itself, so I really could ignore it). This PR didn't touch any code files, but updating pre-commit can make it use a new ruff version, which can reformat source code and (who knows!) cause a bug. It's a long shot, and you know in this case that a new ruff was not changing any source code or wanting it to change. I 100% agree that this PR seems totally safe, but it's on the side of the fine line where I'd wait for it to formally turn green before merging. I just finished that Awkward release; I'll run Vector's tests on |
|
Is this what you meant: https://github.com/scikit-hep/vector/actions/runs/11920001965 ? You linked a workflow with the jobs that passed before. Only the notebook workflow was failing. |
|
FYI, the notebook job doesn't run on main, only on PRs, so it didn't affect the green checkmark on the main page. |
|
You're right—I thought that the failure had been in the "CI" workflow (without checking). Thanks for running the appropriate one, and I see that it passes now! |
updates: