increase version number of PackedTrack to reflect change in the definition of LHCbID

Workarounds have been added to convert the LHCbIDs in the tracks for older data to the new scheme.

requested review from @rmatev and @jonrob

shall we start a test to see what happens to the tests? (ideally, nothing as-is ;-)

added RTA label

@graven Its been a while since I looked at these files, but there seems to be some inconsistency with the versions. Why is the 'default' still version 5

https://gitlab.cern.ch/lhcb/LHCb/-/blob/9263a0dc12ce70866a31d97272696d505b4cd0ae/Event/EventPacker/include/Event/PackedTrack.h#L169

but... then in the cpp file the packing versions hardcoded to 6

https://gitlab.cern.ch/lhcb/LHCb/-/blob/9263a0dc12ce70866a31d97272696d505b4cd0ae/Event/EventPacker/src/lib/PackedTrack.cpp#L19

https://gitlab.cern.ch/lhcb/LHCb/-/blob/9263a0dc12ce70866a31d97272696d505b4cd0ae/Event/EventPacker/src/lib/PackedTrack.cpp#L54

Also, the fact that the value 6 is hardcoded in two places is not great.

Why wasn't the defaultPackingVersion() method updated to return 6, and then use this method in the cpp, rather than hard-code a different version again, in two places ?

@graven Another observation. Setting the packing version at all in the various TrackPacker::pack methods seems a bit strange to me. It also leads to these pointless lines of code

  ptracks.setVersion( 6 );
  const auto ver = ptracks.version();
  if ( !isSupportedVer( ver ) ) return;

because of the first line ver is always 6... checking it is supported is then rather redundant.

It looks to me like adding the hardcoding of the version in, the TrackPacker::pack methods is something that has been 'recently' added. The idea, from run2, was the packers do not set this version but just use what they have been told to use from the input container, and this version was handled at the algorithm level, not the packer level.

https://gitlab.cern.ch/lhcb/LHCb/-/blob/9263a0dc12ce70866a31d97272696d505b4cd0ae/Event/EventPacker/src/component/PackerBaseAlg.h#L76

thats the 'natural' place to define the version, by the component creating the packed container, not I would say the packer itself. It should just check that the version it is requested to use is one it can support (which is how it previously was).

added 1 commit

• a6980c0a - increase version number of PackedTrack to reflect change in the definition of LHCbID

Compare with previous version

added 1 commit

• e0ad8ca5 - increase version number of PackedTrack to reflect change in the definition of LHCbID

Compare with previous version

added 1 commit

• 6a68c807 - increase version number of PackedTrack to reflect change in the definition of LHCbID

Compare with previous version

added 1 commit

• 1b86828b - increase version number of PackedTrack to reflect change in the definition of LHCbID

Compare with previous version

Thanks, things look reasonable to me now so lets see what happens via a test.

/ci-test

added ci-test-triggered label

• [2022-10-10 14:36] Validation started with lhcb-master-mr#5872
• [2022-10-10 17:21] Validation started with lhcb-master-mr#5875

So @graven, if it turns out explicitly dropping support for the older versions causes too much pain, adding support for adapting to the changes in !3226 (merged) really would not be hard at all. Basically, all that is needed I think is to form a map between the old and new channelIDtype type values, using a local definition of the values before the change

    enum class channelIDtype { Velo = 1, TT, IT, OT, Rich, Calo, Muon, VP, FT = 10, UT, HC };

and to then use that to fix up on the fly the values read in to the new ordering...

mentioned in issue #263

added 2 commits

• cc6b9716 - LHCbID : Add explict 'UNDEFINED' channelID type
• b2a4e2db - TrackPacker : Add backwards compatibility workaround for old channelIDtype ordering

Compare with previous version

@graven I've pushed an update, but I am starting to suspect the reason the current master version has lines like

ptracks.setVersion( 5 );

in the packing methods is precisely because the wrong version is subsequently being used to check the packing version

const auto ver = ptracks.version();

My suspicions is someone ran into this, and then 'fixed' it incorrectly.

The bottom line is I think removing these lines is going to cause issues, so we probably have to fix the code to use the right version for the packing checks at the same time here (i.e. we need to address #263).

added 1 commit

• cb02b141 - TrackPacker: Explicitly save packing version to stream buffer, and use correct...

Compare with previous version

/ci-test

• [2022-10-10 17:41] Validation started with lhcb-master-mr#5876

added hlt2-throughput-decreased label

added hlt1-throughput-decreased label

removed hlt2-throughput-decreased label

removed hlt1-throughput-decreased label

So, a quick update as I have been looking into the issues here.

Basically, the issue affecting the LHCb test failure boils down to the inconsistent (and at times incorrect) way we handled the track packing versioning during run2. In fact, it you check the run2-patches branch the track packer there does not have an explicit packingVersion - The code was (ab)using the DataObject version for this, which explains some of the things we see in master.

The problem though is if you read old data, from before the packingVersion was added, these files do not have this and thus the ROOT schema evolving (correctly) just assigns it the default value. The problem is the default is the newest packing version, which of course is wrong for these data and thus if you then use this to make decisions on what to do during the unpacking, things go wrong.

I need to come up with some logic we can use to detect this and react accordingly, whilst still for 'new' data correctly use the packingVersion during the packing and unpacking steps.

One saving grace is we of course plan to migrate completely away from the old event model, and thus with the new one we have a clean slate w.r.t. the versioning...

added 1 commit

• d2e7fa76 - PackedTracks: Add work arounds for historical 9ab)use of DataObject version as packing version

Compare with previous version

/ci-test

changed the description

• [2022-10-11 14:06] Validation started with lhcb-master-mr#5885
• [2022-10-11 15:46] Validation started with lhcb-master-mr#5889

added Tracking needs ref update labels

added 1 commit

• 8e3e47cb - LHCbID: Explicitly set values for all fields in channelIDtype

Compare with previous version

added 1 commit

• 6492a1d5 - TrackPacker: Resort LHCbIDs ater correcting to new labelling scheme

Compare with previous version

/ci-test

added 1 commit

• cafefc26 - TrackPacker: Resort LHCbIDs ater correcting to new labelling scheme

Compare with previous version

added 1 commit

• 9de1e603 - TrackPacker: Resort LHCbIDs after correcting to new labelling scheme

Compare with previous version

added 4 commits

• aec7c09a - PackedTracks: Add work arounds for historical (ab)use of DataObject version as packing version
• a36afbc6 - LHCbID: Explicitly set values for all fields in channelIDtype
• e6ace22b - TrackPacker: Resort LHCbIDs after correcting to new labelling scheme
• 3c2dfe94 - RecVertex_v2: Default initialise WeightedTrack struct members

Compare with previous version

added 1 commit

• 566e4ad6 - RecVertex_v2: Default initialise WeightedTrack struct members

Compare with previous version

mentioned in merge request Moore!1817 (merged)

added 2 commits

• 5ea6acaf - Buffer Packers: Print checksums in hex format
• e144c5eb - Fix PackedTrack packing version in all unpack calls

Compare with previous version

added 1 commit

• 932413af - Fixed formatting

Compare with previous version

added 2 commits

• 86e7c211 - Fix PackedTrack packing version in all unpack calls
• 0c61a7fa - Buffer Packers: Print checksums in hex format

Compare with previous version

added 16 commits

• 0c61a7fa...c3064823 - 6 commits from branch master
• 8d1e7f40 - increase version number of PackedTrack to reflect change in the definition of LHCbID
• f3675d62 - LHCbID : Add explict 'UNDEFINED' channelID type
• 7f377b9a - TrackPacker : Add backwards compatibility workaround for old channelIDtype ordering
• 68b16707 - TrackPacker: Explicitly save packing version to stream buffer, and use correct...
• 91297372 - PackedTracks: Add work arounds for historical (ab)use of DataObject version as packing version
• 7dd68c2b - LHCbID: Explicitly set values for all fields in channelIDtype
• f0152ed5 - TrackPacker: Resort LHCbIDs after correcting to new labelling scheme
• 3d0f336a - RecVertex_v2: Default initialise WeightedTrack struct members
• 8c6b7eb6 - Fix PackedTrack packing version in all unpack calls
• 96671d32 - Buffer Packers: Print checksums in hex format

Compare with previous version

/ci-test --merge Moore!1817 (merged)

added Persistency label