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

update of hltIntegrationTests and path of Run-3 Data file for TSG tests #37283

Merged
merged 2 commits into from
Mar 23, 2022

Conversation

missirol
Copy link
Contributor

@missirol missirol commented Mar 20, 2022

PR description:

This PR includes (1) an update of the scripts hltIntegrationTests and hltListPaths maintained by TSG, and (2) the update of the path to the EDM file used for TSG tests on Run-3 Data.

Regarding (1): [FYI: @Sam-Harper]

  • The main goal of this update is to omit from the HLT integration tests Paths that depend on the result of other Paths in the same job (said Paths are not suited for the current integration tests, as they cannot be tested without other Paths).

  • This use case applies, for example, to the so-called Dataset Paths (i.e. Paths that select events based on the result of other Paths via the TriggerResultsFilter plugin), which will be used in the Run-3 HLT menus (see CMSHLT-2245).

  • The hltListPaths script is extended with a few cmd-line options to ignore certain Paths, and some of its internals are improved accordingly. The update of hltIntegrationTests is minimal, to simply make use of the new functionalities of hltListPaths (plus, a related update to hltCompareResults was also needed).

Regarding (2):

  • the EDM file was copied to the TSG area on EOS in order to avoid issues with remote file access, which seems to have caused trouble lately in IBs (see the failure of the HLT Validation tests for CMSSW_12_3_X_2022-03-19-1100).

PR validation:

TSG tests.

Example command for (1):

hltListPaths /users/missirol/test/dev/CMSSW_12_3_0/CMSHLT_2245/IntegTest_v01/HLT -p --no-dep --exclude "^HLTriggerFinalPath$" "^Dataset_.+" "^Status_.+"

If this PR is a backport, please specify the original PR and why you need to backport that PR:

N/A

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37283/28911

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @missirol (Marino Missiroli) for master.

It involves the following packages:

  • HLTrigger/Configuration (hlt)

@cmsbuild, @missirol, @Martin-Grunewald can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @silviodonato this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@missirol
Copy link
Contributor Author

please test

@Martin-Grunewald
Copy link
Contributor

Martin-Grunewald commented Mar 21, 2022

It seems this requires an additional txt file, different for each menu, in the same directory, which is a book-keeping nightmare. Would it be possible to remove unwanted paths otherwise, say, by a policy/convention to not consider *Dataset* and *Group* paths (grep -v)?

@Martin-Grunewald
Copy link
Contributor

Do you know what happened to the EDM data file? Typically RAW files of data are kept indefinitely.

@Martin-Grunewald
Copy link
Contributor

Martin-Grunewald commented Mar 21, 2022

If the RAW file is really removed, we'd also need to update addOnTestsHLT.py in Configuration/HLT/python

@missirol
Copy link
Contributor Author

missirol commented Mar 21, 2022

Do you know what happened to the EDM data file? Typically RAW files of data are kept indefinitely.

I don't really know. My guess is that the file was previously at CERN, so the following in the integration tests was working

root://eoscms.cern.ch//eos/cms/store/data/Commissioning2021/MinimumBias1/RAW/v1/000/346/304/00000/0949cd03-66a6-4034-a630-b9fef4dde3d2.root

Now, the file is not at CERN anymore, so the above path fails, but the EDM file is at (at least) 1 T2, so the following path used in the addOnTestsHLT should still work

/store/data/Commissioning2021/MinimumBias1/RAW/v1/000/346/304/00000/0949cd03-66a6-4034-a630-b9fef4dde3d2.root

I understood that the input files used for RelVal wfs are cached by the cmsbot (so they remain available), but I don't know if this applies also to the addOnTests. It's probably safer to change also the path of this EDM file in addOnTestsHLT. It can be done in this PR.

@Martin-Grunewald
Copy link
Contributor

Sorry, but the simply removing root://eoscms.cern.ch//eos/cms and just leaving /store/... should work?

@missirol
Copy link
Contributor Author

missirol commented Mar 21, 2022

It seems this requires an additional txt file, different for each menu, in the same directory, which is a book-keeping nightmare. Would it be possible to remove unwanted paths otherwise, say, by a policy/convention to not consider *Dataset* and *Group* paths (grep -v)?

Yes, I think it would be possible if we define a naming convention. On the other hand, this didn't seem to me a book-keeping problem, because the new text file paths.txt is a transient one, created by the integration tests the same way other files are (e.g. hlt.py and hlt.done), and we would not ever track it with git / maintain it ourselves.

Besides this point, what is maybe non-intuitive is that the hltCompareResults script relies heavily on the existence of files created by hltIntegrationTests; in a way, hltCompareResults is a short script that is likely never used standalone, so that code could simply be moved inside hltIntegrationTests, to make the latter a bit more self-contained.

@Martin-Grunewald
Copy link
Contributor

Sorry, I understand that the list of paths are currently extracted from the menu itself, while you want to read it from a pre-existing file (presumably allowing to exclude a few paths), no?

@missirol
Copy link
Contributor Author

Sorry, I understand that the list of paths are currently extracted from the menu itself, while you want to read it from a pre-existing file (presumably allowing to exclude a few paths), no?

Mh, yes but pre-existing means "created at the start of the integration tests with hltListPaths" (paths.txt does not exist before one launches the integration tests, and it lives inside the output folder HLT_Integration_*/). The ConfDB config used to fill paths.txt is the same that is later used to create hlt.py, so the two will be consistent by construction (modulo the restriction that we apply to remove Dataset_ paths and the like from paths.txt).

@missirol
Copy link
Contributor Author

missirol commented Mar 21, 2022

Sorry, but the simply removing root://eoscms.cern.ch//eos/cms and just leaving /store/... should work?

Yes, I think so [1], but afaiu this assumes that the file will always be available at a T2 (and I don't know if that is a safe assumption). For example, right now that file is only avaiable at one T2 [2].

Overall, I thought the most reliable option would be to read the file from the TSG area on EOS, but if /store/data/[..].root is preferred, I'll update the PR with that.

[1] This indeed works

root root://cms-xrd-global.cern.ch//store/data/Commissioning2021/MinimumBias1/RAW/v1/000/346/304/00000/0949cd03-66a6-4034-a630-b9fef4dde3d2.root

[2]

> dasgoclient -query "site file=/store/data/Commissioning2021/MinimumBias1/RAW/v1/000/346/304/00000/0949cd03-66a6-4034-a630-b9fef4dde3d2.root"
T0_CH_CERN_Tape
T1_DE_KIT_Disk
T1_RU_JINR_Tape
T2_US_Wisconsin

@Martin-Grunewald
Copy link
Contributor

Hmm, but the addOn HLT tests use the short form with just /store/... and still fail.

@missirol
Copy link
Contributor Author

Hmm, but the addOn HLT tests use the short form with just /store/... and still fail.

Mh, I missed that. In this PR, they seem to have passed. Where did they fail?

@Martin-Grunewald
Copy link
Contributor

Martin-Grunewald commented Mar 21, 2022

See recent IBs (122, 123, 124) which have completed the HLT tests - now in red with the above failure.
For some reason root://eoscms.cern.ch//eos/cms seems to be pre-pended.

@missirol
Copy link
Contributor Author

See recent IBs (122, 123, 124) which have completed the HLT tests - now in red with the above failure. For some reason root://eoscms.cern.ch//eos/cms seems to be pre-pended.

I don't know these workflows well, but here's my understanding.

  • What I see failing in the IB dashboard (with "HLT" marked by a red box) is the "HLT-Validation" tests, which I thought use the same scripts as the integration tests, i.e. ./runAll.csh IB. It makes sense to see the root://eoscms.cern.ch//eos/cms prefix here, because that's what is in cmsDriver.csh, and thus they fail.
  • Afaiu, the addOnTests are independent of the "HLT-Validation" tests, and I actually don't know if the addOnTests are executed in IBs (I can't find them), or only in PR testing. The addOnTests should not fail, and in recent PRs they indeed do not.

@Martin-Grunewald
Copy link
Contributor

Ah yes, sorry, I confused the two. Well, I'd prefer to just remove the cern specific location prefix to make things work, and it seems it needs to be backported from 12_4 to 12_2 and 12_3.

@missirol
Copy link
Contributor Author

Yes (and I can take care of the backports, no problem). The update of the hltIntegrationTests would also need to be backported to 12_3_X, if we plan to have a GRun with Dataset Paths in 12_3_X eventually (which is the case, in my understanding).

missirol and others added 2 commits March 22, 2022 23:33
The "hltListPaths" script is updated to support FinalPaths, give the option to ignore paths by name (by means of regular expressions), and give the option to ignore paths that depend on the result of other paths in the same job.

The "hltIntegrationTests" script is updated to ignore Paths that depend on the result of other Paths in the same job, as by construction those cannot be tested without other Paths.
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37283/28949

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

Pull request #37283 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again.

@missirol
Copy link
Contributor Author

please test

Propagated the update of the EDM file-path to the addOnTests.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8ca683/23298/summary.html
COMMIT: 62f9150
CMSSW: CMSSW_12_4_X_2022-03-22-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37283/23298/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

There are some workflows for which there are errors in the baseline:
534.0 step 1
The results for the comparisons for these workflows could be incomplete
This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-8ca683/39434.911_TTbar_14TeV+2026D88_DD4hep+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3695650
  • DQMHistoTests: Total failures: 10
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3695617
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 48 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 204 log files, 45 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@missirol
Copy link
Contributor Author

+hlt

  • updates scripts used for testing of HLT menus
  • fixes a problem with file-access in the HLT-Validation tests of IBs

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@missirol
Copy link
Contributor Author

Note: the bin-by-bin comparisons are not relevant for this PR (the latter cannot affect them by definition). In any case, I was trying to check them, and I could not see plots in the usual DQM pages (for example, if I click on "10042.0 overlay GUI (3)" here, I see 0 plots instead of 3), but maybe this is just an issue on my side.

@perrotta
Copy link
Contributor

+1

  • HLT scripts updated according to the original plan and follow-up discussions

@cmsbuild cmsbuild merged commit 6d06242 into cms-sw:master Mar 23, 2022
@missirol missirol deleted the devel_hltIntegTests branch March 23, 2022 09:12
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.

None yet

4 participants