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

allow scram_arch and cmssw version manipulation from the ExternalLHEP… #19457

Merged
merged 1 commit into from Jul 17, 2017

Conversation

perrozzi
Copy link
Contributor

…roducer

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @perrozzi for master.

It involves the following packages:

GeneratorInterface/LHEInterface

@cmsbuild, @govoni, @perrozzi, @thuer, @davidlange6 can you please review it and eventually sign? Thanks.
@alberto-sanchez, @agrohsje, @mkirsano this is something you requested to watch as well.
@davidlange6 you are the release manager for this.

cms-bot commands are listed here

@davidlange6
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 28, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20965/console Started: 2017/06/28 15:07

@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-19457/20965/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 22
  • DQMHistoTests: Total histograms compared: 1756063
  • DQMHistoTests: Total failures: 88
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 1755809
  • DQMHistoTests: Total skipped: 166
  • DQMHistoTests: Total Missing objects: 0
  • Checked 90 log files, 14 edm output root files, 22 DQM output files

@@ -225,7 +225,8 @@ ExternalLHEProducer::beginRunProduce(edm::Run& run, edm::EventSetup const& es)

std::ostringstream eventStream;
eventStream << nEvents_;
args_.push_back(eventStream.str());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was actually wrong with using push_back and/or emplace_back?

Copy link
Contributor Author

@perrozzi perrozzi Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The string vector args, defined in the job cfg, conventionally only contains the gridpack path (P), so args = [P].
Currently ExternalLHEProducer appends the number of events (N), random seed (S) and number of threads (T) to arg,
producing therfore args* = [P,N,S,T] which is expected by run_generic_tarball_cvmfs.sh in this order.

If one adds other parameters (O) to args in the cfg, args = [P,O] and args* = [P,O,N,S,T] therefore run_generic_tarball_cvmfs.sh breaks.
What I did was to enforce args* = [P,N,S,T,O] in ExternalLHEProducer keeping back-compatibility in run_generic_tarball_cvmfs.sh, further allowing O to be passed (whenever present) to runcmsgrid.sh with
./runcmsgrid.sh $nevt $rnum $ncpu ${@:5}
where ${@:5} means "all the arguments passed to the script starting from the 5th, i.e. O"

I tested and seems to work flawlessly, I hope is clear.
Let me know if you have further comments or questions

@perrozzi
Copy link
Contributor Author

perrozzi commented Jul 3, 2017

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 3, 2017

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 requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar

@davidlange6
Copy link
Contributor

hi @perrozzi - this is allowing quite a lot more freedom than suggested by the PR title (basically anything can now be passed). Perhaps its possible to move a bit more of this interface out of the external script and into CMSSW ? [or is that all coming as a second step?]

@perrozzi
Copy link
Contributor Author

perrozzi commented Jul 4, 2017

Hi David,
args is defined as a vector of strings so I am just allowing more arguments to be passed to down to runcmsgrid.sh (so far only the gridpack path was used) keeping a defined order for back-compatibility.
Here are a couple of slides to illustrate the complete picture including modification to genproductions (the last slide contains an executive summary)
https://docs.google.com/presentation/d/1A0Cb40zyNgnZ1jppl23WSiHGr1MOiuoz1iscqrKy4hs/edit#slide=id.g2247f7bf2f_0_0

Bottom line is that

  1. by default only one argument will continue to be passed as today, i.e. the gridpack full path, and runcmsgrid.sh will now source the scram_arch and CMSSW version used to create the gridpack

More flexibility will be possible, in case of need, after merging PR in cmssw:

2.1) stick to campaign scram_arch and CMSSW by adding “false” parameter after gridpack full path
2.2) enforce a different combination of scram_arch and cmssw by adding “true” parameter after gridpack full path and then chosen scram_arch and CMSSW

Let me know if anything is not clear or if you have comments.
Luca

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 5259178 into cms-sw:master Jul 17, 2017
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