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.

Sign up for GitHub

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

Jump to bottom

Auto formatting via pre-commit.ci #136

Merged
merged 4 commits into from
Jan 27, 2022
Merged

Auto formatting via pre-commit.ci #136

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f82e611
Add CI section into pre-commit configuration file
PointKernel Jan 26, 2022
61f8165
Update README
PointKernel Jan 26, 2022
871bb82
Update README
PointKernel Jan 26, 2022
76eb082
Update clang-format to use the latest release
PointKernel Jan 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,18 @@
ci:
autofix_commit_msg: |
[pre-commit.ci] auto code formatting
autofix_prs: true
autoupdate_branch: ''
autoupdate_commit_msg: '[pre-commit.ci] pre-commit autoupdate'
autoupdate_schedule: quarterly
skip: []
submodules: false

repos:
- repo: https://github.com/pre-commit/mirrors-clang-format
rev: v11.1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, this pinning will probably be updated by the “autoupdate” functionality of pre-commit.ci.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, this is why I set the update frequency to quarterly. And according to pre-commit-ci/issues#83, there is no plan to provide a schedule-off option.

Copy link
Contributor

Choose a reason for hiding this comment

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

I support quarterly just to keep the number of PRs down -- but it does seem like we could update the pinnings before we merge this PR, since it's going to keep clang-format updated anyway. (Unless we're trying to stay pinned to v11.1.0 for some reason? I don't think the choice of version matters if it's automatically enforced.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to keep this version in line with RAPIDS? AFAIK most cuCollections developers (at least at present) are RAPIDS devs, and having this misaligned could lead to issues if users choose to invoke clang-format via the CLI and they have a different version installed than the one specified by pre-commit. I would be fine if the answer is no, but we should be aware of the issues this discrepancy could cause.

Copy link
Contributor

@bdice bdice Jan 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think the choice of version matters if it's automatically enforced.

That’s my point: any issues caused by misaligned versions will be auto-fixed by CI, so it doesn’t matter what devs have locally. Moreover, you can’t really pin the version because pre-commit CI will open a PR to update it to the latest version every quarter. Personally, I am perfectly okay with keeping the pinnings updated on a quarterly basis and ignoring whatever devs have locally in favor of the pre-commit autofixes.

If we implemented our own “auto-fix” solution in gpuCI similar to pre-commit.ci, then we’d have greater control over version pinnings. However, I don’t think that is needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with @bdice, any version mismatch issues will be auto fixed by CI anyway. By all means, it's clearly stated in README that directly using clang-format on the command line is not recommended.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To be clear, I understood what @bdice was saying about how things would work. I'm asking for a value judgment on whether we think this is a problem. We explicitly declined to move in this direction for RAPIDS because it would prevent using the format-on-write hooks in VSCode, which can't use the pre-commit hook. I don't personally use VSCode, so I don't care about that feature, but before we commit to this approach I just wanted to make sure that everyone involved recognized the limitations since lack of VSCode integration was deemed sufficient reason not to rely on pre-commit versioning in RAPIDS. I'm happy if we just stick with pre-commit though :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I see your point. This could be a problem not only for VSCode but for all IDEs/editors having auto-formatting support. Technically, users can still stick with the IDE/eidtor clang-format hook. It's just any mismatches will be then auto-fixed by CI (or pre-commit hook).

Copy link
Contributor

@bdice bdice Jan 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It's just any mismatches will be then auto-fixed by CI (or pre-commit hook).

I'm strongly in favor of trying this approach here. If it works well, I want to eventually revisit that conversation for RAPIDS or cuDF in particular. I don't expect it to be painful, and I suspect it will lead to greater productivity than our current approach of waiting for CI to fail and then identifying/fixing/committing/pushing "style fixes." The current manual approach costs us so much in CI retry time and hinders our iteration speed. Lately I've been doing a lot of work via github.dev (to make small changes, since it's vastly quicker than dealing with local branches / etc. and I can work in lots of browser tabs at once) and automatic CI fixes for style would be so awesome to add to that workflow.

hooks:
- id: clang-format
# Using the pre-commit stage to simplify invocation of all
# other hooks simultaneously (via any other hook stage). This
# can be removed if we also move to running clang-format
# entirely through pre-commit.
stages: [commit]
name: clang-format
description: Format files with ClangFormat.
entry: clang-format
language: system
files: \.(cu|cuh|h|hpp|cpp|inl)$
'types_or': [file]
PointKernel marked this conversation as resolved.
Show resolved Hide resolved
args: ['-fallback-style=none', '-style=file', '-i']
Expand Down
28 changes: 2 additions & 26 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,32 +86,8 @@ Binaries will be built into:

## Code Formatting

`cuCollections` uses [`pre-commit`](https://pre-commit.com/) along with [`mirrors-clang-format`](https://github.com/pre-commit/mirrors-clang-format) to format the C++/CUDA files.
To install `pre-commit` via `conda` or `pip`:

```bash
conda install -c conda-forge pre_commit
```

```bash
pip install pre-commit
```

and then running:
```bash
pre-commit install
```

from the root of the `cuCollections` repository. Now code formatting will be run each time you commit changes.

Optionally, you may wish to manually format the code:
```bash
pre-commit run clang-format --all-files
```

### Caveats
`mirrors-clang-format` guarantees the correct version of `clang-format` and avoids version mismatches.
Users should **_NOT_** use `clang-format` directly on the command line to format the code.
`cuCollections` uses [`pre-commit.ci`](https://pre-commit.ci/) along with [`mirrors-clang-format`](https://github.com/pre-commit/mirrors-clang-format) to automatically format involved C++/CUDA files in a pull request.
PointKernel marked this conversation as resolved.
Show resolved Hide resolved
Users should enable the `Allow edits by maintainers` option to get auto-formatting to work.


## Data Structures
Expand Down