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

User plugin ParameterSet validation #24622

Merged
merged 3 commits into from Sep 24, 2018
Merged

Conversation

Dr15Jones
Copy link
Contributor

Extended the ParameterSet validation system to be able to validate user plugins used by modules. See FWCore/ParameterSet/interface/PluginDescription.h for full documentation.

Extended the ParameterSet validation system to support the case where user plugins have different parameters. The code can now use a `fillPSetDescription` static member function of a plugin to validate the configuration before the module using the plugin is constructed. See the documentation in  FWCore/ParameterSet/interface/PluginDescription for the full details.
The module tests writing a cfi using the plugin validation and to visually inspect the results of edmPluginHelp
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

FWCore/Integration
FWCore/ParameterSet
FWCore/PluginManager

@cmsbuild, @smuzaffar, @Dr15Jones can you please review it and eventually sign? Thanks.
@makortel, @Martin-Grunewald, @wddgit this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 21, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/30559/console Started: 2018/09/21 17:40

@Dr15Jones Dr15Jones changed the title Plugin validation User plugin ParameterSet validation Sep 21, 2018
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 22, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/30568/console Started: 2018/09/22 23:44

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24622/30568/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 3146746
  • DQMHistoTests: Total failures: 77
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3146472
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
  • Checked 133 log files, 14 edm output root files, 32 DQM output files

@Dr15Jones
Copy link
Contributor Author

@fabiocos the histograms failures all appear to be from the L1 histograms which are known to not be reliably reproducible.

@Dr15Jones
Copy link
Contributor Author

@fabiocos could we get this into the release soon? A reconstruction based pull request is waiting for this to be in the release so they can update their pull request to use it.

@fabiocos
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit ff5b239 into cms-sw:master Sep 24, 2018
@fabiocos
Copy link
Contributor

@davidlange6
Copy link
Contributor

@Dr15Jones
Copy link
Contributor Author

@smuzaffar It looks like the error pointed out by David shows that we need to change the order the build runs edmWriteConfigs and registration of plugins. This new feature requires that edmWriteConfigs must be able to load plugins some of the times.

@Dr15Jones
Copy link
Contributor Author

@fabiocos I can't see how my change could possibly have lead to the header file problem. Non of the new headerfiles I added are included in those other headers. The one existing header I modified was just me cut-n-paste then change a name on two macros, neither macro being used by files outside of this pull request.

I think you only noticed the header check failing because this pull request caused large number of packages to be checked out and recompiled and therefore also checked.

@smuzaffar
Copy link
Contributor

smuzaffar commented Sep 25, 2018

@Dr15Jones, For this feature we might have to change the way we register plugins. As plugins are registered in one cache file so we can not run it in parallel. Currently we run the plugin registration at the end in a single process. Can we create one plugin cache per plugin (pluginname.edmcache)? this will allow us to generate these caches in parallel and just concatenate them at the end to generate on big cache?

@Dr15Jones
Copy link
Contributor Author

@smuzaffar I think it is different than that. The user plugin being referenced in the cfi file does not have to come from the plugin holding the module. Therefore edmWriteConfigs can only be run once the edmplugincache file has been merged since we have no other way to know where to look for the user plugin.

@Dr15Jones
Copy link
Contributor Author

@fabiocos @davidlange6 I will have a workaround for the IB problem coming in the next few minutes.

@smuzaffar Unfortunately, I have no way to really test the change since it requires the lack of .edmplugincache from being found.

@Dr15Jones
Copy link
Contributor Author

@smuzaffar I was able to devise a way to test my change. It does avoid the problem in the IB.

@Dr15Jones
Copy link
Contributor Author

#24656 fixes the problem in the IB.

@Dr15Jones Dr15Jones deleted the pluginValidation branch September 27, 2018 17:47
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

6 participants