cms-L1TK / cmssw

Fork of CMSSW where improvements to L1 tracking code are developed.
http://cms-sw.github.io/
Apache License 2.0
4 stars 5 forks source link

Added git CI #106

Closed tomalin closed 2 years ago

tomalin commented 3 years ago

This adds git Continuous Integration tests to cms-L1TK/cmssw.

The file .github/workflows/github_CI.yml launches CI on github whenever a PR is made to any branch (typically L1TK-dev-12_0_0_pre4). The CI can also be run for test purposes if a commit is made to any temporarily specified branch. The github CI then launches a gitlab CI run, controlled by https://gitlab.cern.ch/cms-l1tk/cmssw_CI/-/blob/masterCI/.gitlab-ci.yml , which checks out the branch under test from github, rebases it on top of the default branch, compiles it with CMSSW, performs standard code style checks on it, and runs L1TrackNtupleMaker_cfg.py for both HYBRID & HYBRID_NEWKF algos.

Both github_CI.yml & .gitlab-ci.yml will need editing (a few lines) if the CMSSW version changes. Since .github_CI.yml can't be added to the official release, it must be deleted from this repo whenever we make a PR to the official release, and readded when we branch off from it. (It could be backed up in https://gitlab.cern.ch/cms-l1tk/cmssw_CI . Not yet done).

Communication between gitlab & github CIs is done with https://github.com/cms-L1TK/gitlab-mirror-and-ci-action . This is based heavily on https://github.com/aperloff/gitlab-mirror-and-ci-action , which did this action already for the L1 track HLS repo. The new version should via option "IS_CMSSW" be able to work not only for CMSSW, but also for HLS (not yet checked), so if @aperloff agrees can replace the old one.

tomalin commented 3 years ago

I have several concerns about using this both for CMSSW and HLS. Some are stylistic, but others are more substantive. A lot of them will make reference to options here, which are then used in https://github.com/cms-L1TK/gitlab-mirror-and-ci-action/blob/master/entrypoint.sh.

  1. Why do you need the function urlencode? What issue is it trying to solve?
  2. The options L1TRKALGO_CMSSW and GITHUB_REPO_OWNER are stored in text files, which then get added to the CMSSW repo. Are these only located in the GitLab mirror? Could these not be sent as CI variables using push options
  3. Instead of RETURN_FILE, which is just contains a value to be printed to the log for a second time, could you not set an environment variable using this?
  4. I don't have a problem switching from my gitlab-mirror-and-ci-action to the one in cms-L1TK as long as the issues I've brought up are addressed.

Answering Q1: Since https://github.com/aperloff/gitlab-mirror-and-ci-action originally forked from https://github.com/SvanBoxel/gitlab-mirror-and-ci-action , I updated to the latest version of the latter (keeping your changes). SvanBoxel was updated 8 months ago to get the pipeline_id from urlencode https://github.com/SvanBoxel/gitlab-mirror-and-ci-action/commit/6ab7cebd2761b4501e0cd67c4cbe8c0abd4a735d . I do not understand why the authors did this, and agree that it doesn't change the results with respect to your code. I'm happy to delete it if you prefer that.

Answering Q2: I was not familiar with [push options] for CI.variable. But I like this idea. I have now implemented it successfully.

Answering Q3: I believe your proposal would work. However, the current method has the advantage that someone reading https://github.com/cms-L1TK/cmssw/blob/ianGitCI/.github/workflows/github_CI.yml#L66 immediately understands that the call to function entrypoint.sh in cms-L1TK/gitlab-mirror-and-ci-action@master returns the value of the gitlab pipeline ID. Whereas if entrypoint.sh instead stores the pipeline ID in an environment variable, it is less obvious to someone reading github_CI.yml that it is the call to entrypoint.sh that is calculating the pipeline ID.

aperloff commented 3 years ago

Answering Q1: Since https://github.com/aperloff/gitlab-mirror-and-ci-action originally forked from https://github.com/SvanBoxel/gitlab-mirror-and-ci-action , I updated to the latest version of the latter (keeping your changes). SvanBoxel was updated 8 months ago to get the pipeline_id from urlencode SvanBoxel/gitlab-mirror-and-ci-action@6ab7ceb . I do not understand why the authors did this, and agree that it doesn't change the results with respect to your code. I'm happy to delete it if you prefer that.

Ah, I didn't know that https://github.com/SvanBoxel/gitlab-mirror-and-ci-action has been updated. If your version is more up-to-date, then I'm fine keeping those new features.

tomalin commented 2 years ago

I've now got "cmsRun L1TrackNtupleMaker_cfg.py working". The CI runs it twice, once for HYBRID & once for HYBRID_NEWKF algos. It runs on 1000 skimmed ttbar+0PU events, stored in CERNbox.

tomalin commented 2 years ago

@aperloff or @EmyrClement are you willing to approve this PR? As you can see from https://gitlab.cern.ch/cms-l1tk/cmssw_CI/-/pipelines/3258097 , it is now working, including the cmsRun step. It would be nice to have it available to check all the other ongoing PRs.

aperloff commented 2 years ago

I have posted my approval, but since I wasn’t requested for review, I can’t actually approve the PR. Emyr will need to do that.

Cheers, Alexx

From: Ian Tomalin @.> Reply-To: cms-L1TK/cmssw @.> Date: Thursday, November 18, 2021 at 8:10 AM To: cms-L1TK/cmssw @.> Cc: Alexx Stephen Perloff @.>, Mention @.***> Subject: Re: [cms-L1TK/cmssw] Added git CI (PR #106)

@aperloffhttps://github.com/aperloff or @EmyrClementhttps://github.com/EmyrClement are you willing to approve this PR? As you can see from https://gitlab.cern.ch/cms-l1tk/cmssw_CI/-/pipelines/3258097 , it is now working, including the cmsRun step. It would be nice to have it available to check all the other ongoing PRs.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/cms-L1TK/cmssw/pull/106#issuecomment-973006720, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABEV3KXAZ6YJFVSWRKPFRX3UMUP4RANCNFSM5HBW6ULQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

tomalin commented 2 years ago

Hi @aperloff , I tried refreshing the original request for you to be an Approver. Please try approving again.