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

added RecoTracker-DisplacedRegionalTracking in cmssw deps #8318

Merged
merged 1 commit into from
Feb 18, 2023

Conversation

smuzaffar
Copy link
Contributor

No description provided.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @smuzaffar (Malik Shahzad Muzaffar) for branch IB/CMSSW_13_1_X/master.

@cmsbuild, @smuzaffar, @aandvalenzuela, @iarspider can you please review it and eventually sign? Thanks.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.
cms-bot commands are listed here

@mmusich
Copy link

mmusich commented Feb 16, 2023

seems that technically cms-sw/cmssw#40643 run with this addition. Should it be just merged?

@smuzaffar
Copy link
Contributor Author

@mmusich , this should go in along with cms-sw/cmssw#40643 . I would suggest to wait for cms-sw/cmssw#40643 to fully signed and then we can include it.

@mmusich
Copy link

mmusich commented Feb 16, 2023

I would suggest to wait for cms-sw/cmssw#40643 to fully signed and then we can include it.

is there a strong reason to not merge now?
I think it happened several times in the past that a PR depending on some external update was merged before the corresponding external was available leading to failures in IBs.
At any rate the actual input files in cms-data are already there.

@smuzaffar
Copy link
Contributor Author

smuzaffar commented Feb 16, 2023

@mmusich , this is a new data external and it is still not part of cmssw distribution. I would like to not include it in distribution unless the cmssw changes which require it converge. #6907 was opened 20months ago to include this data external but the cmssw PR cms-sw/cmssw#32806 did not converge. So I would suggest to wait for cms-sw/cmssw#40643 to converge first

@slava77
Copy link
Contributor

slava77 commented Feb 17, 2023

recall that a requirement for an external triggers the PR tests to use the head of the IB and could introduce unwanted differences (and repeated tests until by luck there are no relevant new PRs).

So, unless merging the external breaks the baseline, perhaps it's worth to consider changing the policy and merge?

@mmusich
Copy link

mmusich commented Feb 17, 2023

I would like to not include it in distribution unless the cmssw changes which require it converge. #6907 was opened 20months ago to include this data external but the cmssw PR cms-sw/cmssw#32806 did not converge.

@smuzaffar, while I see from where you are coming from, I really think that this time the PR is set to converge on a reasonable timescale (as there is push from physics to have it).

@smuzaffar smuzaffar merged commit ea1517e into IB/CMSSW_13_1_X/master Feb 18, 2023
@smuzaffar
Copy link
Contributor Author

itis merged now

@smuzaffar smuzaffar deleted the smuzaffar-patch-1 branch March 14, 2023 16:41
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.

4 participants