This page contains information about the software review process in ATLAS. It describes the tasks of merge request review shifters and provides some guidance as well as hints for common situations/problems. It is assumed that you are familiar with the general workflow for ATLAS software development as summarised in the Workflow Quick Reference. Furthermore, a solid knowledge of the C++ programming language is mandatory. An overview of the ATLAS coding conventions and style guidelines is desirable.
For every working day, there are two level-1 and two level-2 shifters on shift. Ideally, they work in different time zones (e.g. CERN and US) to provide developers from all timezones with a timely review of their merge requests. Please note that the shift times indicated in the OTP system are CERN times.
The current shift crew can be found on the OTP S&C Shift Crew page.
As a level-1 shifter you are expected to have a thorough understanding of the C++ language and some basic knowledge about python. You should be able to check the formal code requirements listed below as well as the criteria for good documentation and the adherence to the ATLAS coding conventions and style guidelines.
For trivial changes, you should approve MRs directly. If the changes are more
substantial and require a deeper understanding of the Athena code
structure/workflows, you can escalate this MR to the level-2 shifter for
review (but please make sure you explain why, i.e. write ‘Escalating to L2, because I’m not sure whether there is a memory leak in the
bar() method of
Foo.cxx’ or ‘Escalating to L2 because
Foo.cxx has been completely re-written.’) If you are in doubt, you can always pass a MR on to the next level, but try to guide the next shifter to save them from re-reading everything.
In addition to the knowledge required by the level-1 shifts, you have some experience with the usage and design of the Athena software and are able to comment on structural changes. A certain level of proficiency in git is helpful as well.
Your main task is the review of MRs which were escalated to level-2 by the
level-1 shifters. In case that these changes require dedicated expertise in a
specific software domain, you could always include an expert in the discussion
@username mentioning feature in GitLab, see the FAQ for a
list of contact persons). Otherwise, you are expected to approve the MR or
iterate with the developer on the proposed changes.
Your task as review shifter is to review the proposed changes to the ATLAS software and, if necessary, iterate with the developer(s) until these changes fulfil certain criteria outlined below. The outcome of the review process is the recommendation for the release coordinators to accept this merge request (or not). It is up to the release coordinators to finally accept or reject a merge request.
Upon creation merge requests are labelled automatically by the CI system. The labels relevant for the review process are:
review-pending-level-1: The review process has started and a level-1 shifter should have a look at this MR.
review-pending-level-2: The review process is with the level-2 shifter for further clarification/review.
review-pending-expert: The advice of a software domain expert on the proposed changes is needed.
review-user-action-required: The review shifters raised some comments which should be addressed by the developer(s).
review-approved: This MR is approved by the review shifters and is recommended to be accepted by the release coordinators.
Once you are done with your part of the review process, you should enter a (short) comment about your findings and update the review label appropriately. The image below summarises the workflow.
review-pending-level-1/2labels (depending on what shift you do) and review according to the guidelines given below.
Oldest updated) and try to trigger some action (e.g. ping developers, include experts) if appropriate.
sweep:done), as explained in the Git Workflow Tutorial here.
CIlabel to the issue it will appear in the status board.
The review should encompass criteria from the following four categories, ordered by importance:
but should also address some general points. After the review and possible iterations with the developer, please make sure that all discussions are marked as resolved before approving a merge request.
Does the code compile with any error or warnings? A successful build is indicated by this line in the CI result:
✅ Athena: number of compilation errors 0, warnings 0
whereas a problematic build that needs follow-up is marked with a warning sign:
⚠️Athena: number of compilation errors 0, warnings 8
For a MR to be approved there should be zero compilation errors and warnings. In case the warnings are unrelated to the changes in the MR (see the build logs) they can be ignored.
WIPand request it only once it is final (i.e. it builds locally and has been tested).
std::abs(some_value > 2)!
cout/cerr- use the Athena logging service through the message macros!
from module import *!
CMakeLists.txtfiles (e.g. custom code using more than simple
atlas_<foo>(...)functions) please notify Attila (
@akraszna) as a watcher.
Functional code is important and so is maintainability. Therefore, please check that:
/* Copyright (C) 2002-2019 CERN for the benefit of the ATLAS collaboration */
Changestab) have this copyright statement! Trivial job option fragments and boilerplate files are exempt from this rule, usually found in
svnpullas this handles the copyright statement automatically.
git rebasewhich we consider an advanced topic. However, it should be at least pointed out to the developers that guidelines for commit messages exist and that they should be considered in the future.
Sometimes people put information of limited relevance in the description, e.g.
These kind of things should rather be added as comments than being put directly in the MR description. You can always ask the developer to edit the MR title and description. Keep in mind that this is the replacement of the ChangeLog and as such a critical piece of information when it comes to bug fixes and debugging.
git commit --amendto the developers. Merging commits is possible using
git rebase -iwhich you may recommend, but this again is an advanced operation which requires a certain level familiarity with git. Another option here is to use GitLab’s Squash Commits functionality within the Merge Request.
Sticking to the ATLAS guidelines for software development helps other people to understand your code better and faster and, thus, facilitates maintainability. As such, the following points should be mentioned as minor suggestions. Since these guidelines have not been followed too closely in the past, you will see a lot of code which could be improved. Some of it may not even been touched by the actual changes. This is a delicate topic and some developers may react huffish to these kind of comments. Therefore, the general strategy is to aim for gradual improvement than drastic changes:
In the CI builds we’re running three main tests that check whether a particular MR is changing the simulation/overlay/reconstruction outputs. These are named, respectively:
If one or more of these tests are failing, please take the following steps:
Tools/PROCTools/python/RunTier0TestsTools.pyaccordingly and add the changes to the MR,
Only after these steps, the shifter should add the label
review-approved. Also, please
don’t forget to make sure the relevant labels, i.e.
are added to the MR.
The derivation test, currently named
FrozenDerivationTest_optional-test, is handled differently.
If this test is failing, again make sure things are labelled correctly, and the changes are reviewed
by the relevant expert(s). The reference file is updated automatically on a daily basis (nominally around 8 AM CERN time).
Therefore, no further action is needed from the shifter but please be vigilant as any CI build
that happens between the time this MR is accepted by the coordinator and the time the reference
file is updated will have this test failed. In that case, the shifter can ask Jenkins to rebuild
once the reference file is updated.
ChangeLogfiles (no nay never)
cmtdirectories or files therein (There won’t be any
cmtbased builds from git)
.svnfolders (Developers sometimes add them by mistake)
How should developers update their merge request?
If developers would like to update their code changes (either based on feedback from the review shifters or for any other reason), they can simply add further commits to the source branch. The MR will be updated automatically. Since a new CI job is run on every update of the source branch, developers are encouraged to push only working sets of commits instead of pushing each commit individually. This helps reducing the load on the CI system and therefore gives the developers faster turn-around times.
If developers want to update only the MR title, description or labels, this can all be done directly from the GitLab MR page using the
Edit button in the
How do developers notify review shifters after they responded to comments?
If a MR as been flagged as
review-user-action-required, developers should
answer and address the questions raised by the review shifters. In case they
updated the source branch, a new CI job will reset the review label to
review-pending-level-1 automatically. In the event that no code changes were
required, developers should manually change the label back to
review-pending-level-1/2 (whatever they think is appropriate).
Who should resolve discussions?
Before a MR is approved, all discussions should be marked as resolved. In general, a discussion should be resolved by the initiator (which usually is the review shifter). If developers agree with a comment and they have implemented the requested change in an update of the MR, they should briefly state in the discussion that this suggestion was implemented and resolve this discussion directly.
How can I access
The Jenkins server is running at the above address which is only reachable from inside the CERN firewall due to security restrictions for our build machines. In order to access this machine which provides access to all the detailed Jenkins log files, you need use port forwarding or SSH tunneling. There are several ways of doing this. Below is an example which has been tested by several users:
ssh -N -L localhost:9999:atlas-sit-ci.cern.ch:8080 <username>@lxplus.cern.ch
Then you should be able to access the Jenkins server and all log files by
localhost:9999 in the URL.
Jenkins please retry a build
git checkout + git merge) can be accessed from the navigation menu on the left under
Console Output. In order to restart (or cancel) a CI job you need to login with you CERN account using the login button in the top-right corner of the screen. After a successful login, the navigation menu on the left should show a
Rebuildbutton. If that is not the case, your account needs to be added to the relevant Jenkins group as described above.
Where are the per-package build log files?
Once the CI job is finished ATLAS robot publishes a summary comment on the GitLab MR discussion page which contains a link to NICOS. On this page you can access the log files for the
git checkout + git merge operation, the cmake
configuration and the build of the externals by clicking on the three icons in
the Build time, checkout, conf, inst column. The column #PB
also gives a summary about the number of failed packages and packages which had
warnings (in parentheses). Clicking on these numbers gets you to a page which
lists the build logfiles for each individual package where the colors indicate
the severity of compiler warnings/errors.
WIP merge requests processed by the CI system?
Yes, new pushes to a merge request trigger the CI regardless of the WIP status (as of ATLINFR-2717), however if the MR is WIP then the system will not add a review label (as of ATLINFR-2754) since the WIP status indicates that the change is not yet ready for review.
Which expert to contact?
The CI system should automatically tag relevant experts using an expert watch list.
If no one is listed as a watcher (or if this list seems out of date), please
contact someone from the relevant software domain to fix it.
A list of contacts for various software domains (and CP groups) can be found
sure that you mentioned the expert in the GitLab using the
Why is the pipeline status for a merge request not displayed?
Make sure that
atlasbot was added as a developer to the source fork as
here. In case you
spot a MR where the source repository is lacking
atlasbot as a member, you
should make a comment like to the one below
Hi @<username>, please add `atlasbot` as a developer to your fork as described [here](https://atlassoftwaredocs.web.cern.ch/gittutorial/gitlab-fork/) so that the CI pipeline status is displayed correctly for future MRs. Thanks.
Where can I find the output of the unit tests?
Unfortunately, the output of the testing stage can only be viewed directly in the Jenkins logfiles for the moment. We are working on uploading the test results to NICOS.
endmsgin the whole code base would be a valid MR touching thousands of files. It is not the number of changes (or modified files) which should be important but rather whether those changes are all logically related. The goal is that each MR addresses one issue (e.g. one JIRA ticket) and does not contain multiple unrelated updates. If this requires changes to many files, it does not mean it is a bad request. You may as well see it from a different perspective: A merge request is the smallest entity on release level which can be undone easily in case there is a problem. So if a merge request contains multiple changes where there is the hypothetical possibility that you only want to undo certain parts if a bug is discovered, it is better to split the merge request.