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

Fix a.clone(b=dict(c=None)) #22413

Merged
merged 2 commits into from Mar 5, 2018
Merged

Fix a.clone(b=dict(c=None)) #22413

merged 2 commits into from Mar 5, 2018

Conversation

makortel
Copy link
Contributor

@makortel makortel commented Mar 2, 2018

I noticed that

a = cms.PSet(b = cms.PSet(c = ...))
d = a.clone(b=dict(c=None))

did not remove d.b.c. This PR adds a unit test for this case and attempts to fix it.

Tested in CMSSW_10_1_X_2018-02-27-2300, no changes expected (I assume we have not hit this bug before).

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2018

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2018

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

FWCore/ParameterSet

@cmsbuild, @smuzaffar, @Dr15Jones can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @wddgit 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

@makortel
Copy link
Contributor Author

makortel commented Mar 2, 2018

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/26428/console Started: 2018/03/02 09:52

@makortel
Copy link
Contributor Author

makortel commented Mar 2, 2018

type bugfix

_modifyParametersFromDict(p,
value,errorRaiser,
("%s.%s" if type(key)==str else "%s[%s]")%(keyDepth,key))
for k,v in p.iteritems():
setattr(pset,k,v)
try:
oldkeys.remove(k)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you switch to oldkeys.discard(k) you do not have to use the try...except

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, much cleaner with that.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2018

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2018

Pull request #22413 was updated. @cmsbuild, @smuzaffar, @Dr15Jones can you please check and sign again.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2018

-1

Tested at: 0ddfde6

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

I found follow errors while testing this PR

Failed tests: UnitTests AddOn

  • Unit Tests:

I found errors in the following unit tests:

---> test testPythonParameterSet had ERRORS

  • AddOn:

I found errors in the following addon tests:

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2018

Comparison job queued.

@Dr15Jones
Copy link
Contributor

+1
the unit test failiure is unrelated to this pull request.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/26448/console Started: 2018/03/02 15:46

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2018

This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. 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 commented Mar 2, 2018

-1

Tested at: 5d0988a

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

I found follow errors while testing this PR

Failed tests: UnitTests

  • Unit Tests:

I found errors in the following unit tests:

---> test testPythonParameterSet had ERRORS

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2018

Comparison job queued.

@Dr15Jones
Copy link
Contributor

The unit test failure is unrelated to this pull request.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2018

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-22413/26448/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: 2479021
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2478844
  • DQMHistoTests: Total skipped: 176
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 1.36000000009 KiB( 23 files compared)
  • Checked 118 log files, 9 edm output root files, 29 DQM output files

@fabiocos
Copy link
Contributor

fabiocos commented Mar 4, 2018

+1

@Dr15Jones
Copy link
Contributor

@fabiocos this didn't get merged for some reason.

@makortel
Copy link
Contributor Author

makortel commented Mar 5, 2018

@fabiocos this didn't get merged for some reason.

Maybe because of the (unrelated) unit test failure?

@fabiocos
Copy link
Contributor

fabiocos commented Mar 5, 2018

@makortel @Dr15Jones yes, I should force it

@fabiocos
Copy link
Contributor

fabiocos commented Mar 5, 2018

merge

@cmsbuild cmsbuild merged commit 997d1ea into cms-sw:master Mar 5, 2018
@makortel makortel deleted the fixNoneInDict branch March 5, 2018 14:41
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