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

Consumes to CSCDigitizer #782

Merged
merged 8 commits into from Sep 12, 2013
Merged

Conversation

ptcox
Copy link
Contributor

@ptcox ptcox commented Sep 11, 2013

Adds consumes interface to SimMuon/CSCDigitizer - CSCDigiProducer and CSCDigiDump. Updates config file for CSCDigiProducer to use an InputTag to specify simhit input.

@cmsbuild
Copy link
Contributor

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

Consumes to CSCDigitizer

It involves the following packages:

SimMuon/CSCDigitizer

@civanch, @mdhildred, @giamman 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.

@civanch
Copy link
Contributor

civanch commented Sep 12, 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
Copy link
Contributor

ktf commented Sep 12, 2013

It compiles fine, but breaks a couple of workflows:

https://cmssdt.cern.ch/jenkins/job/Pull-Request-Integration/ARCHITECTURE=slc5_amd64_gcc481/466/console

@ptcox
Copy link
Contributor Author

ptcox commented Sep 12, 2013

Hmm... I'm sorry. I don't know how to interpret this output. What is broken exactly?

(I see some LogError messages from a test executable, and although they're a surprise to me, they shouldn't crash the test. In any case I have no idea what to do about those LogErrors. It looks like ParticleDataTable must be broken (?), which is something outside my realm of expertise.)

I also see

...

in: /build/cmsbuild/jenkins-workarea/workspace/Pull-Request-Integration/ARCHITECTURE/slc5_amd64_gcc481/runTheMatrix-results going to execute cd 8.0_BeamHalo+BeamHalo+DIGICOS+RECOCOS+ALCABH+HARVESTCOS

cmsDriver.py BeamHalo_cfi.py --conditions auto:startup -s GEN,SIM -n 10 --eventcontent RAWSIM --relval 9000,100 --scenario cosmics --datatier GEN-SIM --fileout file:step1.root > step1_BeamHalo+BeamHalo+DIGICOS+RECOCOS+ALCABH+HARVESTCOS.log 2>&1

ERROR executing cd 5.1_TTbar+TTbarFS+HARVESTFS; cmsDriver.py TTbar_Tauola_8TeV_cfi --conditions auto:startup --fast -n 10 --eventcontent FEVTDEBUGHLT,DQM --relval 100000,1000 -s GEN,SIM,RECO,EI,HLT:@RelVal,VALIDATION --datatier GEN-SIM-DIGI-RECO,DQM --fileout file:step1.root > step1_TTbar+TTbarFS+HARVESTFS.log 2>&1; ret= 256

...

which again means nothing to me, and cannot be due to my addition of the consumes interface.

Regards, Tim

On Sep 12, 2013, at 14:06 , Giulio Eulisse notifications@github.com wrote:

It compiles fine, but breaks a couple of workflows:

https://cmssdt.cern.ch/jenkins/job/Pull-Request-Integration/ARCHITECTURE=slc5_amd64_gcc481/466/console


Reply to this email directly or view it on GitHub.

@ktf
Copy link
Contributor

ktf commented Sep 12, 2013

Are you sure about this? You changed the digitiser and if it fails in the step 1 of a couple of simulation workflows. In particular it seems to die with the one using the new mixing module, 401.0, and a fastsim one 5.1.

If I do:

scram project CMSSW_7_0_X_2013-09-12-0200 
cd CMSSW_7_0_X_2013-09-12-0200
cmsenv
runTheMatrix.py -l 5.1

it works without your changes but it does not if I add them via:

git cms-merge-topic 782
scram b -j 20

The actual error is:

TypeError: InputCollection does not already exist, so it can only be set to a CMS python configuration type

which is indeed in one of your changes.

@giamman
Copy link
Contributor

giamman commented Sep 12, 2013

I'd like to have a look and help to fix that, but I am not able to access the jenkins page (hadn't this been fixed already? please give access to my agiamman account...)

@ptcox
Copy link
Contributor Author

ptcox commented Sep 12, 2013

Ah ha, I see the problem. This is a result of changing to an InputTag to satisfy the consumes interface.

My change was

  • mixLabel = cms.string('mix'),
  • InputCollection = cms.string('g4SimHitsMuonCSCHits'),
  • simHitTag = cms.InputTag("mix", "g4SimHitsMuonCSCHits"),

But other configs (e.g. Fast Sim) replace 'mixLabel' and 'InputCollection', which I have cleverly removed. This will have to be sorted out with the Sim conveners before the consumes interface goes in.

Thanks,

Tim

On Sep 12, 2013, at 14:54 , Giulio Eulisse notifications@github.com wrote:

Are you sure about this? You changed the digitiser and if it fails in the step 1 of a couple of simulation workflows. In particular it seems to die with the one using the new mixing module, 401.0, and a fastsim one 5.1.

If I do:

scram project CMSSW_7_0_X_2013-09-12-0200
cd CMSSW_7_0_X_2013-09-12-0200
cmsenv
runTheMatrix.py -l 5.1

it works without your changes but it does not if I add them via:

git cms-merge-topic 782
scram b -j 20

The actual error is:

TypeError: InputCollection does not already exist, so it can only be set to a CMS python configuration type

which is indeed in one of your changes.


Reply to this email directly or view it on GitHub.

@ktf
Copy link
Contributor

ktf commented Sep 12, 2013

@mojedasa can you please make sure agiamman can access the above mentioned url?

@nclopezo
Copy link
Contributor

Hi All,

I noticed that Jenkins was not saving the artifacts when something failed. I changed the configuration so It saves them whether it fails or not, and I launched again a test for this Pull Request. You can see them here:

https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/473

@giamman
Copy link
Contributor

giamman commented Sep 12, 2013

@ptcox Yes, that is obviously the reason. So the solution is trivial (and I will work on that). It will fix also workflow 401, which is a FastSim workflow too.
But I have a problem: I will have to edit a file that is heavily modified already in PR 806 which I submitted a few minutes ago. So, how should I proceed? Shall I commit a modification on top of the base release, and then somebody will merge it with PR 806 if the latter is approved for merging, or shall I wait for PR 806 to be merged, and then merge this PR with PR 806 plus the fix that we are talking about?

@nclopezo thanks, now I can access it.

@ptcox
Copy link
Contributor Author

ptcox commented Sep 12, 2013

I guess just building an InputTag for the consumes from the mixLabel & InputCollection? That seems the simplest thing to do in that it won't break other configs. I can do it.

Regards, Tim

On Sep 12, 2013, at 15:19 , Andrea Giammanco notifications@github.com wrote:

@ptcox Yes, that is obviously the reason. So the solution is trivial (and I will work on that). It will fix also workflow 401, which is a FastSim workflow too.
But I have a problem: I will have to edit a file that is heavily modified already in PR 806 which I submitted a few minutes ago. So, how should I proceed? Shall I commit a modification on top of the base release, and then somebody will merge it with PR 806 if the latter is approved for merging, or shall I wait for PR 806 to be merged, and then merge this PR with PR 806 plus the fix that we are talking about?

@nclopezo thanks, now I can access it.


Reply to this email directly or view it on GitHub.

@giamman
Copy link
Contributor

giamman commented Sep 12, 2013

Thanks, that's kind. In the meantime I was trying to start having a look, but stumbled upon something that I don't understand:

[lxplus408] /afs/cern.ch/work/g/giamman/fastsim/csc/CMSSW_7_0_X_2013-09-12-0200/src $ git cms-merge-topic 782
No release tags specified, using default CMSSW_7_0_X_2013-09-12-0200.
You are on branch CMSSW_7_0_X
Checking out FWCore/Version in tag CMSSW_7_0_X_2013-09-12-0200.
fatal: destination path '/afs/cern.ch/work/g/giamman/fastsim/csc/CMSSW_7_0_X_2013-09-12-0200/src' already exists and is not an empty directory.

Can anybody shed some light about this error message?

@ktf
Copy link
Contributor

ktf commented Sep 12, 2013

remove the relval output, I guess...

@giamman
Copy link
Contributor

giamman commented Sep 12, 2013

ah thanks, it worked. I didn't know that cms-merge-topic was intolerant to the presence of other files around.

@ktf
Copy link
Contributor

ktf commented Sep 12, 2013

It's actually git that complains, but yeah, it wants an empty / non existing directory.

@ktf
Copy link
Contributor

ktf commented Sep 12, 2013

I actually just merged #806, so you can just do a separate pull request which fixes it.

@ptcox
Copy link
Contributor Author

ptcox commented Sep 12, 2013

OK, thanks. Does that mean I need a new branch name rather than updating the existing one?
(I have the fix but I am struggling to test it. It seems my test config has been broken :( Working on it now.)

Tim

On Sep 12, 2013, at 16:22 , Giulio Eulisse notifications@github.com wrote:

I actually just merged 806, so you can just do a separate pull request which fixes it.


Reply to this email directly or view it on GitHub.

@giamman
Copy link
Contributor

giamman commented Sep 12, 2013

The trivial fix that I was proposing was to edit this line in FastSimulation/Configuration/python/FamosSequences_cff.py

simMuonCSCDigis.InputCollection = 'MuonSimHitsMuonCSCHits'

into

simMuonCSCDigis.simHitTag = cms.InputTag("mix", "MuonSimHitsMuonCSCHits")

I tested that this works in my local area. I suppose that the easiest is that you commit this into your branch. (Maybe Giulio can comment.)

@ptcox
Copy link
Contributor Author

ptcox commented Sep 12, 2013

Hi Andrea,

THat doesn't seem so good to me, because other systems also have the existing mixLabel & InputCollection. It seems minimally intrusive for now to KEEP those two labels, and just create the InputTag from them, rather than have to change config files all over the place.

Regards, Tim

On Sep 12, 2013, at 16:48 , Andrea Giammanco notifications@github.com wrote:

The trivial fix that I was proposing was to edit this line in FastSimulation/Configuration/python/FamosSequences_cff.py

simMuonCSCDigis.InputCollection = 'MuonSimHitsMuonCSCHits'

into

simMuonCSCDigis.simHitTag = cms.InputTag("mix", "MuonSimHitsMuonCSCHits")

I tested that this works in my local area. I suppose that the easiest is that you commit this into your branch. (Maybe Giulio can comment.)


Reply to this email directly or view it on GitHub.

@giamman
Copy link
Contributor

giamman commented Sep 12, 2013

OK, I trust your opinion as expert. I will have a look at the new pull request when this issue will have been fixed, then.

@cmsbuild
Copy link
Contributor

Pull request #782 was updated. @civanch, @mdhildreth, @giamman can you please check and sign again.

@ptcox
Copy link
Contributor Author

ptcox commented Sep 12, 2013

(I find it difficult to know who I am sending email to when the address appears as a cryptic git-hub thing, so I am adding Giulio and Andrea explicitly in cc)

Not exactly an expert, but it just seems simpler not to mess with multiple config files.
I've reverted to the original mixLabel & InputCollection, and managed to check it runs for CSC like that.
So now it shouldn't break other config files.

I just repushed the 'consumes_to_cscdigitizer' branch.

Regards, Tim

On Sep 12, 2013, at 16:53 , Andrea Giammanco notifications@github.com wrote:

OK, I trust your opinion as expert. I will have a look at the new pull request when this issue will have been fixed, then.


Reply to this email directly or view it on GitHub.

@giamman
Copy link
Contributor

giamman commented Sep 12, 2013

At a first glance it now seems innocuous to FastSim. Did you already run the command runTheMatrix.py -l 5.1,401 ? (Eventually it will be done by the release managers in any case.)

@ptcox
Copy link
Contributor Author

ptcox commented Sep 12, 2013

Hi Andrea,

No, I didn't run the runTheMatrix thing. I don't know anything about it.

Tim


From: Andrea Giammanco [notifications@github.com]
Sent: 12 September 2013 17:30
To: cms-sw/cmssw
Cc: Tim Cox
Subject: Re: [cmssw] Consumes to CSCDigitizer (#782)

At a first glance it now seems innocuous to FastSim. Did you already run the command runTheMatrix.py -l 5.1,401 ? (Eventually it will be done by the release managers in any case.)


Reply to this email directly or view it on GitHubhttps://github.com//pull/782#issuecomment-24330457.

@giamman
Copy link
Contributor

giamman commented Sep 12, 2013

Well, it's as simple as typing

runTheMatrix.py -l 5.1,401

Just let me know. (I could do it myself but not before tomorrow, sorry)

Andrea


Andrea Giammanco
Phone (CERN): +41 22 76 71567
Mobiles: +41 76 2323672 (CH), +32 497 135121 (BE), +39 349 5552471 (IT)


From: ptcox [notifications@github.com]
Sent: 12 September 2013 19:18
To: cms-sw/cmssw
Cc: Andrea Giammanco
Subject: Re: [cmssw] Consumes to CSCDigitizer (#782)

Hi Andrea,

No, I didn't run the runTheMatrix thing. I don't know anything about it.

Tim


From: Andrea Giammanco [notifications@github.com]
Sent: 12 September 2013 17:30
To: cms-sw/cmssw
Cc: Tim Cox
Subject: Re: [cmssw] Consumes to CSCDigitizer (#782)

At a first glance it now seems innocuous to FastSim. Did you already run the command runTheMatrix.py -l 5.1,401 ? (Eventually it will be done by the release managers in any case.)


Reply to this email directly or view it on GitHubhttps://github.com//pull/782#issuecomment-24330457.


Reply to this email directly or view it on GitHubhttps://github.com//pull/782#issuecomment-24339043.

@ptcox
Copy link
Contributor Author

ptcox commented Sep 12, 2013

No urgency from my side. I am only doing this because it is demanded of me :)
Tomorrow seems pretty good!

Tim


From: Andrea Giammanco [notifications@github.com]
Sent: 12 September 2013 19:39
To: cms-sw/cmssw
Cc: Tim Cox
Subject: Re: [cmssw] Consumes to CSCDigitizer (#782)

Well, it's as simple as typing

runTheMatrix.py -l 5.1,401

Just let me know. (I could do it myself but not before tomorrow, sorry)

Andrea


Andrea Giammanco
Phone (CERN): +41 22 76 71567
Mobiles: +41 76 2323672 (CH), +32 497 135121 (BE), +39 349 5552471 (IT)


From: ptcox [notifications@github.com]
Sent: 12 September 2013 19:18
To: cms-sw/cmssw
Cc: Andrea Giammanco
Subject: Re: [cmssw] Consumes to CSCDigitizer (#782)

Hi Andrea,

No, I didn't run the runTheMatrix thing. I don't know anything about it.

Tim


From: Andrea Giammanco [notifications@github.com]
Sent: 12 September 2013 17:30
To: cms-sw/cmssw
Cc: Tim Cox
Subject: Re: [cmssw] Consumes to CSCDigitizer (#782)

At a first glance it now seems innocuous to FastSim. Did you already run the command runTheMatrix.py -l 5.1,401 ? (Eventually it will be done by the release managers in any case.)


Reply to this email directly or view it on GitHubhttps://github.com//pull/782#issuecomment-24330457.


Reply to this email directly or view it on GitHubhttps://github.com//pull/782#issuecomment-24339043.


Reply to this email directly or view it on GitHubhttps://github.com//pull/782#issuecomment-24340601.

@civanch
Copy link
Contributor

civanch commented Sep 12, 2013

+1
I have done runTheMatrix.py -s and no failers. This time I did not make mistakes while checking.
Vladimir

@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
Copy link
Contributor

ktf commented Sep 12, 2013

This also passes:

runTheMatrix.py -l 5.1,401.0

Which was failing before. Merging.

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

6 participants