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

Confdb schedule->HLTSchedule 1210 #36237

Closed

Conversation

Sam-Harper
Copy link
Contributor

PR description:

The HLT has been using HLTSchedule as its schedule. However this has been fixed in 12_2_0. We now need to adjust confdb to output schedule.

This will change the output for all releases and thus we need to fix this for 12_1 and 12_0 as the code will expect confdb to supply HLTScheule when its supplying schedule.

There are two options as I see them

  1. backport the changes to make everything use schedule to make HLT use "schedule" #35858
  2. simply adjust the confdb output back to "HLTSchedule" in these releases

I have no opinion on which one to do, honestly option 1) is probably more correct. This is option 2), if we dont like it, we can do option 1) or something else.

PR validation:

Output with this PR is unchanged
Output with this PR with the new converter is unchanged

Backports

Note, this is special as it fixes a problem in 12_0 , 12_1 that does not exist in 12_2. Hence it doesnt go to the master branch.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Sam-Harper (Sam Harper) for CMSSW_12_1_X.

It involves the following packages:

  • HLTrigger/Configuration (hlt)

@cmsbuild, @missirol, @Martin-Grunewald can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @silviodonato this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@missirol
Copy link
Contributor

missirol commented Nov 25, 2021

Thanks for the PR, Sam.

I can see how this can be useful for 12_0 and 12_1. Let me summarise, so we can decide how to proceed (most of these things are already known, but I prefer to write it down).

--

The ConfDB update applies only to ConfDB-v3, so it only relates to CMSSW_12_X_Y, as older CMSSW releases do not support the use of ConfDB-v3.

--

For 12_2_X:

this release has basically always supported only schedule given the integration of #35858 in 12_2_0_pre2 ('basically' means with the exception of 12_2_0_pre1, which I'm going to neglect b/c that release is equivalent to 12_1_0_pre5); this means that (1) if we have to recreate the extracted HLT_*_cff files after the ConfDB update (so going to HLTSchedule to schedule even inside HLTrigger/Configuration), the framework can cope with that, and (2) there should be no user recipe making use of HLTSchedule in 12_2_X, so no 12_2 recipes that the ConfDB update could break.

--

So, we can restrict the discussion to 12_{0,1}_X (in this matter, the 12_0 and 12_1 cycles are on the same footing).

Here, the rest of CMSSW, and any user recipes developed up to now, assume the existence of HLTSchedule.

This means that right now we could not afford to recreate the extracted HLT_*_cff after the ConfDB update (as that would remove HLTSchedule), but there is no plan to recreate those cff files anyway in those releases; that's why #35858 was not backported. If we had a need to fix this, it could be done by either backporting #35858, or integrating this PR (the two options laid out in the PR description).

What the ConfDB update would also break in 12_{0,1}_X is user recipes that assume HLTSchedule (U), e.g.

# call this use case "U"
hltGetConfiguration [..] --customise [..].myFunctionThatAssumesHLTSchedule

Once ConfDB is updated, this is unavoidable for existing CMSSW releases (can't change those), so people will likely encounter this issue. The question (Q) is whether we want to have new releases in 12_0 and 12_1 that can support U after ConfDB is updated. My assumption was 'no' (my thinking was that the user has to anyway do something, whether that is to change release, or rename HLTSchedule to schedule in his/her own functions), but I have no problem with going for 'yes'.

I can see how this PR ("option 2)") helps with U; I think "option 1)" (backport of #35858) would not fix U.

So, if the answer to Q is yes, we would need new releases in 12_0 and 12_1 with this PR in, and after that we could proceed with the ConfDB update. If the answer to Q is no, we could proceed with the ConfDB update right away.

If I'm missing something, please let me know.

--

Note : the ConfDB update will not affect the recipe created by @silviodonato for Run-3 development in 12_1, as he already took care of backporting #35858, and doing HLTSchedule -> schedule in all the relevant POG customisation functions. He can correct me, if I'm wrong.

@silviodonato
Copy link
Contributor

Thank you @missirol for the detailed summary.
Yes, I included #35858 in the customization functions, in this way they works both in 12_1_X and 12_2_X and they use schedule.
I assume that all the private user customization functions will use schedule, as they need to be integrated in 12_2_X.

My main concern is about the customization functions which are already in CMSSW.
I see there is no HLTSchedule in https://github.com/cms-sw/cmssw/blob/CMSSW_12_1_X/HLTrigger/Configuration/python/customizeHLTforCMSSW.py
while HLTSchedule appears in https://github.com/cms-sw/cmssw/blob/CMSSW_12_1_X/HLTrigger/Configuration/python/customizeHLTforPatatrack.py

This means that case "U" would work with customiseFor2018Input but it would not work with customizeHLTforPatatrack.

If I understand correctly

  • backport make HLT use "schedule" #35858 would fix customizeHLTforPatatrack to run with schedule, and in general will allow us to run using the schedule customization function. It would be useful in this period that we use both CMSSW_12_1_X and CMSSW_12_2_CMSSW_12_2_0_preX`
  • either PR Confdb schedule->HLTSchedule 1210 #36237 or a backport make HLT use "schedule" #35858 would be useful to allow us to use hltGetConfiguration in CMSSW_12_0_X. This might be useful for CMSSW_12_0_X, even though I don't see any specific use case.

So I would have a slight preference for a backport of #35858 to CMSSW_12_1_X.
Either this PR #36237 or #35858 for CMSSW_12_0_X.

What do you think?

@Sam-Harper
Copy link
Contributor Author

So my two cents, we do need to do something to 12_0 and 12_1 as I think we should continue to have the workflow of downloading and running a menu here supported.

I think its reasonable to say to folks, you need to use schedule in your scripts in 12_1 as people will be using 12_1/12_2 right now and its good to ensure everybody is using schedule. So as stated, I also like having #35858 backported to 12_1.

I think for 12_0, it would be much simpler if we just did what is proposed here. Its minimal work on our part (like none) and 12_0 is now deprecated anyways. For this my motivation is just to minimise work on our part while having this workflow still working.

Anyways its yours, Marino and Martin's call , I dont have a strong preference other than having some solution for 12_0/12_1

@missirol
Copy link
Contributor

This means that case "U" would work with customiseFor2018Input but it would not work with customizeHLTforPatatrack.

(reminder: "U" means hltGetConfiguration [..] --customise [..].myFunctionThatAssumesHLTSchedule)

Not exactly. The Patatrack customisation does not assume HLTSchedule (so, it does not fall under "U"), and can already cope with the ConfDB update; it does check for the existence of HLTSchedule (and works around some things if that exists, see #35722), but it does not assume its existence. This can be checked with [*].

The only case I know of a function in HLTrigger/ which assumes HLTSchedule in 12_{0,1}_X is this one, which is there mostly as an example and not really for development.

I think the case of "U" mostly applies to user functions we don't control.

my two cents, we do need to do something to 12_0 and 12_1 as I think we should continue to have the workflow of downloading and running a menu here supported.

Agreed. Maybe I miss what you mean by "downloading and running", but in my understanding standard usage of hltGetConfiguration was never at risk. The only potential problem I saw comes from the combination of hltGetConfiguration with user functions we don't control.


Trying to summarise, the current consensus would be:

  • 12_0_X needs the equivalent of this PR; what HLT provides (to other parts of CMSSW, and to users in general) continues to be HLTSchedule (even after the ConfDB update), like in existing 12_0_X releases.

  • 12_1_X needs the backport of make HLT use "schedule" #35858 (if ORP managers agree); what HLT provides (to other parts of CMSSW, and to users in general) will change from HLTSchedule to schedule; this breaks continuity wrt existing 12_1 releases (and will break use case "U" for users), but the advantage is that 12_1 and 12_2 will behave in the same way, and this is considered more important.

This implies that new 12_0 and 12_1 releases are needed before deploying the new version of ConfDB.

Are there any objections/corrections?

[*]

cmsrel CMSSW_12_1_X_2021-11-24-2300
cd CMSSW_12_1_X_2021-11-24-2300/src
cmsenv

# v3-test uses the updated ConfDB returning "schedule", so there is no "HLTSchedule" in this wf
hltGetConfiguration v3-test/run3:/dev/CMSSW_12_1_0/GRun --max-events 10 --no-output --unprescale --input /store/data/Run2018D/EphemeralHLTPhysics1/RAW/v1/000/323/775/00000/2E066536-5CF2-B340-A73B-209640F29FF6.root --data --globaltag auto:run3_hlt --customise HLTrigger/Configuration/customizeHLTforCMSSW.customiseFor2018Input,HLTrigger/Configuration/customizeHLTforPatatrack.customizeHLTforPatatrack > hlt.py && cmsRun hlt.py &> hlt.log

PS. I do understand that I'm wasting everybody's time with this long discussion on such a small change, so sorry about that.

@Martin-Grunewald
Copy link
Contributor

I would agree with Sam, #36237 (comment)

@missirol
Copy link
Contributor

This means having this PR in 12_0_X (so, closing this one targeting 12_1_X), and backporting #35858 to 12_1_X.
I'll start from the latter, and I can take care of both.

@qliphy
Copy link
Contributor

qliphy commented Nov 29, 2021

This means having this PR in 12_0_X (so, closing this one targeting 12_1_X), and backporting #35858 to 12_1_X. I'll start from the latter, and I can take care of both.

@missirol I see #36251 merged in 12_0_X, and #35858 backported to 12_1_X with #36252, should then this PR #36237 be closed as planned?

@missirol
Copy link
Contributor

@qliphy

Yes, this can be closed. Thanks.

@qliphy
Copy link
Contributor

qliphy commented Nov 29, 2021

thanks!

@qliphy qliphy closed this Nov 29, 2021
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.

6 participants