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

Migration to consumes -- EGamma DQM #812

Merged
merged 4 commits into from Sep 27, 2013

Conversation

nkellams
Copy link
Contributor

updating photon DQM to use getByToken and consume as outlined in the DQM meeting

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @nkellams for CMSSW_7_0_X.

Dqm egamma consumes patch

It involves the following packages:

DQMOffline/EGamma

@danduggan, @rovere, @deguio can you please review it and eventually sign? Thanks.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@ktf you are the release manager for this.


barrelRecHitProducer = cms.string('reducedEcalRecHitsEB'),
barrelRecHitCollection = cms.string(''),
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 remove this parameter you brake the sequence: DQMOfflineHeavyIonsPrePOG
defined in
DQMOffline/Configuration/python/DQMOfflineHeavyIons_cff.py

you have to either put back the parameter or you adapt the sequence.

@deguio
Copy link
Contributor

deguio commented Sep 13, 2013

@nkellams you can update the code and push the changes to this pull request.
I leave it pending for now.

thanks,
F.

@deguio
Copy link
Contributor

deguio commented Sep 17, 2013

@nkellams
any news on this?

thanks,
F.

@rovere

@nkellams
Copy link
Contributor Author

I put the parameters back in so that the DQMOfflineHeavyIons_cff.py would not complain. However, now I get warnings about those parameters being depricated. Is it acceptable to have these warnings?

@cmsbuild
Copy link
Contributor

Pull request #812 was updated. @danduggan, @nclopezo, @rovere, @deguio can you please check and sign again.

@ktf
Copy link
Contributor

ktf commented Sep 19, 2013

Please do not introduce new warnings.

@nkellams
Copy link
Contributor Author

Hi All,
I'm rather confused about this. I had to change these parameters from strings to inputTags to comply with the need to use getByToken (which does not take strings) instead of getByLabel (which did take strings) as discussed in the DQM meetings. Doing so broke the HeavyIon file mentioned above. I put back these parameters, even though they are unused in the EGamma software now so that the HeavyIon file wouldn't complain. This gives me warnings to use inputTags instead of strings, which is why I originally removed them. Given that I am not a developer of the HeavyIon DQM software I don't see how to resolve this issue without the warnings.

@deguio
Copy link
Contributor

deguio commented Sep 19, 2013

hello @nkellams
have a look at:
https://cmssdt.cern.ch/SDT/lxr/source/DQMServices/Components/plugins/DQMLumiMonitor.cc?v=CMSSW_7_0_0_pre4#031
it seems to me that it is still possible to use strings with the getByToken. I didn't get any warning when I did that but I leave @ktf comment about what is better to use.

As you can imagine we cannot integrate changes that brake the existing code. that is the reason why we ask the developers to test the code before submitting a pull request.

I see 2 solutions:

  • if the use of the strings is blessed by @ktf then we can forget about the inputTags
  • we use the inputTags and we can modify the HI code accordingly making sure that everything is consistent. this is possible with GIT. you don't have to be a "HI developer" to do so.

Of course I can help if needed.

thanks,
F.

@rovere @dandaggan

@ktf
Copy link
Contributor

ktf commented Sep 20, 2013

I would go for the second solution and do a proper cleanup.

@cmsbuild
Copy link
Contributor

Pull request #812 was updated. @smuzaffar, @nclopezo, @danduggan, @rovere, @deguio, @eliasron can you please check and sign again.

@deguio
Copy link
Contributor

deguio commented Sep 27, 2013

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next IBs unless changes or unless it breaks tests. @ktf can you please take care of it?

ktf added a commit that referenced this pull request Sep 27, 2013
@ktf ktf merged commit 9c08c71 into cms-sw:CMSSW_7_0_X Sep 27, 2013
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

5 participants