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

Python3 fixes to condb scripts #28157

Merged
merged 3 commits into from Oct 15, 2019
Merged

Python3 fixes to condb scripts #28157

merged 3 commits into from Oct 15, 2019

Conversation

davidlange6
Copy link
Contributor

Fixes to python scripts not ending in .py. Applied some futurize and autopep8 fixers and added @mrodozov changes as well (which I didn't yet test on python2 but look safe)

PR validation:

failure rate of python3 unit tests reduces

@davidlange6
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28157/12214

  • This PR adds an extra 48KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 11, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2890/console Started: 2019/10/11 11:12

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @davidlange6 (David Lange) for master.

It involves the following packages:

CondCore/CondDB
CondCore/Utilities

@ggovi, @cmsbuild can you please review it and eventually sign? Thanks.
@mmusich 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

@cmsbuild
Copy link
Contributor

-1

Tested at: af7f58b

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-a32a97/2890/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a32a97/2890/git-merge-result

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a32a97/2890/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests

  • Unit Tests:

I found errors in the following unit tests:

---> test test_PrepareInputDb had ERRORS
---> test test_MpsWorkFlow had ERRORS

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-a32a97/2890/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a32a97/2890/git-merge-result

@cmsbuild
Copy link
Contributor

Comparison job queued.

@@ -485,7 +485,7 @@ def _getCMSFrontierConnectionString(database):
def _getCMSSQLAlchemyConnectionString(technology,service,schema_name):
if technology == 'frontier':
import urllib
return '%s://@%s/%s' % ('oracle+frontier', urllib.quote_plus(_getCMSFrontierConnectionString(service)), schema_name )
return '%s://@%s/%s' % ('oracle+frontier', urllib.parse.quote_plus(_getCMSFrontierConnectionString(service)), schema_name )
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @mrodozov - i gather that this change is not python2 compatible.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2961064
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2960722
  • 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

-1

Tested at: e332b30

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a32a97/2922/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests

  • Unit Tests:

I found errors in the following unit tests:

---> test test_MpsWorkFlow had ERRORS

@cmsbuild
Copy link
Contributor

Comparison job queued.

@davidlange6
Copy link
Contributor Author

davidlange6 commented Oct 14, 2019 via email

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2961064
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2960721
  • 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

@fabiocos
Copy link
Contributor

@davidlange6 humm, that should have been fixed already by #28150 by @smuzaffar , but apparently there is another issue, in any case it does not look related to this PR.

@davidlange6
Copy link
Contributor Author

davidlange6 commented Oct 15, 2019 via email

@fabiocos
Copy link
Contributor

@davidlange6 ok, I am just not sure about the difference added by @mrodozov wrt your version, discussed in #28161 (comment)
I can anyway take care of rebasing his own version

@fabiocos
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor

@ggovi all this should be technical as discussed yesterday, please have a look anyway

@fabiocos
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit e497640 into cms-sw:master Oct 15, 2019
@ggovi
Copy link
Contributor

ggovi commented Oct 15, 2019

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing).

@smuzaffar
Copy link
Contributor

@fabiocos , we need to revert this. This is breaking many unit tests

@davidlange6
Copy link
Contributor Author

davidlange6 commented Oct 16, 2019 via email

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