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

Revert "Workflow 511.1: herwig7+ MG5+Openloops pptoee at NLO QCD" #24995

Merged
merged 1 commit into from
Oct 31, 2018

Conversation

fabiocos
Copy link
Contributor

Reverts #24813

The added workflow systematically fails the 2 hours time limit of the IB, it is clearly not suitable for regular test. This may point to different issues:

  • recalculation of matrix elements at every generation run instead of pre-computing them (a la gridpack), this looks a questionable structural choice;
  • according to @efeyazgan openloops seems here much slower than in association with sherpa. Provided the comparison is on a similar process, this might point to some integration issues;
  • according to @davidlange6 checks the code is likely not optimal in terms of performances in several places.
    In agreement with @efeyazgan I revert this PR, as it looks pointless. A follow up should be done in view of possible use of this generator configuration in production (it looks quite inefficient).

@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 @fabiocos (Fabio Cossutti) for master.

It involves the following packages:

Configuration/Generator
Configuration/PyReleaseValidation

@cmsbuild, @efeyazgan, @zhenhu, @perrozzi, @prebello, @kpedro88, @pgunnell, @alberto-sanchez, @qliphy can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @makortel 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

@fabiocos
Copy link
Contributor Author

please test

purely academic in this case

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 25, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/31266/console Started: 2018/10/25 14:52

@smuzaffar
Copy link
Contributor

@fabiocos , @davidlange6 has built openloops with optimization ( cms-sw/cmsdist#4448 ) and workflow 511.1 ran with in 1h15. I would suggest to get the cmsdist PR merged

@davidlange6
Copy link
Contributor

davidlange6 commented Oct 25, 2018 via email

@fabiocos
Copy link
Contributor Author

@davidlange6 thank you for the update. @efeyazgan @qliphy confirm my understanding that there is no initial integration providing gridpacks that can be reused, which makes this approach suboptimal in any case. This is still under work by authors. Anyway you work could be useful for future developments as well, regardless of the specific problem of this test. Let's merge your fix and see, In case this PR can be put on hold ot just closed if the GEN conveners see a benefit to keep the test in the sequence (as it was originally proposed..)

@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-24995/31266/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 259 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 2994843
  • DQMHistoTests: Total failures: 310
  • DQMHistoTests: Total nulls: 3
  • DQMHistoTests: Total successes: 2994333
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.136 KiB( 31 files compared)
  • DQMHistoSizes: changed ( 136.85,... ): 0.004 KiB MessageLogger/Warnings
  • DQMHistoSizes: changed ( 136.85,... ): 0.004 KiB MessageLogger/Errors
  • Checked 134 log files, 14 edm output root files, 32 DQM output files

@fabiocos
Copy link
Contributor Author

hold

@cmsbuild
Copy link
Contributor

Pull request has been put on hold by @fabiocos
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

@cmsbuild cmsbuild added the hold label Oct 25, 2018
@efeyazgan
Copy link
Contributor

"> @davidlange6 thank you for the update. @efeyazgan @qliphy confirm my understanding that there is no initial integration providing gridpacks that can be reused, which makes this approach suboptimal in any case. This is still under work by authors. Anyway you work could be useful for future developments as well, regardless of the specific problem of this test. Let's merge your fix and see, In case this PR can be put on hold ot just closed if the GEN conveners see a benefit to keep the test in the sequence (as it was originally proposed..)"

@fabiocos Yes, there is no "Herpack" yet (@Andrej-CMS can give more info).

@Andrej-CMS
Copy link
Contributor

@efeyazgan
Dear Efe,
This is indeed not yet available, but I'm working on it.

@fabiocos
Copy link
Contributor Author

The updated by @davidlange6 of the external has indeed prevented the TimeOut problem. Still is this the way we want to run? Is the GEN group willing to keep this test waiting for a more production-friendly approach?

@efeyazgan
Copy link
Contributor

+1

@prebello
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor Author

unhold

@cmsbuild cmsbuild removed the hold label Oct 31, 2018
@fabiocos
Copy link
Contributor Author

+1

from discussions with the GEN conveners, the workflow needs a revision where LHE files are allowed.
This is still work in progress, and there is no need to exercise at every IB this workflow as it is.

@fabiocos
Copy link
Contributor Author

merge

@cmsbuild cmsbuild merged commit c7e4032 into master Oct 31, 2018
@fabiocos fabiocos mentioned this pull request Nov 6, 2018
@smuzaffar smuzaffar deleted the revert-24813-Workflow511.1 branch December 10, 2018 20:23
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.

7 participants