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

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

Open
makortel opened this issue Oct 25, 2021 · 16 comments

Comments

@makortel
Copy link
Contributor

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

Issue #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
Copy link
Contributor

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
Copy link
Contributor Author

assign core, operations, hlt

@cmsbuild
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor

Martin-Grunewald commented Oct 26, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor

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 added a commit to missirol/cmssw that referenced this issue Oct 27, 2021
For more information, see
cms-sw#35842
@missirol
Copy link
Contributor

@Martin-Grunewald @makortel,

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

  • changing the ConfDB source code right away would introduce issues for people that use (hltGetConfiguration + 'customisations that assume HLTSchedule') (e.g. Patatrack) in existing releases

  • as pointed by Matti, for now one could rename HLTSchedule to schedule in one of the fixed customisations that HLT applies on top of the ConfDB outputs, e.g. customizeHLTforAll; the other customisations (e.g. Patatrack) can be changed accordingly, confining the 1st step of these changes to CMSSW (users recipes that explicitly use HLTSchedule should be rare, and 'HLT-support' will have to help users with that if needed; it is a simple fix)

  • once 11_X and 12_0 become old enough (and people really stop using them for HLT development), the change in ConfDB can be done, and the HLT customisations cleaned up in master

  • after this, it should be possible for Core to enforce the requirement of not creating a Schedule not called schedule

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
Copy link
Contributor

@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
Copy link
Contributor Author

@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
Copy link
Contributor

Martin-Grunewald commented Nov 10, 2021

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
Copy link
Contributor

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.

silviodonato pushed a commit to silviodonato/cmssw that referenced this issue Nov 22, 2021
For more information, see
cms-sw#35842
@missirol
Copy link
Contributor

missirol commented Nov 25, 2021

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:

  • 12_0_X: needs #36251, which is an ad-hoc change for this release, so that HLT can continue to deliver HLTSchedule to its clients.

  • 12_1_X: needs #36252, which is a backport of #35858 (and a related PR), putting 12_1_X on the same footing as 12_2_X; since HLT development is mostly done in 12_1 and 12_2 these days, making the two branches as close as possible is considered the preferred way to go.

  • 12_2_X (currently, master): ready for the ConfDB update, no further action needed.

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
Copy link
Contributor

missirol commented Dec 9, 2021

Quick update from HLT:

  • the ConfDB update has been deployed offline with cms-sw/hlt-confdb/V03-01-00 (and is being propagated to DAQ via CMSHLT-2201);

  • counter-measures for older releases have been integrated (in 12_1_1 and 12_0_4) according the plan in #35842 (comment), and so far we didn't encounter any significant problems;

  • the updates in #36374 and #36386 effectively removed the usage of "HLTSchedule" in 12_3_X (master) and 12_2_X, respectively;

  • #36395 literally removed all mentions of it in 12_3_X (master), and was tested in an IB.

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

@missirol
Copy link
Contributor

+hlt

@makortel
Copy link
Contributor Author

Thanks @missirol!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants