cms-sw / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1.09k stars 4.33k forks source link

Enforce that Process can have at most one Schedule object, and the label must be schedule #35842

Open makortel opened 3 years ago

makortel commented 3 years ago

There should be 0 or 1 cms.Schedule objects in the cms.Process, and the label of the cms.Schedule should be schedule.

Issue https://github.com/cms-sw/cmssw/issues/35624 made it apparent that ConfigBuilder and HLT menus have been using cms.Schedule object labelled HLTSchedule for a long time. We should look into expressing the desired behavior with the label process.schedule, after which we could enforce the rule.

cmsbuild commented 3 years ago

A new Issue was created by @makortel Matti Kortelainen.

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

cms-bot commands are listed here

makortel commented 3 years ago

assign core, operations, hlt

cmsbuild commented 3 years ago

New categories assigned: operations,core,hlt

@fabiocos,@qliphy,@davidlange6,@missirol,@Dr15Jones,@smuzaffar,@perrotta,@makortel,@Martin-Grunewald you have been requested to review this Pull request/Issue and eventually sign? Thanks

makortel commented 3 years ago

@cms-sw/hlt-l2 I'd like to first understand if the cms.Schedule label of HLTSchedule has any specific meaning in all the other uses of HLT menus than in conjunction with ConfigBuilder (e.g. use in the online farm, ConfDB)?

Martin-Grunewald commented 3 years ago

HLTSchedule is used in various places - see: https://cmssdt.cern.ch/lxr/search?%21v=CMSSW_12_1_X_2021-10-24-2300&_filestring=&_string=HLTSchedule&_casesensitive=1 It is created by ConfDb, based on the list of all paths, when extracting the HLT menu python dumps for the release located in HLTrigger/Configuration/test and ../python.

makortel commented 3 years ago

Thanks @Martin-Grunewald, I was expecting to see HLTSchedule in many places within CMSSW.

Do I understand correctly that the HLTSchedule name gets added by the ConfDB code? That would add a complication to a migration, but it would still seem possible to do (e.g. I would imagine it to be possible to rename HLTSchedule to schedule in the customization functions that are applied to the fragment in the HLT menu dumps when they are loaded into cms.Process).

Martin-Grunewald commented 3 years ago

HLTSchedule is written here: https://github.com/cms-sw/hlt-confdb/blob/confdbv3/src/confdb/converter/python/PythonConfigurationWriter.java#L217 so I presume its name/label can be changed...

missirol commented 3 years ago

@Martin-Grunewald @makortel,

Based on your comments, I tried to implement a workaround in #35858, with the following in mind:

A biproduct of these changes will be that the edmConfigDump of a HLT-only config will now include the schedule (which is currently not the case, b/c HLTSchedule gets ignored).

missirol commented 3 years ago

@makortel @Martin-Grunewald (FYI: @Sam-Harper @fwyzard @silviodonato)

Just a quick update on this issue.

In my intentions, the use of HLTSchedule in CMSSW has been effectively removed in 12_2_X by #35858 (plus some further cleanup in #36073), except for the pkg HLTrigger/Configuration. In the latter pkg, we are currently renaming the object process.HLTSchedule to schedule.

The next step will be to rename HLTSchedule to schedule in the ConfDB source code.

This change looks very simple; otoh, it is an external change, and requires some planning. In my understanding:

  1. this ConfDB update should be done only after we know we don't need to do any more reparsing/migration of HLT menus in 12_1_X or lower (maybe we are already at this stage);

  2. this ConfDB update could affect users of older releases that are using customisation functions that assume the existence of HLTSchedule (for example, on top of hltGetConfiguration); I'm not sure how big of an issue this could be; for example, I realised that customizeHLTforPatatrack does not make this assumption, so it could already cope today with this change in ConfDB; I also don't see other 'central' customisation functions in HLTrigger/ that assume HLTSchedule in 12_{0,1}_X (except for this one which isn't really used).

Maybe we could consider making this change in ConfDB between 12_2_0 and 12_3_0 (i.e. between Jan-11 and Mar-1, in the current release schedule), but I'm not sure I see all the possible implications of this change.

It would be useful to hear your comments/suggestions.

makortel commented 3 years ago

@missirol I can't comment when would be a good time to make the change in ConfDB itself (except I'm fully with you that it needs to be planned carefully), but I thank you for all the work in CMSSW side!

Martin-Grunewald commented 3 years ago

We could also do it for 12_2_0 already, ie, between now and early January and see during this period if there are unexpected side effects. The ConfDb change only affects the extracted py configs. In any case we need to advertise in Monday/Wednesday TSG meetings

missirol commented 3 years ago

In any case we need to advertise in Monday/Wednesday TSG meetings

Agreed, I will add a bullet on this for next Monday, and we can discuss it then.

missirol commented 3 years ago

Hi all, a quick update on this issue from the HLT side.

A reminder: the goal is to deploy an update of ConfDB (external to CMSSW) so that it outputs schedule instead of HLTSchedule (thus removing all refs to HLTSchedule in master moving forward).

This ConfDB update can have side effects for users of 12_{0,1}_X, so HLT needs to make sure this is minimised (users of releases "< 12_X_Y" have access only to an older version of ConfDB, a.k.a. "ConfDB-v2", which will not be updated anyway).

As discussed in #36237, we converged on the following plan:

This plan implies that a new 12_0 release and a new 12_1 release will be needed. After that, it would be possible to deploy the ConfDB update, and complete the HLT side of this issue.

Please let us know if you see any issues with this plan.

missirol commented 2 years ago

Quick update from HLT:

Unless there are objections, I think I can put the HLT signature on this issue. We will of course continue to follow it.

missirol commented 2 years ago

+hlt

makortel commented 2 years ago

Thanks @missirol!