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 ONNX Runtime to v1.0.0 #5346

Conversation

hqucms
Copy link
Contributor

@hqucms hqucms commented Nov 12, 2019

Based on the v1.0.0 release, plus the modification in #5259.

v1.0.0 is not compatible with the eigen version we have -- given that eigen is a header-only library without any binaries, there is no runtime dependency on eigen, I think it should be fine to let ONNX Runtime use a different version (shipped as a git submodule).

@smuzaffar Could you please create a new branch in https://github.com/cms-externals/onnxruntime/?

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @hqucms (Huilin Qu) for branch IB/CMSSW_11_0_X/master.

@cmsbuild, @smuzaffar, @mrodozov can you please review it and eventually sign? Thanks.
cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
Tested with other pull request(s) cms-sw/cmssw#28112

@cmsbuild
Copy link
Contributor

Pull request #5346 was updated.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 13, 2019

The tests are being triggered in jenkins.
Tested with other pull request(s) cms-sw/cmssw#28112
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3449/console Started: 2019/11/13 06:41

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

%define github_user cms-externals
Source: git+https://github.com/%{github_user}/%{n}.git?obj=%{branch}/%{tag}&export=%{n}-%{realversion}&submodules=1&output=/%{n}-%{realversion}.tgz

BuildRequires: cmake ninja zlib python3
Requires: eigen protobuf
Requires: protobuf
Copy link
Contributor

Choose a reason for hiding this comment

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

@hqucms , yes it is not needed as onnxruntime uses eigen internally i.e. non of the onnxruntime public headers include eigen. So removing it is correct. We need to be careful when onnxruntime public headers starts using eigen. In that case we have to make sure that onnxruntime and cmssw use the same eigen 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.

@smuzaffar Thank you very much for confirming that!

@hqucms
Copy link
Contributor Author

hqucms commented Nov 13, 2019

@smuzaffar I think it would be helpful to have the python package available to CMS users as well, but I have not figured out how to build the python wheel in the CMS environment. Would you mind taking a look, or point me to some examples?

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-63abc4/3449/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 5 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2936360
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2936017
  • DQMHistoTests: Total skipped: 341
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@cmsbuild
Copy link
Contributor

Pull request #5346 was updated.

@smuzaffar
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 14, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3485/console Started: 2019/11/14 17:36

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 14, 2019

The tests are being triggered in jenkins.
Tested with other pull request(s) cms-sw/cmssw#28112
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3489/console Started: 2019/11/14 19:43

@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-87d558/3485/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2936360
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2936017
  • DQMHistoTests: Total skipped: 341
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@smuzaffar
Copy link
Contributor

+externals

@cmsbuild
Copy link
Contributor

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

@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-63abc4/3489/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 5 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2936360
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2936017
  • DQMHistoTests: Total skipped: 341
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@slava77
Copy link
Contributor

slava77 commented Nov 14, 2019

similar to the earlier version of this PR, I think that it can be merged first (not coupled with the CMSSW PR).

@smuzaffar smuzaffar merged commit 395c122 into cms-sw:IB/CMSSW_11_0_X/master Nov 15, 2019
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