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

ALCAHARVEST:SiPixelAli is broken #31381

Closed
mmusich opened this issue Sep 7, 2020 · 16 comments
Closed

ALCAHARVEST:SiPixelAli is broken #31381

mmusich opened this issue Sep 7, 2020 · 16 comments

Comments

@mmusich
Copy link
Contributor

mmusich commented Sep 7, 2020

Since CMSSW_11_2_0_pre4 the following command:

cmsDriver.py pedeStep --data --conditions auto:run2_data --era Run2_2018 --scenario pp -s ALCAHARVEST:SiPixelAli --filein /store/group/alca_trackeralign/debug/PromptCalibProdSiPixelAli.root

returns:

ALCAHARVEST:SiPixelAli
magnetic field option forced to: AutoFromDBCurrent
entry /store/group/alca_trackeralign/debug/PromptCalibProdSiPixelAli.root
Step: ALCAHARVEST Spec: ['SiPixelAli']
Starting  cmsRun  pedeStep_ALCAHARVEST.py
07-Sep-2020 12:50:56 CEST  Initiating request to open file root://eoscms.cern.ch//eos/cms/store/group/alca_trackeralign/debug/PromptCalibProdSiPixelAli.root
%MSG-w XrdAdaptor:  file_open 07-Sep-2020 12:51:03 CEST pre-events
Data is served from cern.ch instead of original site eoscms
%MSG
07-Sep-2020 12:51:03 CEST  Successfully opened file root://eoscms.cern.ch//eos/cms/store/group/alca_trackeralign/debug/PromptCalibProdSiPixelAli.root
07-Sep-2020 12:51:32 CEST  Closed file root://eoscms.cern.ch//eos/cms/store/group/alca_trackeralign/debug/PromptCalibProdSiPixelAli.root
%MSG-e MillePedeFileReader:   MillePedeDQMModule:SiPixelAliDQMModule@endProcessBlock  07-Sep-2020 12:51:32 CEST post-events
Could not read millepede end-file.
%MSG
%MSG-e MillePedeFileReader:   MillePedeDQMModule:SiPixelAliDQMModule@endProcessBlock  07-Sep-2020 12:51:32 CEST post-events
Could not read millepede log-file.
%MSG
%MSG-e MillePedeFileReader:   MillePedeDQMModule:SiPixelAliDQMModule@endProcessBlock  07-Sep-2020 12:51:32 CEST post-events
Could not read millepede result-file.
%MSG
DQMFileSaver::globalEndRun()
%MSG-w Alignment:   AlignmentProducerAsAnalyzer:SiPixelAliPedeAlignmentProducer@endJob  MillePedeAlignmentAlgorithm::doIO() 07-Sep-2020 12:53:10 CEST EndJob
'vstring mergeTreeFiles' and 'vstring mergeBinaryFiles' differ in size
%MSG

The output DQM file contains no information whatsoever of the alignment parameters fit, while in previous releases the same command does not have any issue:

CMSSW_11_2_0_pre4 CMSSW_11_2_0_pre3
image image

I strongly suspect that's tied to #30698.
It looks like MillePedeFileReader ::read()

void MillePedeFileReader ::read() {
readMillePedeEndFile();
readMillePedeLogFile();
readMillePedeResultFile();

is called even before the result files millepede.end (res, log) are created.
What guarantees the ordering is correct in the endProcessBlock()?
The sequence which is run is:

process.ALCAHARVESTSiPixelAli = cms.Sequence(process.SiPixelAliMilleFileExtractor+process.SiPixelAliPedeAlignmentProducer+process.SiPixelAliDQMModule+process.dqmEnvSiPixelAli)
process.SiPixelAli = cms.Path(process.ALCAHARVESTSiPixelAli)
process.schedule = cms.Schedule(*[ process.SiPixelAli, process.ALCAHARVESTDQMSaveAndMetadataWriter ], tasks=[process.patAlgosToolsTask])

cc:
@dmeuser @connorpa @vbotta

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 7, 2020

A new Issue was created by @mmusich Marco Musich.

@Dr15Jones, @dpiparo, @silviodonato, @smuzaffar, @makortel, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@Dr15Jones
Copy link
Contributor

assign alca, dqm

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 7, 2020

New categories assigned: dqm,alca

@jfernan2,@christopheralanwest,@andrius-k,@fioriNTU,@tlampen,@pohsun,@tocheng,@kmaeshima you have been requested to review this Pull request/Issue and eventually sign? Thanks

@makortel
Copy link
Contributor

makortel commented Sep 7, 2020

What guarantees the ordering is correct in the endProcessBlock()?

Do I read the code correctly that AlignmentProducerAsAnalyzer creates the files in endJob() , and the intention is for the MillePedeDQMModule to read something from them into DQM?

Then a correct ordering can be guaranteed by changing AlignmentProducerAsAnalyzer to write the files in endProcessBlock() instead of endJob() and to produce something into ProcessBlock (e.g. empty struct AlignmentToken{};), and changing MillePedeDQMModule to consume that product.

@mmusich
Copy link
Contributor Author

mmusich commented Sep 8, 2020

Hi @makortel

Do I read the code correctly that AlignmentProducerAsAnalyzer creates the files in endJob() , and the intention is for the MillePedeDQMModule to read something from them into DQM?

the intention is that MillePedeDQMModule reads something from those files and then puts it into DQM.

Then a correct ordering can be guaranteed by changing AlignmentProducerAsAnalyzer to write the files in endProcessBlock() instead of endJob() and to produce something into ProcessBlock (e.g. empty struct AlignmentToken{};), and changing MillePedeDQMModule to consume that product.

Thanks for the suggestion.
Can you point to some documentation and / or example on how that should be done?
Also AlignmentProducerAsAnalyzer (as the name is betraying) is an EDAnalyzer (actually: edm::one::EDAnalyzer<edm::one::WatchRuns, edm::one::WatchLuminosityBlocks, edm::one::SharedResources>.

class AlignmentProducerAsAnalyzer
: public AlignmentProducerBase,
public edm::one::EDAnalyzer<edm::one::WatchRuns, edm::one::WatchLuminosityBlocks, edm::one::SharedResources> {

Does your suggestion imply it should become an EDProducer?

@makortel
Copy link
Contributor

makortel commented Sep 8, 2020

@mmusich

Then a correct ordering can be guaranteed by changing AlignmentProducerAsAnalyzer to write the files in endProcessBlock() instead of endJob() and to produce something into ProcessBlock (e.g. empty struct AlignmentToken{};), and changing MillePedeDQMModule to consume that product.

Thanks for the suggestion.
Can you point to some documentation and / or example on how that should be done?

In a sense DQM itself serves as an example (but see more specific suggestions below).

Also AlignmentProducerAsAnalyzer (as the name is betraying) is an EDAnalyzer

Does your suggestion imply it should become an EDProducer?

Yes, it would have to become an EDProducer, just like in DQM all DQMEDAnalyzers are actually EDProducers.

Overall the necessary changes would be, in addition,

  • Make AlignmentProducerAsAnalyzer to use edm::Accumulator, and edm::EndProcessBlockProducer abilities
    • Accumulator in order to keep the module running for each event, similar to DQMEDAnalyzer
  • Create a "token" class similar to DQMToken, I'll call it AlignmentToken in the following but the name can be "anything"
    • In principle DQMToken could be used as well, but I think a separate product type would make the intention more clear, and with that there would be no interference with some DQM code consuming all DQMTokens in ProcessBlock in the future
  • Make AlignmentProducerAsAnalyzer to produce the AlignmentToken in endProcessBlockProduce()
  • Make MillePedeDQMModule to consume the AlignmentToken in the edm::Transition::EndProcessBlock transition edm::InProcess branch. The consumes declaration is sufficient, there is no need to read the actual token.

@mmusich
Copy link
Contributor Author

mmusich commented Sep 9, 2020

@makortel
Thanks for the detailed recipe. I think I have something working for the AlignmentProducerAsAnalyzer but it's not very clear to me what is the right syntax to do this

Make MillePedeDQMModule to consume the AlignmentToken in the edm::Transition::EndProcessBlock transition. The consumes declaration is sufficient, there is no need to read the actual token.

I've tried:

diff --git a/Alignment/MillePedeAlignmentAlgorithm/plugins/MillePedeDQMModule.cc b/Alignment/MillePedeAlignmentAlgorithm/plugins/MillePedeDQMModule.cc
index ea391c5e9ac..a8fca1594bf 100644
--- a/Alignment/MillePedeAlignmentAlgorithm/plugins/MillePedeDQMModule.cc
+++ b/Alignment/MillePedeAlignmentAlgorithm/plugins/MillePedeDQMModule.cc
@@ -30,8 +30,14 @@
 /*** Thresholds from DB ***/
 #include "CondFormats/DataRecord/interface/AlignPCLThresholdsRcd.h"
 
+/*** Necessary Framework infrastructure ***/
+#include "FWCore/Framework/interface/ProcessBlock.h"
+#include "DataFormats/Alignment/interface/AlignmentToken.h"
+
 MillePedeDQMModule ::MillePedeDQMModule(const edm::ParameterSet& config)
-    : mpReaderConfig_(config.getParameter<edm::ParameterSet>("MillePedeFileReader")) {}
+  : mpReaderConfig_(config.getParameter<edm::ParameterSet>("MillePedeFileReader")) {
+  consumes<AlignmentToken, edm::Transition::EndProcessBlock>();
+}
 
 MillePedeDQMModule ::~MillePedeDQMModule() {}

but that clearly doesn't even compile.
Having a look in cmssw I see similar usages e.g. here:

consumes<DQMToken, edm::InRun>(edm::InputTag("siStripQTester", "DQMGenerationQTestRun"));

what's the syntactic equivalent for ProcessBlock of edm::InRun ?

@Dr15Jones
Copy link
Contributor

what's the syntactic equivalent for ProcessBlock of edm::InRun ?

Your syntax was correct, however you stll need to pass an edm::InputTag as an argument to the consumes function.

@mmusich
Copy link
Contributor Author

mmusich commented Sep 9, 2020

@Dr15Jones perhaps I am doing something very dumb but using:

+  consumes<AlignmentToken, edm::Transition::EndProcessBlock>(edm::InputTag("alignmentToken"));

I am still getting this compilation error: https://paste.in/VlCgCb

@Dr15Jones
Copy link
Contributor

The second template argument isn't a Transition, it is a edm::BranchType. So you want

consumes<AlignmentToken, edm::InProcess>(edm::InputTag("alignmentToken"));

Sorry I missed that initially.

@makortel
Copy link
Contributor

makortel commented Sep 9, 2020

The second template argument isn't a Transition, it is a edm::BranchType.

Right, apologies for the confusion in my recipe #31381 (comment) (which I just updated), without thinking I was assuming that consumes() would work also with Transition as produces() does.

@mmusich
Copy link
Contributor Author

mmusich commented Sep 9, 2020

@Dr15Jones @makortel thanks for the suggestions.
Indeed with using consumes<AlignmentToken, edm::InProcess>(edm::InputTag("alignmentToken")); it compiles, though I am still unable to make it run in the correct order. I prepared a draft PR #31411, if you can give a quick look and suggest what is still missing.
Thanks!

mmusich added a commit to mmusich/cmssw that referenced this issue Sep 9, 2020
@mmusich
Copy link
Contributor Author

mmusich commented Sep 22, 2020

This issue has been solved by #31411 AlCa and DQM conveners might want to sign this.

@mmusich mmusich closed this as completed Sep 22, 2020
@christopheralanwest
Copy link
Contributor

christopheralanwest commented Sep 22, 2020

+alca

For the record, the PR that resolves the issue is #31411.

@jfernan2
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

This issue is fully signed and ready to be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants