Skip to content

port lint workflows from CircleCI to GHA #7401

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

Merged
merged 16 commits into from
Mar 10, 2023
Merged

port lint workflows from CircleCI to GHA #7401

merged 16 commits into from
Mar 10, 2023

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Mar 9, 2023

Per title. I don't expect any problems here so I'm ok with removing the lint workflows on CircleCI right away. LMK if you disagree so I can revert d31f3bb.

cc @seemethere

@pytorch-bot
Copy link

pytorch-bot bot commented Mar 9, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/7401

Note: Links to docs will display an error until the docs builds have been completed.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@pmeier pmeier marked this pull request as ready for review March 9, 2023 14:31
@pmeier pmeier requested review from NicolasHug and osalpekar March 9, 2023 14:31
@NicolasHug
Copy link
Member

Can you make these jobs fail so we can get a sense of the workflow for checking the errors?

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks Philip, looks like the detail of the failure results are just one click away which is nice. LGTM!

@pmeier
Copy link
Collaborator Author

pmeier commented Mar 9, 2023

I looked into why the C lint job emits quite few of these warnings: https://github.com/pytorch/vision/actions/runs/4375903581/jobs/7657253653#step:10:196

./clang-format: /lib64/libtinfo.so.5: no version information available (required by ./clang-format)

According to this SO answer, ncurses from conda defaults channel simply doesn't provide this information. The apparent solution is to install from conda-forge instead.

Since we are not building or testing here, this shouldn't be an issue and I tried that in 0ab8bf7. However the clang-format we are using depends on libtinfo.so.5 that is provided by ncurses=5, which is over 6 years old at this point and also doesn't contain the version info. So this is patched in libtinfo.so.6 from conda-forge, but that doesn't help us here.

This was not an issue on CircleCI since we were able to install it as system package on Ubuntu:

vision/.circleci/config.yml

Lines 295 to 297 in e59cf64

- apt_install:
args: libtinfo5
descr: Install additional system libraries

The new runners use CentOS 7 and I was unable to install it through the system package manager.

Thus, since this is only a warning and doesn't affect the job otherwise, I'm ok to keep it as is. Any concerns @NicolasHug? @osalpekar I don't think there is an option to specify a docker image for the generic Linux job, is there?

@NicolasHug
Copy link
Member

NicolasHug commented Mar 9, 2023

still OK for me. We rely on the clang-format job once in a blue moon anyway

@pmeier pmeier mentioned this pull request Mar 9, 2023
21 tasks
Copy link
Member

@osalpekar osalpekar left a comment

Choose a reason for hiding this comment

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

LGTM in general. As a longer term change, would be great to move linting to lintrunner like we have in PT Core (see more here: https://github.com/pytorch/pytorch/wiki/lintrunner). But this change is good-to-go!

Copy link
Member

@seemethere seemethere left a comment

Choose a reason for hiding this comment

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

LGTM, this looks great!

@pmeier pmeier merged commit 82cf540 into pytorch:main Mar 10, 2023
@pmeier pmeier deleted the lint-ci branch March 10, 2023 08:24
@github-actions
Copy link

Hey @pmeier!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

@pmeier pmeier mentioned this pull request Mar 10, 2023
facebook-github-bot pushed a commit that referenced this pull request Mar 30, 2023
Reviewed By: vmoens

Differential Revision: D44416612

fbshipit-source-id: 0c5e199258248c776dcec6606016abfeb50f78aa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants