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 of LumiList.py so to allow the merge of JSON files #44201

Merged
merged 1 commit into from
Mar 1, 2024
Merged

Fix of LumiList.py so to allow the merge of JSON files #44201

merged 1 commit into from
Mar 1, 2024

Conversation

syuvivida
Copy link
Contributor

PR description:

While merging the JSON files with mergeJSON.py, we ran into this error:
File "/cvmfs/cms.cern.ch/el9_amd64_gcc12/cms/cmssw/CMSSW_14_0_0/bin/el9_amd64_gcc12/mergeJSON.py", line 44, in <module> finalList = finalList | localList File "/cvmfs/cms.cern.ch/el9_amd64_gcc12/cms/cmssw/CMSSW_14_0_0/src/FWCore/PythonUtilities/python/LumiList.py", line 186, in __or__ runs = set(aruns + bruns) TypeError: unsupported operand type(s) for +: 'dict_keys' and 'dict_keys'

as in Python 3.x, dict.keys doesn't return a list, but instead a view object, dict_keys. Therefore, this PR converted all the runs to a list to solve this error.

PR validation:

The PR has been validated by copying mergeJSON.py to a local area at lxplus and point to the local LumiList.py, with this PR, two JSONs could be merged without errors. Additionally, we also use this version of LumiList.py to launch cron jobs of DCS-only JSON files at lxplus in the central DQM-DC team. The cron jobs patch the new runs with good siStrip/pixel DCS status every day on top of the previous runs (using mergeJSON.py). These cron jobs have been launched 7 days ago and produced results without error.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 27, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44201/39183

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @syuvivida for master.

It involves the following packages:

  • FWCore/PythonUtilities (core)

@smuzaffar, @Dr15Jones, @makortel, @cmsbuild can you please review it and eventually sign? Thanks.
@makortel, @wddgit, @missirol this is something you requested to watch as well.
@antoniovilela, @sextonkennedy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

Copy link
Contributor

@makortel makortel left a comment

Choose a reason for hiding this comment

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

Could you change back the cases where the generator is sufficient (because it is more efficient to loop over than creating an intermediate list)?

@@ -73,12 +73,12 @@ def __init__(self, filename = None, lumis = None, runsAndLumis = None, runs = No
if isinstance(runsAndLumis, list):
queued = {}
for runLumiList in runsAndLumis:
for run, lumis in runLumiList.items():
for run, lumis in list(runLumiList.items()):
Copy link
Contributor

Choose a reason for hiding this comment

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

Here the earlier use of the items() generator would work, and would be more efficient

Suggested change
for run, lumis in list(runLumiList.items()):
for run, lumis in runLumiList.items():

queued.setdefault(run, []).extend(lumis)
runsAndLumis = queued

if runsAndLumis:
for run in runsAndLumis.keys():
for run in list(runsAndLumis.keys()):
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be better with the generator as well

Suggested change
for run in list(runsAndLumis.keys()):
for run in runsAndLumis.keys():

@@ -100,14 +100,14 @@ def __init__(self, filename = None, lumis = None, runsAndLumis = None, runs = No
self.compactList[runString] = [[1, 0xFFFFFFF]]

if compactList:
for run in compactList.keys():
for run in list(compactList.keys()):
Copy link
Contributor

Choose a reason for hiding this comment

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

This too

Suggested change
for run in list(compactList.keys()):
for run in compactList.keys():

runString = str(run)
if compactList[run]:
self.compactList[runString] = compactList[run]

# Compact each run and make it unique

for run in self.compactList.keys():
for run in list(self.compactList.keys()):
Copy link
Contributor

Choose a reason for hiding this comment

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

And here

Suggested change
for run in list(self.compactList.keys()):
for run in self.compactList.keys():

@@ -336,7 +336,7 @@ def selectRuns (self, runList):
Selects only runs from runList in collection
'''
runsToDelete = []
for run in self.compactList.keys():
for run in list(self.compactList.keys()):
Copy link
Contributor

Choose a reason for hiding this comment

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

And here

Suggested change
for run in list(self.compactList.keys()):
for run in self.compactList.keys():

@syuvivida
Copy link
Contributor Author

Hi @makortel
The latest commit has reverted all the previous changes when looping over runs back to the original LumiList.py. This commit has also been checked locally with mergeJSON.py and cron-job of DQM DCS-only JSON production.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44201/39268

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

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

@makortel
Copy link
Contributor

Thanks! Could you squash the three commits into one in order to have simpler history?

@syuvivida
Copy link
Contributor Author

Thanks! Could you squash the three commits into one in order to have simpler history?

Sorry does this mean I shall close this PR and create a new one?

@makortel
Copy link
Contributor

Thanks! Could you squash the three commits into one in order to have simpler history?

Sorry does this mean I shall close this PR and create a new one?

Opening a new PR is not necessary. You can use

to collapse the commits to one, and then force-push (git push -f ...) to the same branch, and this PR will be updated.

I'd suggest to make a "backup" of your branch before doing anything else, e.g. git branch master_backup master just in case.

@syuvivida
Copy link
Contributor Author

Thanks! Could you squash the three commits into one in order to have simpler history?

Sorry does this mean I shall close this PR and create a new one?

Opening a new PR is not necessary. You can use

* `git cms-squash-topic --current` as instructed in https://cms-sw.github.io/tutorial-resolve-conflicts.html

* Follow https://cms-sw.github.io/faq.html#how-do-i-collapse-multiple-commits-into-one

* Use interactive rebase `git rebase -i $CMSSW_VERSION`
  
  * or if done outside of a CMSSE developer area, replace `$CMSSW_VERSION` with the tag/commit id of the base of your `master` branch

to collapse the commits to one, and then force-push (git push -f ...) to the same branch, and this PR will be updated.

I'd suggest to make a "backup" of your branch before doing anything else, e.g. git branch master_backup master just in case.

Thanks for the tips! Now the 3 commits are merged into one.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44201/39288

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

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

@makortel
Copy link
Contributor

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9f33c7/37823/summary.html
COMMIT: 8c6369e
CMSSW: CMSSW_14_1_X_2024-02-29-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/44201/37823/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

@makortel
Copy link
Contributor

@makortel
Copy link
Contributor

+core

@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. @rappoccio, @sextonkennedy, @antoniovilela (and backports should be raised in the release meeting by the corresponding L2)

@antoniovilela
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit b5ad441 into cms-sw:master Mar 1, 2024
11 checks passed
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.

4 participants