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

external lib for HYDJET++ #7483

Merged

Conversation

wouf
Copy link
Contributor

@wouf wouf commented Dec 1, 2021

Hydjet++ (v2.4.3) integration as external.
RP cmssw: cms-sw/cmssw#36316

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2021

A new Pull Request was created by @wouf for branch IB/CMSSW_12_2_X/master.

@cmsbuild, @smuzaffar, @iarspider, @ddaina can you please review it and eventually sign? Thanks.
@perrotta, @dpiparo, @qliphy you are the release manager for this.
cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2021

Pull request #7483 was updated.

@smuzaffar
Copy link
Contributor

assign generators

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2021

New categories assigned: generators

@mkirsano,@alberto-sanchez,@SiewYan,@GurpreetSinghChahal,@Saptaparna,@agrohsje you have been requested to review this Pull request/Issue and eventually sign? Thanks

hydjet2.spec Outdated
@@ -0,0 +1,20 @@
### RPM external hydjet2 2.x.y
Copy link
Contributor

Choose a reason for hiding this comment

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

please provide the exact version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we put here the link to the actual version instead of hardcoded number?

Copy link
Contributor

Choose a reason for hiding this comment

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

what do you mean ? can you please give an example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like

RPM external hydjet2 version

where the variable version would be determined from the link to the json file on the author's site, or just from the some text file, placed on the author's website.
Mayby ### RPM external hydjet2 http://..../version
Would it work?

Copy link
Contributor

Choose a reason for hiding this comment

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

no, it will not work. One needs to explicitly provide the version to be used here and then you can use the version to auto generate the source url e.g.

### RPM externals hydjet2 2.4.3
Source: http://cern.ch/lokhtin/hydjet++/%{n}-%{realversion}.tar.gz

So in future when you update the version to e.g. 2.4.4 then package system will try to download http://cern.ch/lokhtin/hydjet++/hydjet2-2.4.4.tar.gz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But only after PR, right? I'm looking a way to avoid of necessity to make PR for every minor update of HYDJET++

Copy link
Contributor

@smuzaffar smuzaffar Dec 2, 2021

Choose a reason for hiding this comment

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

We do not want our build system to automatically pick up new updates which can break the whole release. imagine that we have tested one version in IBs and when we want to build a release then author updates the version which can break the whole system.

When there is a new version then it should be explicitly requested, tested via PR before we integrate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I see, thanks!

hydjet2.spec Outdated
@@ -0,0 +1,20 @@
### RPM external hydjet2 2.x.y

Source: http://cern.ch/lokhtin/hydjet++/%{n}.tar.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

the URL http://cern.ch/lokhtin/hydjet++/hydjet2.tar.gz is very generic and our packaging system is not going to redownload it when you need to update the version. Can you please provide separate tar files for each version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the author's site is just one version for the new HYDJET++ - the actual one. This naming policy implies to move all outdated version to other links/tarballs. The author is the member of CMS HI, since very beginning and he was updating PYQUEN and HYDJETs focusing primarily on the interests of the CMS. As far as I understand, during compiling of each CMSSW version, the external archive downloaded from the source website, so if author will update the version, it will be available for the next CMSSW version. It's ok for CMS HI group, we always need the more actual version on top.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or You mean difficulties related to the backport for the new version?

Copy link
Contributor

Choose a reason for hiding this comment

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

For each version of a package there should be a unique url. If author just keeps on updating http://cern.ch/lokhtin/hydjet++/hydjet2.tar.gz (pointing to latest version) then there is no guarantee that two cmssw builds will have the same contents. I have no issues with http://cern.ch/lokhtin/hydjet++/hydjet2.tar.gz pointing to latest version but there should link for specific versions too e.g. http://cern.ch/lokhtin/hydjet++/hydjet2-%{version}.tar.gz and this should be used in our spec files.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that cmsBuild ( our packaging system) also caches the contents of each source URL. So cmsBuild will always use the previously cached contents if url itself is not changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, thanks. I will ask for this changes.

@davidlange6
Copy link
Contributor

davidlange6 commented Dec 2, 2021 via email

@wouf
Copy link
Contributor Author

wouf commented Dec 2, 2021

I am in contact with the main author of HYDJET++. He prefer to have all items related to the event generator on his personal web-page, but he "doesn't mind if any necessary items would be duplicated via GitHub as well". But I think that stability of personal area does not matter, because of cmsBuild "also caches the contents of each source URL. So cmsBuild will always use the previously cached contents if url itself is not changed".

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2021

Pull request #7483 was updated.

@smuzaffar
Copy link
Contributor

@wouf , looks like hydjet2 also makes a copy of libpyquen.so in hydjet2/lib dirctory [a]. Although we can delete it in our sopec file but I think proper fix should be to remove install(FILES ${PYQUEN_LIBRARIES} DESTINATION lib) line from hydjet2/CMakeLists.txt

>ls -ld hydjet2/2.4.3-f3c1bda44ebbd799b8162526a0512b93/lib/*
-rwxr-xr-x. 1 cvmfs cvmfs  220896 Dec  5 09:22 hydjet2/2.4.3-f3c1bda44ebbd799b8162526a0512b93/lib/libhydjet2.so
-rw-r--r--. 1 cvmfs cvmfs 1076656 Nov 12 05:31 hydjet2/2.4.3-f3c1bda44ebbd799b8162526a0512b93/lib/libpyquen.so

@wouf
Copy link
Contributor Author

wouf commented Dec 5, 2021

install is needed for standalone usage. anyway ot makes a copy only if PYQUEN library absent in the system. I guess i have to adjust FindPYQUEN module for cmssw case. Where pyquen library should be looking for at compilation time?

@smuzaffar
Copy link
Contributor

cmake properly found cmssw pyquen

-- Found PYQUEN: /data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/slc7_amd64_gcc900/external/pyquen/1.5.4-edb141c01d142e7411bce63143948569/lib/libpyquen.so 

see the full build log https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cae5ac/20993/external/hydjet2/2.4.3-f3c1bda44ebbd799b8162526a0512b93/log . but because of install(FILES ${PYQUEN_LIBRARIES} DESTINATION lib) line it also forces it to install along with hydjet2 libs.

@wouf
Copy link
Contributor Author

wouf commented Dec 6, 2021

Thank you for this important catch! The problem is more global than it may seem. If someone checks libhydjet2.so , he would have found that its RUNPATH is setted:

`$ objdump -x /afs/cern.ch/work/w/wouf/TMP/2git/lib/slc7_amd64_gcc900/external/hydjet2/2.4.3-5c4bd156a11e3c022693e40677fa8e04/lib/libhydjet2.so | egrep 'R(|UN)PATH'

RPATH /afs/cern.ch/work/w/wouf/TMP/2git/lib/tmp/BUILDROOT/5c4bd156a11e3c022693e40677fa8e04/opt/cmssw/slc7_amd64_gcc900/external/hydjet2/2.4.3-5c4bd156a11e3c022693e40677fa8e04/lib:/afs/cern.ch/work/w/wouf/TMP/2git/lib/slc7_amd64_gcc900/external/pyquen/1.5.4-edb141c01d142e7411bce63143948569/lib:/afs/cern.ch/work/w/wouf/TMP/2git/lib/slc7_amd64_gcc900/external/lhapdf/6.4.0-d2f68e74e7b7722f8a01055c82dc1e31/lib
`
It's not really correct, especially for CMSSW. I will care about it.

@smuzaffar smuzaffar changed the base branch from IB/CMSSW_12_2_X/master to IB/CMSSW_12_3_X/master December 6, 2021 21:29
@wouf
Copy link
Contributor Author

wouf commented Dec 8, 2021

It's now fixed, please try again.

@smuzaffar
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0ab80e/21169/summary.html
COMMIT: fcac651
CMSSW: CMSSW_12_3_X_2021-12-09-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmsdist/7483/21169/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: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 42
  • DQMHistoTests: Total histograms compared: 3250704
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3250676
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
  • Checked 177 log files, 37 edm output root files, 42 DQM output files
  • TriggerResults: no differences found

@smuzaffar
Copy link
Contributor

please test
let me re-run the tests for all os/archs

@smuzaffar
Copy link
Contributor

please test for slc7_aarch64_gcc11

@smuzaffar
Copy link
Contributor

please test for slc7_ppc64le_gcc11

@smuzaffar
Copy link
Contributor

please test for cs8_amd64_gcc11

@cmsbuild
Copy link
Contributor

-1

Failed Tests: RelVals
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0ab80e/21753/summary.html
COMMIT: 4175cf6
CMSSW: CMSSW_12_3_X_2022-01-16-2300/slc7_aarch64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmsdist/7483/21753/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0ab80e/21753/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0ab80e/21753/git-merge-result

RelVals

----- Begin Fatal Exception 17-Jan-2022 19:39:09 CET-----------------------
An exception of category 'Vertex' occurred while
   [0] Processing  Event run: 194533 lumi: 329 event: 462355458 stream: 0
   [1] Running path 'dqmofflineOnPAT_1_step'
   [2] Prefetching for module SingleTopTChannelLeptonDQM_miniAOD/'singleTopElectronMediumDQM_miniAOD'
   [3] Prefetching for module PATMuonSlimmer/'slimmedMuons'
   [4] Prefetching for module PATMuonSelector/'selectedPatMuons'
   [5] Prefetching for module PATMuonProducer/'patMuons'
   [6] Prefetching for module MuonProducer/'muons'
   [7] Prefetching for module PFProducer/'particleFlowTmp'
   [8] Prefetching for module PFBlockProducer/'particleFlowBlock'
   [9] Prefetching for module PFElecTkProducer/'pfTrackElec'
   [10] Prefetching for module PFConversionProducer/'pfConversions'
   [11] Calling method for module ConversionProducer/'allConversions'
Exception Message:
Refitted track not found in list
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 17-Jan-2022 19:50:02 CET-----------------------
An exception of category 'Vertex' occurred while
   [0] Processing  Event run: 326479 lumi: 7 event: 1579493 stream: 0
   [1] Running path 'dqmoffline_8_step'
   [2] Prefetching for module SMPDQM/'SMPDQM'
   [3] Prefetching for module MuonProducer/'muons'
   [4] Prefetching for module PFProducer/'particleFlowTmp'
   [5] Prefetching for module PFBlockProducer/'particleFlowBlock'
   [6] Prefetching for module PFElecTkProducer/'pfTrackElec'
   [7] Prefetching for module PFConversionProducer/'pfConversions'
   [8] Calling method for module ConversionProducer/'allConversions'
Exception Message:
Refitted track not found in list
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 17-Jan-2022 20:08:46 CET-----------------------
An exception of category 'Vertex' occurred while
   [0] Processing  Event run: 319450 lumi: 76 event: 106007323 stream: 0
   [1] Running path 'dqmoffline_10_step'
   [2] Prefetching for module SMPDQM/'SMPDQM'
   [3] Prefetching for module MuonProducer/'muons'
   [4] Prefetching for module PFProducer/'particleFlowTmp'
   [5] Prefetching for module PFBlockProducer/'particleFlowBlock'
   [6] Prefetching for module PFElecTkProducer/'pfTrackElec'
   [7] Prefetching for module PFConversionProducer/'pfConversions'
   [8] Calling method for module ConversionProducer/'allConversions'
Exception Message:
Refitted track not found in list
----- End Fatal Exception -------------------------------------------------

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0ab80e/21760/summary.html
COMMIT: 4175cf6
CMSSW: CMSSW_12_3_X_2022-01-16-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmsdist/7483/21760/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0ab80e/21760/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0ab80e/21760/git-merge-result

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 43
  • DQMHistoTests: Total histograms compared: 3464734
  • DQMHistoTests: Total failures: 7
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3464704
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 42 files compared)
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 181 log files, 42 edm output root files, 43 DQM output files
  • TriggerResults: no differences found

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0ab80e/21761/summary.html
COMMIT: 4175cf6
CMSSW: CMSSW_12_3_X_2022-01-16-2300/cs8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmsdist/7483/21761/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0ab80e/21761/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0ab80e/21761/git-merge-result

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 65791 differences found in the comparisons
  • DQMHistoTests: Total files compared: 43
  • DQMHistoTests: Total histograms compared: 3464734
  • DQMHistoTests: Total failures: 614026
  • DQMHistoTests: Total nulls: 312
  • DQMHistoTests: Total successes: 2850374
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.342 KiB( 42 files compared)
  • DQMHistoSizes: changed ( 10224.0 ): -0.288 KiB SiStrip/MechanicalView
  • DQMHistoSizes: changed ( 11834.0 ): 2.191 KiB SiStrip/MechanicalView
  • DQMHistoSizes: changed ( 250202.181 ): -0.111 KiB SiStrip/MechanicalView
  • DQMHistoSizes: changed ( 25202.0 ): -0.129 KiB SiStrip/MechanicalView
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • DQMHistoSizes: changed ( 7.3 ): -2.009 KiB SiStrip/MechanicalView
  • Checked 181 log files, 42 edm output root files, 43 DQM output files
  • TriggerResults: found differences in 14 / 42 workflows

@smuzaffar
Copy link
Contributor

+externals
looks good to go in. cms-sw/cmssw#36316 requires this

@SiewYan
Copy link
Contributor

SiewYan commented Jan 24, 2022

thank you @smuzaffar , are those two errors concerning? I can't view those error in the Details.

@smuzaffar
Copy link
Contributor

@SiewYan , no errors for slc7_aarch64_gcc11 are already there in the IBs. So those can be ignored for this PR test

@SiewYan
Copy link
Contributor

SiewYan commented Jan 24, 2022

@smuzaffar , thanks, I will sign this PR.

@SiewYan
Copy link
Contributor

SiewYan commented Jan 24, 2022

+generators

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next IB/CMSSW_12_3_X/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)

@smuzaffar
Copy link
Contributor

merging it as it is needed by cms-sw/cmssw#36316

@smuzaffar smuzaffar merged commit bbd5012 into cms-sw:IB/CMSSW_12_3_X/master Feb 8, 2022
@perrotta
Copy link
Contributor

perrotta commented Feb 8, 2022

merging it as it is needed by cms-sw/cmssw#36316

Thank you @smuzaffar : I was forgetting it, indeed...

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