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

Update dqmservices for protobuf changes #22568

Merged
merged 2 commits into from Apr 3, 2018

Conversation

mrodozov
Copy link
Contributor

@mrodozov mrodozov commented Mar 9, 2018

No description provided.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 9, 2018

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 9, 2018

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22568/3894

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22568/3894/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22568/3894/git-diff.patch | patch -p1

You can run scram build code-checks to apply code checks directly

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 10, 2018

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22568/3895

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22568/3895/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22568/3895/git-diff.patch | patch -p1

You can run scram build code-checks to apply code checks directly

@mrodozov
Copy link
Contributor Author

@cmsbuild please test with cms-sw/cmsdist#3835

@mrodozov
Copy link
Contributor Author

code checks misinterprets a lambda function definition with a variable definition and adds semicolon at the end of the square brackets, which leads to nonsense that doesn't compile could you please edit the label to test this ?

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mrodozov (Mircho Rodozov) for master.

It involves the following packages:

DQMServices/Core

@vazzolini, @kmaeshima, @dmitrijus, @cmsbuild, @jfernan2, @vanbesien can you please review it and eventually sign? Thanks.
@barvic 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

@cms-sw cms-sw deleted a comment from cmsbuild Mar 10, 2018
@smuzaffar
Copy link
Contributor

please test with cms-sw/cmsdist#3835

@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-22568/26788/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 29
  • DQMHistoTests: Total histograms compared: 2477919
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2477742
  • DQMHistoTests: Total skipped: 176
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.8299999999 KiB( 23 files compared)
  • Checked 118 log files, 9 edm output root files, 29 DQM output files

@smuzaffar
Copy link
Contributor

assign reconstruction
@slava77 , any objection on updating tensorflow to 1.6.0. We wanted to update protobuf and noticed that we can update tensorflow too which is using the newer protobuf.

@cmsbuild
Copy link
Contributor

New categories assigned: reconstruction

@slava77,@perrotta you have been requested to review this Pull request/Issue and eventually sign? Thanks

@slava77
Copy link
Contributor

slava77 commented Mar 12, 2018

New categories assigned: reconstruction

perhaps we should just add a possibility for assignment of the externals.
Code changes in this PR itself have nothing to do with reco.

@slava77
Copy link
Contributor

slava77 commented Mar 12, 2018

Looking at tensorflow/tensorflow@v1.5.0...v1.6.0
I see that
tensorflow/core/common_runtime/direct_session.cc and tensorflow/core/common_runtime/direct_session.h have changed in some non-trivial way.

I see that jenkins tests passed. So, changes are probably not essential for the job integrity.
Still, a consistent update of the NTSession and TBBSession would be nice.

@dmitrijus
Copy link
Contributor

+1

This is fine for DQM, however, we will need to bump fastHadd version once this is merged (and goes into production).

@slava77
Copy link
Contributor

slava77 commented Mar 13, 2018

@riga
please check the notes in #22568 (comment)
Are you available to update the CMSSW session implementations?

@smuzaffar @mrodozov
how should we handle the update to NTSession and TBBSession (especially if it's considered somewhat essential).
The API did not change and the update in NTSession and TBBSession can probably be done out of sync, but I'd like to hear from @riga that this way (update to upstream implementation later) we will not trigger some unwanted issues.

@riga
Copy link
Contributor

riga commented Mar 13, 2018

@slava77 Yes, I'm available for updating the TF session code. I will start looking into the diffs tomorrow morning.

@slava77
Copy link
Contributor

slava77 commented Mar 14, 2018

@riga
Thank you for providing the update quickly.

Was it clear that there were any operational issues that require TF 1.6 update to be in syn with #22603 ?
I wanted to reduce integration inter-dependencies.

@riga
Copy link
Contributor

riga commented Mar 14, 2018

@slava77 I couldn't test locally, but the interface changes should be backwards-compatible with TF 1.5 (most of the changes just do some restructuring of internal members).

@slava77
Copy link
Contributor

slava77 commented Mar 14, 2018

unassign reconstruction
[in case it works]
we'll follow up in #22603

@cmsbuild
Copy link
Contributor

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 will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@smuzaffar smuzaffar modified the milestones: CMSSW_10_1_X, CMSSW_10_2_X Mar 29, 2018
@fabiocos
Copy link
Contributor

fabiocos commented Apr 3, 2018

+1

@cmsbuild cmsbuild merged commit f3c02a0 into cms-sw:master Apr 3, 2018
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

7 participants