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.

Sign up for GitHub

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

Jump to bottom

[14_1_X] Remove tensorflow::setLogging() as thread-unsafe #46106

Merged
merged 1 commit into from
Sep 26, 2024
Merged

[14_1_X] Remove tensorflow::setLogging() as thread-unsafe #46106

merged 1 commit into from
Sep 26, 2024

Conversation

makortel
Copy link
Contributor

PR description:

Backport of #46065

PR validation:

None beyond #46065

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

Backport of #46065 (following request in #46065 (comment))

@makortel
Remove tensorflow::setLogging() as thread-unsafe
34d6223 
The setLogging() calls setenv(), which is not required to be thread
safe, and specifically in glibc leads to a race condition with any
concurrent getenv() calls.
@cmsbuild cmsbuild added this to the CMSSW_14_1_X milestone Sep 23, 2024
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @makortel for CMSSW_14_1_X.

It involves the following packages:

  • DQM/DTMonitorClient (dqm)
  • L1Trigger/L1CaloTrigger (l1, upgrade)
  • L1Trigger/L1THGCal (l1, upgrade)
  • L1Trigger/Phase2L1ParticleFlow (l1, upgrade)
  • PhysicsTools/TensorFlow (ml)
  • RecoEcal/EgammaCoreTools (reconstruction)
  • RecoMuon/TrackerSeedGenerator (reconstruction)
  • RecoTauTag/HLTProducers (hlt)

@Martin-Grunewald, @Moanwar, @aloeliger, @antoniovagnerini, @cmsbuild, @epalencia, @jfernan2, @mandrenguyen, @mmusich, @nothingface0, @rvenditti, @srimanob, @subirsarkar, @syuvivida, @tjavaid, @valsdav, @y19y19 can you please review it and eventually sign? Thanks.
@CeliaFernandez, @Fedespring, @HuguesBrun, @Martin-Grunewald, @Prasant1993, @ReyerBand, @Sam-Harper, @a-kapoor, @abbiendi, @afiqaize, @amarini, @andrea21z, @argiro, @azotz, @battibass, @bellan, @cericeci, @jainshilpi, @jbsauvan, @jhgoh, @lgray, @mbluj, @missirol, @mmusich, @ram1123, @rchatter, @riga, @rociovilar, @sameasy, @silviodonato, @sobhatta, @thomreis, @trocino, @valsdav, @varuns23, @wang0jin this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 23, 2024 •
edited
Loading

cms-bot internal usage

@makortel makortel mentioned this pull request Sep 23, 2024
Merged
@makortel
Copy link
Contributor Author

@cmsbuild, please test with cms-sw/cmsdist#9431

This was referenced Sep 23, 2024
Merged
Closed
@cmsbuild
Copy link
Contributor

+1

Size: This PR adds an extra 52KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b8a7a4/41716/summary.html
COMMIT: 34d6223
CMSSW: CMSSW_14_1_X_2024-09-23-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46106/41716/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 98 lines from the logs
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 44
  • DQMHistoTests: Total histograms compared: 3330894
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3330871
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 43 files compared)
  • Checked 191 log files, 161 edm output root files, 44 DQM output files
  • TriggerResults: no differences found

@jfernan2
Copy link
Contributor

+1

@mmusich
Copy link
Contributor

mmusich commented Sep 24, 2024

+hlt

@valsdav
Copy link
Contributor

valsdav commented Sep 24, 2024

+ml

@aloeliger
Copy link
Contributor

+l1

@Moanwar
Copy link
Contributor

Moanwar commented Sep 24, 2024

+Upgrade

@antoniovagnerini
Copy link

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_14_1_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_14_2_X is complete. This pull request will now be reviewed by the release team before it's merged. @antoniovilela, @rappoccio, @sextonkennedy, @mandrenguyen (and backports should be raised in the release meeting by the corresponding L2)
Notice This PR was tested with additional Pull Request(s), please also merge them if necessary: cms-sw/cmsdist#9431

@antoniovilela
Copy link
Contributor

cms-sw/cmsdist#9431 merged

@antoniovilela
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit aa4e822 into cms-sw:CMSSW_14_1_X Sep 26, 2024
9 checks passed
@makortel makortel deleted the tensorflowRemoveLogging_141x branch September 26, 2024 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
dqm-approved fully-signed hlt-approved l1-approved ml-approved orp-approved reconstruction-approved requires-external tests-approved upgrade-approved
Projects
None yet
Milestone
CMSSW_14_1_X
Development

Successfully merging this pull request may close these issues.
9 participants
@makortel @cmsbuild @jfernan2 @mmusich @valsdav @aloeliger @Moanwar @antoniovagnerini @antoniovilela