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

Add enzyme/tools to clang-format check #1221

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

PragmaTwice
Copy link
Collaborator

@PragmaTwice PragmaTwice commented May 23, 2023

Some misc work are done in this PR:

  • move .clang-format to the top directory, as a common way
  • add pull_request event for the clang-format check CI
  • add enzyme/tools dir for clang-format check
  • format enzyme/tools dir via clang-format
  • remove some useless content (e.g. CMakeLists.txt in the exclude list, which is nonsense)

Close #1211. cc @wsmoses @ZuseZ4

@ZuseZ4
Copy link
Member

ZuseZ4 commented May 23, 2023

@tgymnich @wsmoses iirc we had a reason to exclude PRs?
Also, how about running the rest of CI only once clang-fmt passes, if it's not too much work? Formating needs to get fixed before merging anyway and might save us CI time or the work to manually cancel old runs which already failed due to clang-format.

@PragmaTwice
Copy link
Collaborator Author

Also, how about running the rest of CI only once clang-fmt passes, if it's not too much work? Formating needs to get fixed before merging anyway and might save us CI time or the work to manually cancel old runs which already failed due to clang-format.

Refer to https://stackoverflow.com/questions/58457140/dependencies-between-workflows-on-github-actions , it seems we can add workflow_run clause to other workflows.

@ZuseZ4
Copy link
Member

ZuseZ4 commented May 24, 2023

@wsmoses any objections to it?
It's nothing big, but it is easy to fix formating when it breaks CI and it is annoying to go trough all of our CI categories to cancel runs in which the formating has failed. Having clang-format as first check would prevent that and clang format also doesn't take long to run (15s).

@wsmoses
Copy link
Member

wsmoses commented May 24, 2023

@ZuseZ4 yeah if its already run on every commit push, also running for PR (which would require a commit) is redundant.

@wsmoses
Copy link
Member

wsmoses commented May 24, 2023

I'm fine with the don't run the rest of CI, but let's enable it for everything except the LLVM CI (that way it can still check that it builds on all versions), which takes the least time.

@ZuseZ4
Copy link
Member

ZuseZ4 commented May 24, 2023

Sounds good
@PragmaTwice, would you be fine with implementing these changes (removing runs on PR and adding the two level CI)?

@PragmaTwice
Copy link
Collaborator Author

PragmaTwice commented May 24, 2023

yeah if its already run on every commit push, also running for PR (which would require a commit) is redundant.

Thanks, I can get your point. But I think it is not friendly to outside contributors who fork this repo and open pull requests from their own fork, in which case these CI status does not appear in the PR page.

It seems an alternative solution is something like:

on:
  push:
    branches:
    -  main
  pull_request:

So that if you push to a branch X (not main) and open a pull request upon X, the format CI will only be triggered once per commit.

@ZuseZ4
Copy link
Member

ZuseZ4 commented May 25, 2023

That sounds good, feel free to implement it this way :)

@tgymnich
Copy link
Member

LGTM. There are just a few minor formatting issues left.

@PragmaTwice
Copy link
Collaborator Author

PragmaTwice commented May 27, 2023

Conflict has been solved now.

@PragmaTwice
Copy link
Collaborator Author

It seems this method cannot work well, i.e. these workflows by workflow_run event cannot be triggered as expected.

Maybe need some investigation or workaround, or just keep the original behavior and try it in another PR.

@PragmaTwice
Copy link
Collaborator Author

@ZuseZ4
Copy link
Member

ZuseZ4 commented May 27, 2023

It does look like Integration CI is now blocked on all Enzyme CI tests passing. Especially the apple runners take a while, so by now it would increase our CI times by a good amount. Can you update it please to make sure that we are only waiting for the clang-format check to pass before starting Integration CI?

Edit: looks like we noticed it simultaneously.

@PragmaTwice
Copy link
Collaborator Author

Edit: looks like we noticed it simultaneously.

🤣

https://stackoverflow.com/questions/72551643/workflow-run-not-triggered-as-expected-after-parent-workflow-completes
Maybe the root cause is from the quote. Some trying...

Some try, still not triggered. I will revert all these workflow_run changes firstly.

@PragmaTwice
Copy link
Collaborator Author

PragmaTwice commented May 28, 2023

Hi @ZuseZ4 @wsmoses , now it is reverted from workflow_run clauses to build dependencies between workflows.

I think we can firstly merge current changes to check tblgen code format and prevent further conflicts, and try to make workflow dependencies later.

devmotion pushed a commit to devmotion/Enzyme that referenced this pull request Jan 27, 2024
* Runtime splat

* fix internal side
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add .clang-format and corresponding CI pass to check code format
4 participants