search

cms-sw / cms-bot

A few scripts to automate approval / testing process
28 stars 245 forks source link

forcing merge of externals once cmssw PR is merged #2147

Closed mmusich closed 7 months ago

mmusich commented 9 months ago

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 https://github.com/cms-data/RecoEgamma-ElectronIdentification/pull/28#issuecomment-1893276157. I am wondering if it would be desireable to automatize the merge procedure to avoid having frequently broken IBs.

cmsbuild commented 9 months ago

cms-bot internal usage

cmsbuild commented 9 months ago

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 commented 9 months ago

@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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

@iarspider has opened https://github.com/cms-sw/cms-bot/pull/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 commented 9 months ago

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 commented 8 months ago

@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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

https://github.com/cms-sw/cms-bot/pull/2169 is ready to go it. This will allow bot to automatically open and merge cmsdist PR for cms-data changes

antoniovilela commented 8 months ago

https://github.com/cms-sw/cms-bot/pull/2169 is ready to go it. This will allow bot to automatically open and merge cmsdist PR for cms-data changes

Thanks

smuzaffar commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

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 commented 6 months ago

Example showing the model is not working, both

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

mmusich commented 6 months ago

Another example:

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

mmusich commented 1 month ago

Another example:

was merged without the corresponding externals update https://github.com/cms-data/CalibTracker-SiPixelESProducers/pull/4.

mandrenguyen commented 1 month ago

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