fastmachinelearning / hls4ml

Machine learning on FPGAs using HLS
https://fastmachinelearning.org/hls4ml
Apache License 2.0
1.22k stars 396 forks source link

Adiabatically turn on `pre-commit` #678

Closed jmduarte closed 1 year ago

jmduarte commented 1 year ago

Adiabatically turn on pre-commit. The idea is that pre-commit is only run on files touched in a PR. Simultaneously we can open PRs to run pre-commit on files not often updated (or that just aren't touched by any open PRs). Eventually, we can run it on the whole repo.

Changes

Note the con that this may make it harder to review PRs (as a lot of changes will just be style). To be discussed if that's an issue.

To see what it would look like if we ran this on the whole repo in one go, you can check out this branch and do (on Linux):

pre-commit install 
pre-commit run --all-files

I say on Linux, because there seems to be some sed issue with OSX.

jmduarte commented 1 year ago

@vloncar what do you think about this approach?

vloncar commented 1 year ago

I think this is great. I still didn't get around to check how does the formatted C++ look like.

I guess we should also note in the documentation about contributing that users are advised to run pre-commit before making PRs, it will save us a fixing commits on many new PRs.

jmduarte commented 1 year ago

@vloncar to see the diff if we do pre-commit --run all on this PR you can look here: https://github.com/jmduarte/hls4ml/compare/pre-commit...jmduarte:hls4ml:pre-commit-run-all

Note there are still some manual things that we would need to fix (mostly flake8 violations), but they're all pretty trivial.

vloncar commented 1 year ago

This looks great! Is there any way to keep some of the current conventions? I understand black's preference, but the codebase uses single quotes everywhere in python, so majority of the changes are related to that. So maybe we add --skip-string-normalization? I have a slight preference for that. What do others think?

In HLS code, can we use 4 spaces instead of two (IndentWidth) and the line limit (ColumnLimit) that matches the Python one (125)? The rest of the C++ formatting looks ok.

jmduarte commented 1 year ago

@vloncar updated as you requested and new diff is here: https://github.com/jmduarte/hls4ml/compare/pre-commit...jmduarte:hls4ml:pre-commit-run-all

vloncar commented 1 year ago

Thanks, I'm liking this one a lot. Is there a way to exclude some C++ files? I'm thinking the contents of ap_fixed and ac_fixed. These are (mostly) copied from the vendors, perhaps we should keep them intact, as it would be easier to track any future changes to them.

jmduarte commented 1 year ago

yes, let me add that. i was thinking the same.

vloncar commented 1 year ago

Anyone else with their style preferences? This is the perfect bikeshedding PR :smile: If not, I propose we merge it.

jmduarte commented 1 year ago

@vloncar no complaints, so I say we merge 😄

vloncar commented 1 year ago

And now, the avalanche of changes begins :mountain_snow: