Illegal include of PhysicsTools/PatAlgos/plugins in PhysicsTools/NanoAOD

[guitargeek]

Here is something that I noticed with the automatic BuildFile cleaning in #30873.

There is a place in NanoAOD where the header file from another plugins directory is included:

https://github.com/cms-sw/cmssw/blob/master/PhysicsTools/NanoAOD/plugins/BJetEnergyRegressionMVA.cc#L27

This should not be done, because it makes is difficult to handle the dependencies correctly and I think it's also not really allowed by the CMSSW design.

I think we should either consider moving PhysicsTools/PatAlgos/plugins/BaseMVAValueMapProducer.h to

1. PhysicsTools/PatAlgos/interface/BaseMVAValueMapProducer.h or to
2. PhysicsTools/NanoAOD/plugins/BaseMVAValueMapProducer.h.

IMHO the second option is better because the *BaseMVAValueMapProducer plugins are only used in PhysicsTools/NanoAOD, so it's not clear to my why they are in another package. However, I saw that this is where these plugins were previously, and then they got moved out. Therefore I didn't do anything about it in #30873, as my proposed change would have undone previous changes that might have been done so some good reason.

[cmsbuild]

A new Issue was created by @guitargeek Jonas Rembser.

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

cms-bot commands are listed here

[makortel]

assign xpog, analysis, reconstruction

[cmsbuild]

New categories assigned: analysis,xpog,reconstruction

@slava77,@fgolf,@mariadalfonso,@santocch,@perrotta,@jpata,@gouskos,@peruzzim you have been requested to review this Pull request/Issue and eventually sign? Thanks

[makortel]

There is a place in NanoAOD where the header file from another plugins directory is included:

https://github.com/cms-sw/cmssw/blob/master/PhysicsTools/NanoAOD/plugins/BJetEnergyRegressionMVA.cc#L27

This should not be done, because it makes is difficult to handle the dependencies correctly and I think it's also not really allowed by the CMSSW design.

Correct, including headers from plugins directory of another package is not allowed.

[slava77]

from #20626 description (and possibly induced memory of previous discussion)
I'd say that the PhysicsTools/NanoAOD/plugins/BJetEnergyRegressionMVA.cc should be moved to PhysicsTools/PatAlgos/plugins/

[slava77]

from #20626 description (and possibly induced memory of previous discussion)
I'd say that the PhysicsTools/NanoAOD/plugins/BJetEnergyRegressionMVA.cc should be moved to PhysicsTools/PatAlgos/plugins/

sorry, wrong file mentioned in the first post, edited now.
The rationale is that PhysicsTools/NanoAOD/plugins is to contain producers of just what's directly used in nano flat trees or nanoEDM files, while the ValueMaps are more generic and fit more appropriately the PatAlgos.

[guitargeek]

Thanks for your answer! If this is the rationale, I don't understand why the configurations of these plugins are exclusively done in NanoAOD (here, here, here and here), instead of in PatAlgos and then imported. But okay, doesn't really matter too much for this issue; it is clear now where the sources should go to avoid not-allowed includes.

[slava77]

Thanks for your answer! If this is the rationale, I don't understand why the configurations of these plugins are exclusively done in NanoAOD (here, here, here and here), instead of in PatAlgos and then imported. But okay, doesn't really matter too much for this issue; it is clear now where the sources should go to avoid not-allowed includes.

well, it could be that my view of what's expected in PhysicsTools/NanoAOD/plugins is a bit reductionist.
@cms-sw/xpog-l2
please comment.

[peruzzim]

I agree with @slava77 that we should put the header of the base class in PatAlgos/interface, and keep the BJetEnergyRegressionMVA in NanoAOD unless someone else takes ownership of this class and maintains it under the responsibility of a POG. Otherwise, if this is considered "analysis code" used only for NanoAOD production, it's much more practical to keep it in the NanoAOD directory where it can be properly maintained as the recommendations change over time.

[guitargeek]

Hi @peruzzim, thanks for your message! What you suggests sounds good, I'll make a quick PR to fix this include problem.