Improve PluginDescription with VPSets #35477

wddgit · 2021-09-29T20:47:28Z

PR description:

This allows the ParameterSetDescriptions to be different for each element of a VPSet in the special case where the elements hold a PluginDescription. Previously all elements would need to have the same plugin type.

PR validation:

Extended an existing unit test to cover this case. It seems to only really be different in the ParameterSetDescription part of things in a minor way.

This allows the ParameterSetDescription to be different for each element of a VPSet in the special case where the elements hold a PluginDescription.

cmsbuild · 2021-09-29T20:55:15Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35477/25640

This PR adds an extra 44KB to repository

cmsbuild · 2021-09-29T20:55:40Z

A new Pull Request was created by @wddgit (W. David Dagenhart) for master.

It involves the following packages:

FWCore/Integration (core)
FWCore/ParameterSet (core)

@makortel, @smuzaffar, @cmsbuild, @Dr15Jones can you please review it and eventually sign? Thanks.
@makortel 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

wddgit · 2021-09-29T21:01:26Z

please test

makortel · 2021-09-29T23:33:00Z

FWCore/ParameterSet/interface/PluginDescription.h

@@ -124,7 +124,7 @@ namespace edm {

    void validate_(ParameterSet& pset, std::set<std::string>& validatedLabels, bool optional) const final {
      loadPlugin(findType(pset));
-      cache_->validate(pset);
+      parameterSetDescription_->validate(pset);


The parameterSetDescription_ appears to be used only right after loadPlugin(). How about loadPlugin() would return the ParameterSetDescription object for which the validate() (and writeCfi() below) are called?

Or is there some other need to keep the object alive longer?

I agree with Matti.

I agree also. That is even better. I made the change and am running the unit tests. I will push it as soon as they complete. Thanks.

makortel · 2021-09-29T23:33:15Z

@Dr15Jones Do you have any concerns?

cmsbuild · 2021-09-30T00:53:28Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8c4756/19256/summary.html
COMMIT: 10fedd8
CMSSW: CMSSW_12_1_X_2021-09-29-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35477/19256/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: 10 differences found in the comparisons
DQMHistoTests: Total files compared: 40
DQMHistoTests: Total histograms compared: 3211080
DQMHistoTests: Total failures: 14
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3211043
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 39 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 169 log files, 37 edm output root files, 40 DQM output files
TriggerResults: no differences found

Dr15Jones · 2021-09-30T14:22:32Z

Thinking a bit more, what if we took a different tack. Keep the cache_ but add a std::string cacheName_. In the load, compare the passed in name to cacheName_ and if they match return. If they do not match, update the cache_ and reset the cacheName_ to the new name. In that way, we basically keep the preformance benefits of the cache (if the same type is used repreatedly) but properly handle multiple types.

wddgit · 2021-09-30T15:30:43Z

@Dr15Jones We could do that, but this cache isn't helping much anyway. If I am remembering how this works, we create a description run the validation for one module label and the description gets deleted. It doesn't help in cases where a VPSet isn't used, because it is only used once. Even with a VPSet it would only help if there were consecutive elements of the same plugin type. The current issue is the first case where VPSet is used at all and Matti said they are only going to put one or two elements in the VPSet and it sounds like the types are different... Maybe this is premature or even unnecessary optimization... I only see 4 cases where the PluginDescription is used at all (hopefully that will change, I think there are significantly more cases where it would be useful...) I don't feel strongly. What you suggest will work and if you and Matti agree I will put it in. At worst it slightly complicates the code in PluginDescription. Is there some use case where this optimization will make a significant performance difference?

Dr15Jones · 2021-09-30T15:32:20Z

I have no problem dropping the cache_ until a time that we see it as a performance need.

cmsbuild · 2021-09-30T15:42:46Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35477/25660

This PR adds an extra 44KB to repository

wddgit · 2021-09-30T15:42:59Z

please test

cmsbuild · 2021-09-30T15:43:07Z

Pull request #35477 was updated. @makortel, @smuzaffar, @Dr15Jones can you please check and sign again.

cmsbuild · 2021-09-30T19:59:28Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8c4756/19293/summary.html
COMMIT: 85bc95c
CMSSW: CMSSW_12_1_X_2021-09-30-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35477/19293/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: 40
DQMHistoTests: Total histograms compared: 3211080
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3211052
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 39 files compared)
Checked 169 log files, 37 edm output root files, 40 DQM output files
TriggerResults: no differences found

makortel · 2021-09-30T20:40:17Z

+1

cmsbuild · 2021-09-30T20:40:38Z

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)

perrotta · 2021-10-01T11:19:28Z

+1

Improve PluginDescription with VPSets

10fedd8

This allows the ParameterSetDescription to be different for each element of a VPSet in the special case where the elements hold a PluginDescription.

cmsbuild added this to the CMSSW_12_1_X milestone Sep 29, 2021

cmsbuild added code-checks-pending core-pending orp-pending pending-signatures tests-pending labels Sep 29, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 29, 2021

cmsbuild added tests-started and removed tests-pending labels Sep 29, 2021

This was referenced Sep 29, 2021

Using PluginDescription for VPSet leads to validation failure if the vector contains different plugins #35426

Closed

[WIP] add fillPSetDescription to CkfTrackCandidateMakerBase and propagate it to CkfTrackCandidateMaker #35385

Closed

makortel mentioned this pull request Sep 29, 2021

Allow elements of VPSet of PluginDescriptions to point to different plugins cms-sw/framework-team#289

Closed

makortel reviewed Sep 29, 2021

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Sep 30, 2021

Minor simplification, remove unneeded data member

85bc95c

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Sep 30, 2021

cmsbuild added code-checks-approved tests-started and removed tests-pending code-checks-pending labels Sep 30, 2021

cmsbuild added tests-approved and removed tests-started labels Sep 30, 2021

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

cmsbuild added orp-approved and removed orp-pending labels Oct 1, 2021

cmsbuild merged commit 44523e5 into cms-sw:master Oct 1, 2021

wddgit deleted the pluginDescVPSets branch May 2, 2022 14:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve PluginDescription with VPSets #35477

Improve PluginDescription with VPSets #35477

wddgit commented Sep 29, 2021

cmsbuild commented Sep 29, 2021

cmsbuild commented Sep 29, 2021

wddgit commented Sep 29, 2021

makortel Sep 29, 2021

Dr15Jones Sep 30, 2021

wddgit Sep 30, 2021

makortel commented Sep 29, 2021

cmsbuild commented Sep 30, 2021

Dr15Jones commented Sep 30, 2021

wddgit commented Sep 30, 2021

Dr15Jones commented Sep 30, 2021

cmsbuild commented Sep 30, 2021

wddgit commented Sep 30, 2021

cmsbuild commented Sep 30, 2021

cmsbuild commented Sep 30, 2021

makortel commented Sep 30, 2021

cmsbuild commented Sep 30, 2021

perrotta commented Oct 1, 2021

Improve PluginDescription with VPSets #35477

Improve PluginDescription with VPSets #35477

Conversation

wddgit commented Sep 29, 2021

PR description:

PR validation:

cmsbuild commented Sep 29, 2021

cmsbuild commented Sep 29, 2021

wddgit commented Sep 29, 2021

makortel Sep 29, 2021

Choose a reason for hiding this comment

Dr15Jones Sep 30, 2021

Choose a reason for hiding this comment

wddgit Sep 30, 2021

Choose a reason for hiding this comment

makortel commented Sep 29, 2021

cmsbuild commented Sep 30, 2021

Comparison Summary

Dr15Jones commented Sep 30, 2021

wddgit commented Sep 30, 2021

Dr15Jones commented Sep 30, 2021

cmsbuild commented Sep 30, 2021

wddgit commented Sep 30, 2021

cmsbuild commented Sep 30, 2021

cmsbuild commented Sep 30, 2021

Comparison Summary

makortel commented Sep 30, 2021

cmsbuild commented Sep 30, 2021

perrotta commented Oct 1, 2021