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

Question on Era of Run-3 2024 workflows #43756

Closed
missirol opened this issue Jan 19, 2024 · 24 comments
Closed

Question on Era of Run-3 2024 workflows #43756

missirol opened this issue Jan 19, 2024 · 24 comments

Comments

@missirol
Copy link
Contributor

If I checked correctly, most of the current 2023 wfs use --era Run3_2023, while the 2024 ones use --era Run3 (e.g. wf 12824.0).

Even if correct, this is arguably a bit confusing.

Re-iterating what was asked in #41271 (comment), would it make sense to create a appropriate Run3_2024 era to be used by these wfs, and maybe even a Run3_2025 era for future usage ?

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 19, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

A new Issue was created by @missirol Marino Missiroli.

@rappoccio, @Dr15Jones, @makortel, @antoniovilela, @sextonkennedy, @smuzaffar can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@makortel
Copy link
Contributor

assign reconstruction, operations, pdmv

(somewhat unsure who exactly assign to)

@cmsbuild
Copy link
Contributor

New categories assigned: reconstruction,operations,pdmv

@AdrianoDee,@davidlange6,@jfernan2,@sunilUIET,@miquork,@mandrenguyen,@rappoccio,@antoniovilela,@fabiocos you have been requested to review this Pull request/Issue and eventually sign? Thanks

@mmusich
Copy link
Contributor

mmusich commented Jan 20, 2024

Re-iterating what was asked in #41271 (comment), would it make sense to create a appropriate Run3_2024 era to be used by these wfs, and maybe even a Run3_2025 era for future usage ?

#43761 is a (draft) proposal along these lines.

@missirol
Copy link
Contributor Author

Thanks @mmusich !

@malbouis
Copy link
Contributor

malbouis commented Jan 21, 2024

Hi! If I remember correctly, Run3_2023 was created in order to have the Hcal PF thresholds modified properly avoiding the need of including cumbersome customizations in cmsDriver. It was also used in a Tier0 scenario during the 2023 data-taking for updating the hcal pf thresholds.

But for 2024 we intend to have these hcalpfcuts being consumed directly from the GT. An overview of the integration of the consuming of the hcalpfcuts from the GT is ongoing here: #43312

I might be misunderstanding something of course, so please correct me if it is the case.

Otherwise I would think that there is known reason to have the Run3_2024 and so on added (at least not for the Tier0 scenarios) unless there are foreseen customizations that should be included. But I don't have a strong opiniion about it, just wanted to point it out.

Thanks!

@mmusich
Copy link
Contributor

mmusich commented Jan 22, 2024

Otherwise I would think that there is known reason to have the Run3_2024 and so on added (at least not for the Tier0 scenarios) unless there are foreseen customizations that should be included. But I don't have a strong opiniion about it, just wanted to point it out.

While probably not strictly necessary for Tier0, for any application that does not read from GlobalTag I think you still want to use the Hcal PF cuts à la 2023, no? Also it is awkward to go back to Run3, given that so far any year has been built on top of previous one, see #41271 (comment). What are the cons of the proposal at #43761?

@malbouis
Copy link
Contributor

From a quick look at #43761 , I understood that it is taking the the HB rechit threshold values from Run3_2023 which in turn sets these values "by hand" through a modifier (please correct me if I'm wrong). A potential problem would arise in case hcal would need to update their thresholds. We would have to keep track of the differences between the thresholds updated in the GT and the ones introduced manually in the Run3_2024 (and Run3_2025) scenarios. There could also be potential delays due to the fact that for updating the scenario and Era one necessarily needs to have also a new release.

@mmusich
Copy link
Contributor

mmusich commented Jan 22, 2024

I understood that it is taking the the HB rechit threshold values from Run3_2023 which in turn sets these values "by hand" through a modifier (please correct me if I'm wrong).

mmh, no. We don't override the default usePFThresholdsFromDB = True used at #43025 and elsewhere. It just configures the python version of the threshold to agree with the content of the GT.

We would have to keep track of the differences between the thresholds updated in the GT and the ones introduced manually in the Run3_2024 (and Run3_2025) scenarios. There could also be potential delays due to the fact that for updating the scenario and Era one necessarily needs to have also a new release.

as long as there is even the possibility to configure the threshold manually, having them to stay in synch is a maintenance concern. If ultimately people don't want to be liable to having them stay in synch I would suggest to remove the provision to pass them by configuration.

@malbouis
Copy link
Contributor

mmh, no. We don't override the default usePFThresholdsFromDB = True used at #43025 and elsewhere. It just configures the python version of the threshold to agree with the content of the GT.

Thanks, indeed the default thresholds are the ones from GT. But what I was trying to argue is that in case we need to change the thresholds in 2024, for example, we would also need to create a new modifier with the new values (iiuc), similar to https://github.com/cms-sw/cmssw/blob/master/Configuration/Eras/python/Modifier_run3_egamma_2023_cff.py and there would need to be a new release etc, which is what we were trying to move away from by introducing the HcalPFCuts in the GT.
So I guess I am asking what is the exact need of these new Eras/scenarios ? I am not convinced that using Run3_2024 scenario at Tier0 is mandatory.

as long as there is even the possibility to configure the threshold manually, having them to stay in synch is a maintenance concern. If ultimately people don't want to be liable to having them stay in synch I would suggest to remove the provision to pass them by configuration.

Indeed, keeping them in synch is a concern. So maybe in the future the option of passing them via configuration could be reconsidered or restricted to a specific use case. I would humbly suggest to not propagate the possibility of having not synched thresholds (via GT versus configuration) used in RelVals and Tier0 scenarios.

But I am happy to discuss more about all the implications of having these new scenarios. Thanks for opening the PR and the discussion and thanks Marino for opening this issue! I think it is important to understand all the pros and cons before moving forward with it, in case it is decided so.

@mmusich
Copy link
Contributor

mmusich commented Jan 22, 2024

But what I was trying to argue is that in case we need to change the thresholds in 2024, for example, we would also need to create a new modifier with the new values (iiuc), similar to https://github.com/cms-sw/cmssw/blob/master/Configuration/Eras/python/Modifier_run3_egamma_2023_cff.py and there would need to be a new release etc, which is what we were trying to move away from by introducing the HcalPFCuts in the GT.

well, as long as the tier-0 uses the GT, there is no need of any modifier or any other action.

I would humbly suggest to not propagate the possibility of having not synched thresholds (via GT versus configuration) used in RelVals and Tier0 scenarios.

then we are agreed! Without #43761 this is precisely what happens (2023 threshold in GT vs old thresholds in configuration).

So I guess I am asking what is the exact need of these new Eras/scenarios ?

see #43756 (comment). Also in case something happens to the detector (likely) that needs special treatment, it's already there.
All in all, I still don't see any con.

@mmusich
Copy link
Contributor

mmusich commented Jan 22, 2024

This way the egamma modifier is not called (there would be no mismatch between the GT and what the scenario would use from the configuration if the GT was not default)

I am not understanding why the egamma modifier should not be called. In this way there would be a mismatch between GT and configuration. Or do I misunderstand that we don't want to use the 2023 thresholds for 2024?
At least in MC GlobalTags they are the same.

@malbouis
Copy link
Contributor

I am not understanding why the egamma modifier should not be called. In this way there would be a mismatch between GT and configuration. Or do I misunderstand that we don't want to use the 2023 thresholds for 2024? At least in MC GlobalTags they are the same.

If the modifier is not called, then the only information we would have (code wise) would be from the GT (assuming all consuming code was migrated to get the thresholds from the GT), so no apparent mismatch, in case one goes through the code in a few years from now.
My discomfort with introducing this scenario with the call to the thresholds set by hand are exactly that we loose the ability of changing them quickly through the GT only (in case it is needed) and there is the risk of mismatch between the configuration and GT that would be put in evidence through the scenarios/Eras.
In 2023, there was the need to introduce this scenario for these thresholds, now in 2024 the thresholds are in the GT, so in principle no need for the scenario, unless it is to be a placeholder for other issues (in which case, I would not include the thresholds called from the modifier).
But I feel we are now going in circles. :-)
So, to summarize, from my point of view: I see the point of having those scenarios as a placeholder, but then it should not introduce unneeded settings for 2024 (like the manual thresholds) and the HB thresholds should be read from the GT only (with no calls to modifiers that would potentially change them);

@mmusich
Copy link
Contributor

mmusich commented Jan 22, 2024

but then it should not introduce unneeded settings for 2024 (like the manual thresholds) and the HB thresholds should be read from the GT only (with no calls to modifiers that would potentially change them);

to the best of my understanding even with #43761 the thresholds will always be read from GT only (the modifier does not act on switching the reading from DB on or off, it just specifies which thresholds to read IF something else overrides the reading from DB). @swagata87 might dispel any confusion in case there's something wrong in the reasoning.

@swagata87
Copy link
Contributor

I will think a bit more. For now I just want to point out that there is another era called Run3_2022_rereco, which is not in master branch but it is only in 12_4_X branch. It was introduced via #41581 in 12_4_X and was never forward-ported to master. This comment #41581 (comment), and later comments in that PR has some discussions along the same lines. Maybe this is relevant for the discussion here.

@mmusich
Copy link
Contributor

mmusich commented Jan 26, 2024

what's an appropriate venue to discuss this in more details?

@malbouis
Copy link
Contributor

Hi @mmusich , we could discuss this at the ORP meeting this Tuesday.
@cms-sw/reconstruction-l2 , do you have a preference on how to go for the Eras?

@mandrenguyen
Copy link
Contributor

@malbouis I haven't yet given it a ton of thought, so I'm not sure I should comment, but my first instinct would be to only introduce a new era at which point it actually becomes necessary.
If settings can be controlled entirely based on the GT, then we should keep it like that.

@mmusich
Copy link
Contributor

mmusich commented Jan 26, 2024

If settings can be controlled entirely based on the GT, then we should keep it like that.

I think people have entirely misunderstood the purpose of this issue. Probably discussing by voice at the ORP meeting might clarify few things.

@malbouis
Copy link
Contributor

malbouis commented Feb 2, 2024

I realised there was no summary here of the outcome of the discussion at the ORP meeting last Tuesday. From what I understood, it was decided to remove Run3_2023 Era from the RunTheMatrix workflows and go back to using Run3 Era. This is now possible as the hcalpfcuts are consumed from the GT. Thanks @makortel for the suggestion.
Also, thanks @mmusich and @RSalvatico for this bug fix, PR #43819 .
Please all, let us know if @cms-sw/pdmv-l2 can proceed with updating the workflows.

@mandrenguyen
Copy link
Contributor

@malbouis Good from my side.

@mandrenguyen
Copy link
Contributor

+reconstruction
Summary here: #43756 (comment)

@missirol
Copy link
Contributor Author

missirol commented Sep 3, 2024

The Eras Run3_2024 and Run3_2025 were ultimately introduced via #43761 (14_1_X) and #45291 (14_0_X). Worth noting that Run3_2024 is not based on Run3_2023 (it is based directly on Run3), while Run3_2025 is based on Run3_2024 (see the relevant PRs for further details). Closing this issue. Thanks !

@missirol missirol closed this as completed Sep 3, 2024
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

7 participants