-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
edm::OwnVector is deprecated #42734
Comments
assign core |
New categories assigned: core @Dr15Jones,@smuzaffar,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks |
A new Issue was created by @makortel Matti Kortelainen. @Dr15Jones, @rappoccio, @smuzaffar, @makortel, @sextonkennedy, @antoniovilela can you please review it and eventually sign/assign? Thanks. cms-bot commands are listed here |
1. Can be acted on immediately (nothing is persisted)Clearly unused, can be cleaned up
Looks unused, can likely be cleaned up
To be migrated to
|
2. Could be acted on now, but likely causes backwards incompatibilities
The The |
3. Dynamic polymorphism is used, need a new solution, backwards incompatibilities pretty much guaranteed
DiscussionWe could develop an If we proceed with the |
4. Fireworks uses many of the collectionsSome (many?) of the classes described in 2 and 3 were used in Fireworks. I'm not sure Fireworks needs any specific treatment beyond the migration described in 2 and 3. Backwards compatibility likely breaks in both cases. |
assign reconstruction,fastsim,xpog,visualization FYI @cms-sw/trk-dpg-l2 @cms-sw/tracking-pog-l2 @cms-sw/muon-dpg-l2 @cms-sw/muon-pog-l2 @cms-sw/btv-pog-l2 @cms-sw/pf-l2 |
New categories assigned: fastsim,xpog,reconstruction,visualization @mdhildreth,@jfernan2,@sbein,@ssekmen,@Dr15Jones,@simonepigazzini,@makortel,@mandrenguyen,@alja,@vlimant,@civanch you have been requested to review this Pull request/Issue and eventually sign? Thanks |
For the cases listed in #42734 (comment) (migrating bunch of cases from |
A major downside to this design is it inverts the code coupling. Presently, packages using the base class only have to be dependent upon the package with the base class. Using a Additionally, adding a new type that inherits from the base class means updating all uses of |
Just to remind that
|
Thanks Josh. Is the reading of a MINIAOD file produced on an earlier release cycle in a newer release cycle something that might become a requirement, or is it just hoped to work? (since strictly speaking backwards compatibility is guaranteed only for RAW) |
it is something that we rely on quite heavily to reduce the number of times we have to start from AOD (or even earlier) in order to produce new NANO |
And this is really across release cycles, rather than backporting the necessary changes to the release cycle where the MiniAOD was produced? |
indeed. For two reason (as far as I can see):
All this being said, if the change is necessary we can workout the right time to do it to minimize the need of accessing older MINIAODs. In general this will require re-making certain MINIAODs campaign starting from AOD in a release in which the OwnVector has been removed. The main challenge I see here is with Run2 MINIAODs. It is by far easier to read them as they are to make new NANOAODs rather than either backport changes or reproduce all of them starting from AOD. |
The same comment holds true for almost all Tracker alignment and calibration ALCARECO flavours. |
assign alca
|
New categories assigned: alca @perrotta,@consuegs,@saumyaphor4252 you have been requested to review this Pull request/Issue and eventually sign? Thanks |
Thanks @mmusich for bringing this point up. Could you (or @cms-sw/alca-l2) elaborate why the same release cycle that was used to produce the ALCARECOs could not be used (or would be difficult to use) to derive these calibrations? |
A variety of reasons, but mostly these two:
|
The only drawback I see for if one knows that At least for track/tracking the design is closed (the base class supports poor-man RTTI for fast down-casting) and most of the derived classes are in a single Data Format package I hope root will support "schema evolution" in case one adds/deletes a type from a variant.
|
Are the aforementioned expectations of MINIAOD and (some?) ALCARECOs to be readable by later release cycles being tested in the IBs? |
Are we 100 % sure the same classes (like |
Yes, e.g.
Alignment/OfflineValidation ).
|
Looking at 2023 prompt reco, the following collections seem to be used in MINIAOD
and the following in AOD (in addition to the ones in MINIAOD)
so already the migration of (half of) the classes in category 2 (#42734 (comment)) would run into conflict with the aforementioned backwards compatibility expectations. @cms-sw/xpog-l2 From the |
Hi @makortel, an in person discussion would indeed be great. Our XPOG meeting on Wednesday 2pm is usually not too busy, we can accommodate a slot for you in an upcoming one (next one is Oct. 2nd). |
Unfortunately next week (Oct 4) conflicts with O&C week. In the mean time @Dr15Jones prototyped the migration from the |
Hi @makortel, the next meeting is going to be on the 18th at 2pm GVA time.
Based on these two points we can either discuss in person at the meeting on the 18th (in case substantial effort will be required by POGs) otherwise we can iterate here and in the relevant PRs. |
I suspect that most of the |
This already exists via the use of |
ah ok excellent. Thought it was necessary to introduce a "translator" producer. |
How far back is there practical value in maintaining backward compatibility? Is 10_6_X the oldest release which might have been used to create input files for future processes creating NANO from MINIAOD or doing whatever it is that ALCA will be doing or other similar use cases? Is there some point where this backwards compatibility is already broken? I ask this understanding that there has always been a hard requirement to be able to read RAW from some very early point in CMS history. |
Correct, we guarantee backwards compatibility for RAW (that also includes Scouting). And formally, that has been the only backwards compatibility guarantee, and for other data types the backwards compatibility has been on best effort basis (although in practice the schema evolution has worked quite widely).
The scope we have planned so far are creation of NanoAOD(SIM) / MiniAOD(SIM) from MiniAOD(SIM) / AOD(SIM) that were produced in the Run 2 Ultra Legacy processing (done in 10_6_X) in official processing campaigns, and reading of ALCARECOs produced in the same Run 2 UL processing in the scope that those are being read in CMSSW tests.
In principle no, because both the Mini/AOD case and ALCARECO case should be being tested in CMSSW. In practice, at least one subtle issue had already creeped in (#43923), one can always worry about the coverage of the tests. |
PR #43931 deals with the "Clearly unused" cases. |
PR #43987 takes care of |
PR #44047 deals with |
PR #44063 deals with |
ROOT's new
RNTuple
columnar data storage is not going to support dynamic polymorphism (as opposed toTTree
). One such use case in our data formats isedm::OwnVector<T>
that effectively behaves asstd::vector<std::unique_ptr<T>>
, including the dynamic polymorphism (i.e. can store owning pointers to objects of any class that inherits fromT
). The purpose of this issue is to list all the current uses ofedm::OwnVector
, discuss how to address them, and track the progress.It should be noted than the deployment of the migration needs some thought, as the changes are very likely not backwards compatible (although, strictly speaking, the backwards compatibility of
edm::OwnVector
is not guaranteed across CMSSW release cycles).I expect the framework group to do majority of the work, but help from domain experts would also be very welcome.
The text was updated successfully, but these errors were encountered: