Remove GEMELMap from CMSSW, its an old format that's not used #33524
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33524/22283
|
|
A new Pull Request was created by @watson-ij (Ian J. Watson) for master. It involves the following packages: CondCore/GEMPlugins @malbouis, @yuanchao, @christopheralanwest, @cmsbuild, @tlampen, @ggovi, @pohsun, @francescobrivio can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
test parameters:
|
|
@cmsbuild please test |
|
@watson-ij are you also planning a backport to 113X? The backport for #33507 to 113X has also been merged (#33509) |
I think the backport is for the other issue in that PR. I don't think there's any particular need to backport this removal. |
The |
|
-1 Failed Tests: RelVals RelVals |
The failure here is: |
|
@francescobrivio Ah, that was a typo sorry. It should be the upgrade workflow 34621.0 that I tested. |
|
Though also, I was just reporting that as the workflow I used for to test locally, any workflow with GEM could be used to test. |
Ok! Then I guess the standard matrix tests should be fine. |
|
test parameters:
|
|
@cmsbuild please test |
|
+alca
|
|
After discussion among the AlCaDB conveners, we believe that the back-ward compatibility of the old GTs including the concerned classes should be preserved, irrespectively from the status of the underlying consuming code. |
|
-1 |
|
-alca |
@ggovi, @francescobrivio |
|
Which sort of backwards compatibility is this preserving?
On Apr 28, 2021 6:14 PM, ggovi ***@***.***> wrote:
After discussion among the AlCaDB conveners, we believe that the back-ward compatibility of the old GTs including the concerned classes should be preserved, irrespectively from the status of the underlying consuming code.
Therefore, we propose to leave unchanged the classes. A discussion focused on how to properly document the obsolete classes can follow, involving the CMSSW release experts...
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub<#33524 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABGPFQ24775MNPINID5C7D3TLAX6FANCNFSM43SQL7SA>.
|
After you remove the record you can't read the old GT (with the payloads associated to the removed record) in the new release. What bothers me is how the elected records to be preserved are chosen. |
|
Eg the gt name has to change in a development cycle? Seems a low price to pay (as opposed to keeping this code around for 15 years?)
On Apr 28, 2021 9:00 PM, Marco Musich ***@***.***> wrote:
Which sort of backwards compatibility is this preserving?
After you remove the record you can't read the old GT (with the payloads associated to the removed record) in the new release. What bothers me is how the elected records to be preserved are chosen.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<#33524 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABGPFQ7273N3MOXOHNWWMWLTLBLMTANCNFSM43SQL7SA>.
|
E.g. the very same configuration that run until some point, becomes broken overnight and to fix it you need to change GlobalTag and buy with it a lot of other potential changes. Seems quite an high price to me. |
|
I don't get what mean by lots of other changes?
On Apr 28, 2021 9:42 PM, Marco Musich ***@***.***> wrote:
Eg the gt name has to change in a development cycle? Seems a low price to pay (as opposed to keeping this code around for 15 years?)
E.g. the very same configuration what run until some point it becomes broken overnight and to fix it you need to change GlobalTag and buy with it a lot of other potential changes. Seems quite an high price to me.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<#33524 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABGPFQ7P52QN3F7GA4Y3DH3TLBQJPANCNFSM43SQL7SA>.
|
|
Or why autocond doesn't solve this completely? (Alva seems to be regularly changing gt in the tier0 so hardwired gt names aren't easy to maintain regardless)
On Apr 28, 2021 10:05 PM, David Lange ***@***.***> wrote:
I don't get what mean by lots of other changes?
On Apr 28, 2021 9:42 PM, Marco Musich ***@***.***> wrote:
Eg the gt name has to change in a development cycle? Seems a low price to pay (as opposed to keeping this code around for 15 years?)
E.g. the very same configuration what run until some point it becomes broken overnight and to fix it you need to change GlobalTag and buy with it a lot of other potential changes. Seems quite an high price to me.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<#33524 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABGPFQ7P52QN3F7GA4Y3DH3TLBQJPANCNFSM43SQL7SA>.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub<#33524 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABGPFQ6UOZTDO6SUYM47TWTTLBS7RANCNFSM43SQL7SA>.
|
Let's say I had an offline data Global Tag cut in 10.6.X (before any ultra-legacy changes) that worked perfectly fine for my application. Now I need to move to a new offline data Global Tag cut in 12.0.X if I want to test my application in a recent release.
So, overnight I have a lot of spurious changes and I can't really reproduce my original output in the new release, unless I painfully reconstruct the same tag <-> records association + the right snapshot.` |
autoCond is not the solution, when you want to keep using the same old GlobalTag... |
|
So the usecase is using new software and old gt. (eg one wants changes from others in code but doesn't want them in conditions). Ok thanks.
On Apr 28, 2021 10:14 PM, Marco Musich ***@***.***> wrote:
Or why autocond doesn't solve this completely? (Alva seems to be regularly changing gt in the tier0 so hardwired gt names aren't easy to maintain regardless)
autoCond is not the solution, when you want to keep using the same old GlobalTag...
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<#33524 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABGPFQ745PTVZHYLIW5K5MLTLBUAXANCNFSM43SQL7SA>.
|
|
@mmusich As you might have noticed, #31729 did not get the db signature. My fault, certainly, since I was late... Anyhow, No 'record selection' is applied in the present action, and even less in the policy applied. It is simply matter of efficiency in spotting the cases and treat them accordingly, especially taking into account that the policy has been discussed explicitly within AlcaDB only yesterday for the first time. |
Shall we assume that from now on, no further deletion of existing records will be allowed? |
I confirm that this is the intended policy that has been proposed, to support the functionality of pre-existing GTs in a given release. In addition, removal of persistent CondFormats classes is dangerous since it allows to re-introduce, at some point, a (potentially) different class with the same name, which might then cause crashes at payload access time... |
I think that this policy is the safest one, as nicely explained by @ggovi in #33524 (comment). |
|
On Apr 29, 2021, at 9:58 AM, francescobrivio ***@***.***> wrote:
Shall we assume that from now on, no further deletion of existing records will be allowed?
I think that this policy is the safest one, as nicely explained by @ggovi in #33524 (comment).
Shall we make it more official and include it in CMSSW rules somewhere? Because if we keep it as a rule only inside AlCaDB we might have the same conversation next time this happens (hopefully not soon!)
Please include also alca’s plan for supporting any code that ends up in this situation in your proposed rule.
…
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
I've added an item to discuss about this tomorrow at the ORP meeting. @ggovi is there a way to check automatically this new rule? I'm wondering about a unit test that try to read the old global tags used in the main past productions to check if CMSSW is actually able to read the CondFormat. |
We could test all of the GTs in the autocond. And we may discover that some of the GTs are actually failing. To make the policy more reasonable, we could use a release-based list of 'removed records' to relax the check we do at GT initialisation ( the one that currently causes the error ). |
PR description:
Remove GEMELMap and related classes (GEMELMapRcd, GEMROMap) which were used for storing the electronics map for GEM before the final emap classes were made (the classes that replace the ones deleted here are GEMeMap and GEMROMapping). The GEMELMapRcd was recently removed from AlCaDB #33507 so there should be no more references to these classes.
@jshlee
@hyunyong
PR validation:
Checked the commit with the April 18 and 25 nightlies, running upgrade workflow 34261.0. The April 18th nightly (before Rcd removal) fails due to trying to load GEMELMapRcd from the CondDB, while the April 25th nightly succeeds.
if this PR is a backport please specify the original PR and why you need to backport that PR:
Before submitting your pull requests, make sure you followed this checklist: