[12_4_X] Use "best matching" EDAlias in module dependence checks when placing ConditionalTask modules in Path #39530

makortel · 2022-09-28T20:34:29Z

PR description:

Backport of #39409. From original description

This PR fixes the problem reported in #36938 (comment). If the EDAlias has wildcards for one module, and more specific aliases for another module, we should use the one that has the most constrained match. There is one ambiguous case left, for which an exception is thrown. I hope we won't encounter that situation in practice.

PR validation:

Tests of #39409, also the original development

…ConditionalTask modules in Path If the EDAlias has wildcards for one module, and more specific aliases for another module, we should use the one that has the most constrained match. There is one ambiguous case left, for which an exception is thrown. I hope we won't encounter that situation in practice. The problem was encountered with a SwitchProducer having the EDAlias case, but I think the problem could happen also without SwitchProducer.

cmsbuild · 2022-09-28T20:34:48Z

A new Pull Request was created by @makortel (Matti Kortelainen) for CMSSW_12_4_X.

It involves the following packages:

FWCore/Framework (core)
FWCore/Integration (core)

@cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please review it and eventually sign? Thanks.
@missirol, @wddgit this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

Backported from Use "best matching" EDAlias in module dependence checks when placing ConditionalTask modules in Path #39409

makortel · 2022-09-28T20:35:18Z

type bug-fix

makortel · 2022-09-28T20:35:27Z

backport of #39409

makortel · 2022-09-28T20:35:35Z

@cmsbuild, please test

cmsbuild · 2022-09-29T00:28:18Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7e98ac/27830/summary.html
COMMIT: 708b8d3
CMSSW: CMSSW_12_4_X_2022-09-28-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39530/27830/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3677402
DQMHistoTests: Total failures: 8
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3677372
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
Checked 208 log files, 45 edm output root files, 50 DQM output files
TriggerResults: no differences found

makortel · 2022-09-30T14:25:27Z

+1

cmsbuild · 2022-09-30T14:25:48Z

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

perrotta

Could you please tell where this difference wrt what merged in master and in 12_5_X comes from?
Moreover, could you explain why is this needed for 12_4_X: will HLT request a new release for the last few weeks of 2022 data taking with this fix?

perrotta · 2022-10-01T06:12:10Z

FWCore/Framework/src/StreamSchedule.cc

@@ -143,6 +144,74 @@ namespace edm {

      return workerManager.getWorker(*modpset, preg, prealloc, processConfiguration, moduleLabel);
    }
+
+    std::optional<std::string> findBestMatchingAlias(
+        std::multimap<std::string, edm::BranchDescription const*> const& conditionalModuleBranches,


These two are std::unordered_multimap in master and 12_5_X

makortel · 2022-10-03T13:26:41Z

Could you please tell where this difference wrt what merged in master and in 12_5_X comes from?

The multimap vs unordered_multimap difference comes from #38790 that was merged for 12_5_X. I didn't backport it at the time because the performance difference looked negligible (ok, one could ask if it would make sense to backport it now with the argument of keeping the commit of this PR the same as in 12_5_X and master).

Moreover, could you explain why is this needed for 12_4_X: will HLT request a new release for the last few weeks of 2022 data taking with this fix?

I blindly provided a fix for the release where the problem was seen. I leave it to @cms-sw/hlt-l2 to comment if it is really needed in 12_4_X for the remaining 2022 data taking. (and if not, I'm fine with just closing this PR).

Martin-Grunewald · 2022-10-03T14:56:14Z

Currently the HLT menu uses your fix of providing the collection labels explicitely, rather than using * so I suppose it is not strictly needed for 12_4.

makortel · 2022-10-04T15:38:25Z

Closing following the discussion in ORP. If need arises nevertheless, I can reopen the PR.

cmsbuild added this to the CMSSW_12_4_X milestone Sep 28, 2022

cmsbuild added core-pending orp-pending pending-signatures tests-pending labels Sep 28, 2022

makortel changed the title ~~[12_4_X]Use "best matching" EDAlias in module dependence checks when placing ConditionalTask modules in Path~~ [12_4_X] Use "best matching" EDAlias in module dependence checks when placing ConditionalTask modules in Path Sep 28, 2022

cmsbuild added the bug-fix label Sep 28, 2022

cmsbuild added the backport label Sep 28, 2022

cmsbuild added tests-started backport-ok and removed tests-pending backport labels Sep 28, 2022

This was referenced Sep 28, 2022

on running unscheduled HLT modules together with other "steps" #36938

Open

Fix a problem in EDAlias with wildcard used in SwitchProducer in ConditionalTask cms-sw/framework-team#437

Closed

cmsbuild added tests-approved and removed tests-started labels Sep 29, 2022

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Sep 30, 2022

perrotta reviewed Oct 1, 2022

View reviewed changes

makortel closed this Oct 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[12_4_X] Use "best matching" EDAlias in module dependence checks when placing ConditionalTask modules in Path #39530

[12_4_X] Use "best matching" EDAlias in module dependence checks when placing ConditionalTask modules in Path #39530

makortel commented Sep 28, 2022

cmsbuild commented Sep 28, 2022 •

edited

makortel commented Sep 28, 2022

makortel commented Sep 28, 2022

makortel commented Sep 28, 2022

cmsbuild commented Sep 29, 2022

makortel commented Sep 30, 2022

cmsbuild commented Sep 30, 2022

perrotta left a comment

perrotta Oct 1, 2022

makortel commented Oct 3, 2022

Martin-Grunewald commented Oct 3, 2022

makortel commented Oct 4, 2022

[12_4_X] Use "best matching" EDAlias in module dependence checks when placing ConditionalTask modules in Path #39530

[12_4_X] Use "best matching" EDAlias in module dependence checks when placing ConditionalTask modules in Path #39530

Conversation

makortel commented Sep 28, 2022

PR description:

PR validation:

cmsbuild commented Sep 28, 2022 • edited

makortel commented Sep 28, 2022

makortel commented Sep 28, 2022

makortel commented Sep 28, 2022

cmsbuild commented Sep 29, 2022

Comparison Summary

makortel commented Sep 30, 2022

cmsbuild commented Sep 30, 2022

perrotta left a comment

Choose a reason for hiding this comment

perrotta Oct 1, 2022

Choose a reason for hiding this comment

makortel commented Oct 3, 2022

Martin-Grunewald commented Oct 3, 2022

makortel commented Oct 4, 2022

cmsbuild commented Sep 28, 2022 •

edited