forcing merge of externals once cmssw PR is merged

[mmusich]

It is fairly frequent that an IB gets broken because @cms-sw/orp-l2 merge a PR without the corresponding cms-data update.
The most recent example is cms-data/RecoEgamma-ElectronIdentification#28 (comment).
I am wondering if it would be desireable to automatize the merge procedure to avoid having frequently broken IBs.

[cmsbuild]

cms-bot internal usage

[cmsbuild]

A new Issue was created by @mmusich Marco Musich.

@makortel, @Dr15Jones, @smuzaffar, @rappoccio, @antoniovilela, @sextonkennedy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[smuzaffar]

@mmusich , thanks for open this issue. I have thought of this but I am afraid that this can be misused e.g. L2 can add any cms-data PR (needed or not for cmssw). I am afraid if @cms-sw/orp-l2 are not paying attention to merge cms-data PR then it might be more difficult for them to pay attention to all extra PRs which cmssw PR can automatically merge :-)

[mmusich]

I have thought of this but I am afraid that this can be misused e.g. L2 can add any cms-data PR (needed or not for cmssw).

honestly seems a pretty weak argument as generally speaking L2 seems more conscientious than ORP.
In any case having to deal with broken IB on a daily basis is rather frustrating for development, not to speak of wasted resources (test job sent already doomed to fail), so please between you and @cms-sw/orp-l2 try to avoid this (recurrent) situation.

[smuzaffar]

I would prefer to be on safe side instead of merging PR automatically :-) @cms-sw/orp-l2 should pay bit more attention while merge/signing

[smuzaffar]

@iarspider has opened #2148 to add extra reminders (once PR is fully signed) for ORP to merge the extra PRs if needed. I hope this might help

[mmusich]

add extra reminders (once PR is fully signed) for ORP to merge the extra PRs if needed. I hope with might help

much appreciated, thank you.

[smuzaffar]

@mmusich , I guess we can close this issue now. Lets hope the extra reminder to @cms-sw/orp-l2 will help them merge the external PRs

[antoniovilela]

I do not know if you are looking for my opinion, but in any case.
Merging the cms-data related PR does not seem like a good idea. What could possibly be automated though is the merging of the cmsdist PR that is already automatically created after merging the cms-data PR, updating its tag in cmsdist.

[mmusich]

guess we can close this issue now. Lets hope the extra reminder to @cms-sw/orp-l2 will help them merge the external PRs

let's see how it goes, I would keep this open for a few more days.

[smuzaffar]

I do not know if you are looking for my opinion, but in any case. Merging the cms-data related PR does not seem like a good idea. What could possibly be automated though is the merging of the cmsdist PR that is already automatically created after merging the cms-data PR, updating its tag in cmsdist.

@antoniovilela , yes this seems like a reasonable thing to do. We will update bot to automatically merge cmsdist data PRs which are automatically created by bot. We will only do it if the cms-data PR was on the default branch (master or main). Note that sometimes we have special branches in cms-data repos for old release cycles and I think bot by default opens cmsdist PRs for master branch of cmsdist. So we do not want such PRs to go in cmsdist master.

[antoniovilela]

I do not know if you are looking for my opinion, but in any case. Merging the cms-data related PR does not seem like a good idea. What could possibly be automated though is the merging of the cmsdist PR that is already automatically created after merging the cms-data PR, updating its tag in cmsdist.

@antoniovilela , yes this seems like a reasonable thing to do. We will update bot to automatically merge cmsdist data PRs which are automatically created by bot. We will only do it if the cms-data PR was on the default branch (master or main). Note that sometimes we have special branches in cms-data repos for old release cycles and I think bot by default opens cmsdist PRs for master branch of cmsdist. So we do not want such PRs to go in cmsdist master.

Makes sense. Thanks.

[smuzaffar]

#2169 is ready to go it. This will allow bot to automatically open and merge cmsdist PR for cms-data changes

[antoniovilela]

#2169 is ready to go it. This will allow bot to automatically open and merge cmsdist PR for cms-data changes

Thanks

[smuzaffar]

@mmusich @antoniovilela , I think we can close this issue. Though bot does not automatic merge the external PR e.g. cms-data but it does now add a comment to remind @cms-sw/orp-l2 to check the externals PRs and also it automatically merge the cmsdist PR which were automatically created by bot for cms-data externals.

[antoniovilela]

@mmusich @antoniovilela , I think we can close this issue. Though bot does not automatic merge the external PR e.g. cms-data but it does now add a comment to remind @cms-sw/orp-l2 to check the externals PRs and also it automatically merge the cmsdist PR which were automatically created by bot for cms-data externals.

yes

[mmusich]

Though bot does not automatic merge the external PR e.g. cms-data but it does now add a comment to remind

I am not convinced that's sufficient, but fine - you decided to close the issue.

[mmusich]

Example showing the model is not working, both

• updated parameters and model for photon XGboost MVA (HLT) [14_1_X] cmssw#44719
• updated parameters and model for photon XGboost MVA (HLT) [14_0_X (backport)] cmssw#44720 (it's backport to 14.0.x)

were merged without the corresponding external update (cms-data/RecoEgamma-PhotonIdentification#15).
This resulted in broken unit tests:

• https://cmssdt.cern.ch/SDT/cgi-bin/logreader/el8_amd64_gcc12/CMSSW_14_1_X_2024-04-13-1100/unitTestLogs/RecoEgamma/PhotonIdentification#/42-42
• https://cmssdt.cern.ch/SDT/cgi-bin/logreader/el8_amd64_gcc12/CMSSW_14_0_X_2024-04-13-1100/unitTestLogs/RecoEgamma/PhotonIdentification#/42-42

[mmusich]

Another example:

• Introduce Unified Particle Transformer AK4 jet tagger cmssw#44641

was merged without the corresponding external update: cms-data/RecoBTag-Combined#57 resulting in widespread failures in IB, see e.g. log

[mmusich]

Another example:

• update createTestDBObjects unit test to use Tracker geometry T33 cmssw#45775

was merged without the corresponding externals update cms-data/CalibTracker-SiPixelESProducers#4.

[mandrenguyen]

Another example:

* [update `createTestDBObjects` unit test to use Tracker geometry T33 cmssw#45775](https://github.com/cms-sw/cmssw/pull/45775)

was merged without the corresponding externals update cms-data/CalibTracker-SiPixelESProducers#4.

Indeed, my bad, and thanks for pointing it out. Rookie mistake, but it's fixed now