Add edmDumpClassVersion and checkDictionaryUpdate.py scripts to check class version and checksum updates #45423

makortel · 2024-07-10T14:21:58Z

PR description:

This PR is a work-in-progress prototype of a edmDumpClassVersion script to print class versions and checksums of all (non-transient) classes defined in a classes_def.xml file. The script would be used in automated assignment of changes-dataformats label discussed in cms-sw/cms-bot#2245.

The command-line interface is similar to edmCheckClassVersion with the -x / --xml_file and -l / --lib parameters.

Resolves cms-sw/framework-team#947

PR validation:

Tested the output on DataFormats/Common/src/classes_def.xml.

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

Potentially to be backported to 14_0_X.

cmsbuild · 2024-07-10T14:22:20Z

cms-bot internal usage

cmsbuild · 2024-07-10T14:24:59Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45423/40873

cmsbuild · 2024-07-10T14:25:21Z

A new Pull Request was created by @makortel for master.

It involves the following packages:

FWCore/Utilities (core)

@Dr15Jones, @cmsbuild, @makortel, @smuzaffar can you please review it and eventually sign? Thanks.
@felicepantaleo, @missirol, @wddgit this is something you requested to watch as well.
@antoniovilela, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

makortel

I think FWCore/Utilities is not the best package for these utilities (mostly because of the dependence on ROOT).

Maybe FWCore/Reflection would be better? (although I feel it would not be a perfect match either)

makortel · 2024-07-10T14:22:53Z

FWCore/Utilities/python/ClassesDefXmlUtils.py

I moved the code from edmCheckClassVersion that I wanted to re-use into this file (with a few changes that I'll note below).

There is enough of somewhat complicated code shared between edmCheckClassVersion and edmDumpClassVersion that I'd prefer to have the edmDumpClassVersion in CMSSW rather than in cms-bot (which then implies the need to backport to 14_0_X).

makortel · 2024-07-10T14:23:59Z

FWCore/Utilities/python/ClassesDefXmlUtils.py

+    classVersionIndex=1
+    versionsToChecksumIndex = 2
+
+    def __init__(self, filename, includeNonVersionedClasses=False, normalizeClassNames=True):


The includeNonVersionedClasses and normalizeClassNames are new options for this class. The default values are those used by edmCheckClassVersion (I could make that script to pass the values also explicitly). The edmDumpClassVersion uses opposite values.

makortel · 2024-07-10T14:26:41Z

FWCore/Utilities/scripts/edmCheckClassVersion

-from sys import version_info
-if version_info[0] > 2:
-    atol = int
-else:
-    from string import atol


I just replaced all atol() calls with int().

FWCore/Utilities/scripts/edmCheckClassVersion

FWCore/Utilities/scripts/edmDumpClassVersion

makortel · 2024-07-10T14:39:05Z

The present printout is along

edm::WrapperBase 4 3914746810
edm::EDProductGetter 11 2247759761
edm::RefCore 11 3425324092
edm::RefCoreWithIndex 11 2602044085
edm::ThinnedAssociation 10 1914950449
edm::Wrapper<edm::ThinnedAssociation> 4 2054679015

If we proceed by the bot prints the checksums of the baseline and of the baseline+PR for every PR test, this format might be sufficient (simple diff should be sufficient to tell if there are changes; in this case the output would probably be good to be sorted to avoid false positives from simple reordering of things in classes_def.xml).

If we proceed by having a storing the versions and checksums for all data format classes_def.xml files, the output would be better as something more machine-readable, e.g. JSON.

makortel · 2024-07-11T20:13:47Z

@smuzaffar What way do you think would make most sense to use the version+checksum information in the PR tests?

smuzaffar · 2024-07-12T09:06:38Z

@makortel , I am think something like the following

PR tests can generate and upload the version+checksum ref if not already available. Ref should be stored in one file per cmssw package
PR tests can generate a local version+checksum for the packages changed by PR . If a package is dropped/removed in PR then it should still generate an empty file.
Run a simple diff and report in PR summary that there are dataformat changes
Once PR tests are completed then bot can add the changes-dataformats label if needed. This label can be removed by L2 (using type -changes-dataformats comment) if it is false positive

Note that this will be done during PR tests and not during code-checks (we do not want to build/run anything during self started code-checks tests)

makortel · 2024-07-12T14:20:53Z

@smuzaffar Ok, sounds good. Do I understand correctly that a simple text format (like presently in this PR) would be sufficient?

What about packages that have multiple classes_def.xml files? I'd (slightly) prefer this script to operate on one classes_def.xml file at a time, so would the bot then e.g. concatenate+sort the output of all classes_def.xml files into one file per package?

smuzaffar · 2024-07-12T14:26:44Z

@makortel , for few classes_def.xml files edmDumpClassVersion failed with error like [a]. For packages which it failed are [b].

[a]

Traceback (most recent call last):
  File "/build/muz/cv/CMSSW_14_1_X_2024-07-11-2300/bin/el8_amd64_gcc12/edmDumpClassVersion", line 34, in <module>
    main(args)
  File "/build/muz/cv/CMSSW_14_1_X_2024-07-11-2300/bin/el8_amd64_gcc12/edmDumpClassVersion", line 22, in main
    (error, version, checksum) = ClassesDefUtils.checkClass(name, 0, {})
  File "/build/muz/cv/CMSSW_14_1_X_2024-07-11-2300/src/FWCore/Utilities/python/ClassesDefXmlUtils.py", line 95, in checkClass
    raise RuntimeError("failed to load dictionary for class '"+name+"'")
RuntimeError: failed to load dictionary for class 'std::pair<LocalVector,LocalVector>'

[b]

edmDumpClassVersion -l libAnalysisDataFormatsTrackInfo.so -x /cvmfs/cms-ib.cern.ch/sw/x86_64/week1/el8_amd64_gcc12/cms/cmssw-patch/CMSSW_14_1_X_2024-07-11-2300/src/AnalysisDataFormats/TrackInfo/src/classes_def.xml
edmDumpClassVersion -l libCondFormatsHcalObjects.so -x /cvmfs/cms-ib.cern.ch/sw/x86_64/week1/el8_amd64_gcc12/cms/cmssw-patch/CMSSW_14_1_X_2024-07-11-2300/src/CondFormats/HcalObjects/src/classes_def.xml
edmDumpClassVersion -l libDataFormatsEcalRecHitROCmAsync.so -x /cvmfs/cms-ib.cern.ch/sw/x86_64/week1/el8_amd64_gcc12/cms/cmssw-patch/CMSSW_14_1_X_2024-07-11-2300/src/DataFormats/EcalRecHit/src/alpaka/classes_rocm_def.xml
edmDumpClassVersion -l libDataFormatsHGCRecHit.so -x /cvmfs/cms-ib.cern.ch/sw/x86_64/week1/el8_amd64_gcc12/cms/cmssw-patch/CMSSW_14_1_X_2024-07-11-2300/src/DataFormats/HGCRecHit/src/classes_def.xml
edmDumpClassVersion -l libDataFormatsL1Trigger.so -x /cvmfs/cms-ib.cern.ch/sw/x86_64/week1/el8_amd64_gcc12/cms/cmssw-patch/CMSSW_14_1_X_2024-07-11-2300/src/DataFormats/L1Trigger/src/classes_def.xml
edmDumpClassVersion -l libDataFormatsTauReco.so -x /cvmfs/cms-ib.cern.ch/sw/x86_64/week1/el8_amd64_gcc12/cms/cmssw-patch/CMSSW_14_1_X_2024-07-11-2300/src/DataFormats/TauReco/src/classes_def_3.xml
edmDumpClassVersion -l libDataFormatsTrackSoAROCmAsync.so -x /cvmfs/cms-ib.cern.ch/sw/x86_64/week1/el8_amd64_gcc12/cms/cmssw-patch/CMSSW_14_1_X_2024-07-11-2300/src/DataFormats/TrackSoA/src/alpaka/classes_rocm_def.xml
edmDumpClassVersion -l libDataFormatsTrackingRecHitSoAROCmAsync.so -x /cvmfs/cms-ib.cern.ch/sw/x86_64/week1/el8_amd64_gcc12/cms/cmssw-patch/CMSSW_14_1_X_2024-07-11-2300/src/DataFormats/TrackingRecHitSoA/src/alpaka/classes_rocm_def.xml
edmDumpClassVersion -l libFireworksCore.so -x /cvmfs/cms-ib.cern.ch/sw/x86_64/week1/el8_amd64_gcc12/cms/cmssw-patch/CMSSW_14_1_X_2024-07-11-2300/src/Fireworks/Core/src/classes_def.xml
edmDumpClassVersion -l libOnlineDBEcalCondDB.so -x /cvmfs/cms-ib.cern.ch/sw/x86_64/week1/el8_amd64_gcc12/cms/cmssw-patch/CMSSW_14_1_X_2024-07-11-2300/src/OnlineDB/EcalCondDB/src/classes_def.xml
edmDumpClassVersion -l libSimGeneralTrackingAnalysis.so -x /cvmfs/cms-ib.cern.ch/sw/x86_64/week1/el8_amd64_gcc12/cms/cmssw-patch/CMSSW_14_1_X_2024-07-11-2300/src/SimGeneral/TrackingAnalysis/src/classes_def.xml
edmDumpClassVersion -l libSimTrackerTrackTriggerAssociation.so -x /cvmfs/cms-ib.cern.ch/sw/x86_64/week1/el8_amd64_gcc12/cms/cmssw-patch/CMSSW_14_1_X_2024-07-11-2300/src/SimTracker/TrackTriggerAssociation/src/classes_def.xml

smuzaffar · 2024-07-12T14:34:06Z

@smuzaffar Ok, sounds good. Do I understand correctly that a simple text format (like presently in this PR) would be sufficient?

If I understand correctly, we do want to flag in case of any change in dataformat i.e. any new dict, removal of existing dict or change in checksum ... right? If yes then simple diff with this text format should be enough.

What about packages that have multiple classes_def.xml files? I'd (slightly) prefer this script to operate on one classes_def.xml file at a time, so would the bot then e.g. concatenate+sort the output of all classes_def.xml files into one file per package?

yes, bot will run single process per xml file but it will run nproc such process in parallel ( to speed up the processing). At the end it will concatenate+sort all the xmls for each package in its package file ( i.e. we will have one version-checksum file per package). Bot then can compare PR packages files with release packages file

smuzaffar · 2024-07-12T14:37:07Z

@makortel , I also see some classes with -1 as version [a]. Is this expected ?

[a]

> edmDumpClassVersion -l libCondFormatsHcalObjects.so -x /cvmfs/cms-ib.cern.ch/sw/x86_64/week1/el8_amd64_gcc12/cms/cmssw-patch/CMSSW_14_1_X_2024-07-11-2300/src/CondFormats/HcalObjects/src/classes_def.xml
....
...
HcalCondObjectContainer<HcalLutMetadatum> -1 3488635253
HcalLutMetadata -1 1184932721
HcalLutMetadata::NonChannelData -1 1765044631
HcalDcsValue -1 3257269307

makortel · 2024-07-12T14:42:04Z

@smuzaffar Ok, sounds good. Do I understand correctly that a simple text format (like presently in this PR) would be sufficient?

If I understand correctly, we do want to flag in case of any change in dataformat i.e. any new dict, removal of existing dict or change in checksum ... right?

Right, this is my understanding as well.

If yes then simple diff with this text format should be enough.

Ok, good. I'll then update the PR with the earlier review suggestions.

What about packages that have multiple classes_def.xml files? I'd (slightly) prefer this script to operate on one classes_def.xml file at a time, so would the bot then e.g. concatenate+sort the output of all classes_def.xml files into one file per package?

yes, bot will run single process per xml file but it will run nproc such process in parallel ( to speed up the processing). At the end it will concatenate+sort all the xmls for each package in its package file ( i.e. we will have one version-checksum file per package). Bot then can compare PR packages files with release packages file

Sounds good.

makortel · 2024-07-12T14:48:02Z

I also see some classes with -1 as version [a]. Is this expected ?

I have no idea. I'll tag @pcanal in the hope we'd get an answer when he gets back.

makortel · 2024-07-12T14:54:18Z

for few classes_def.xml files edmDumpClassVersion failed with error like [a]

Thanks, I'll take a look of these as well.

smuzaffar · 2024-07-12T14:54:48Z

What about packages that have multiple classes_def.xml files?

yes bot will process all the xml files of the package. Also it will process any addition alpaka backend xml files

edmDumpClassVersion -l libDataFormatsStdDictionaries.so -x DataFormats/StdDictionaries/src/classes_def_vector.xml
edmDumpClassVersion -l libDataFormatsStdDictionaries.so -x DataFormats/StdDictionaries/src/classes_def_pair.xml
edmDumpClassVersion -l libDataFormatsStdDictionaries.so -x DataFormats/StdDictionaries/src/classes_def_others.xml
edmDumpClassVersion -l libDataFormatsStdDictionaries.so -x DataFormats/StdDictionaries/src/classes_def_map.xml
....
edmDumpClassVersion -l libDataFormatsBeamSpot.so -x DataFormats/BeamSpot/src/classes_def.xml
edmDumpClassVersion -l libDataFormatsBeamSpotCudaAsync.so -x DataFormats/BeamSpot/src/alpaka/classes_cuda_def.xml
edmDumpClassVersion -l libDataFormatsBeamSpotROCmAsync.so -x DataFormats/BeamSpot/src/alpaka/classes_rocm_def.xml

davidlange6 · 2024-07-12T14:56:13Z

why would you expect a class version != -1 for these?

cmssw/CondFormats/HcalObjects/src/classes_def.xml

Line 307 in c103a34

smuzaffar · 2024-07-23T18:21:33Z

@makortel , all look good

makortel · 2024-07-23T18:42:06Z

+core

cmsbuild · 2024-07-23T18:42:30Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @sextonkennedy, @mandrenguyen, @rappoccio, @antoniovilela (and backports should be raised in the release meeting by the corresponding L2)

mandrenguyen · 2024-07-26T11:42:24Z

@makortel
Would you please resolve the open discussion threads, just to confirm that this is ready to merged?

makortel · 2024-07-26T13:53:18Z

Would you please resolve the open discussion threads, just to confirm that this is ready to merged?

I'd actually prefer to leave them open. 3 of them are my own comments about why some things were done (and there is nothing to react), and 1 is a question/suggestion for @smuzaffar (for which there is nothing to react in this PR). I feel all of them are more useful to be left open than closed for anyone looking at this PR in the future.

mandrenguyen · 2024-07-26T14:06:59Z

Would you please resolve the open discussion threads, just to confirm that this is ready to merged?

I'd actually prefer to leave them open. 3 of them are my own comments about why some things were done (and there is nothing to react), and 1 is a question/suggestion for @smuzaffar (for which there is nothing to react in this PR). I feel all of them are more useful to be left open than closed for anyone looking at this PR in the future.

Fair enough. But just to confirm, these are left open for the future, but the PR itself is ready to be merged (as the signatures already indicate)?

makortel · 2024-07-26T14:10:13Z

But just to confirm, these are left open for the future, but the PR itself is ready to be merged (as the signatures already indicate)?

Correct.

mandrenguyen · 2024-07-26T18:14:04Z

+1

makortel · 2024-07-26T20:07:57Z

Coming back to the classes_def.xml files the script was unable to fully digest (i.e. continuation of #45423 (comment))

AnalysisDataFormats/TrackInfo/src/classes_def.xml

Fixed in #45564

DataFormats/TauReco/src/classes_def_3.xml

I need to talk to @pcanal to understand better best practices with type aliases in classes_def.xml files.

SimGeneral/TrackingAnalysis/src/classes_def.xml
SimTracker/TrackTriggerAssociation/src/classes_def.xml

Fixed in #45567

CondFormats/HcalObjects/src/classes_def.xml

Fixed in #45553

Fireworks/Core/src/classes_def.xml

Fixed in #45546

OnlineDB/EcalCondDB/src/classes_def.xml

Fixed in #45545

DataFormats/EcalRecHit/src/alpaka/classes_rocm_def.xml
DataFormats/TrackSoA/src/alpaka/classes_rocm_def.xml
DataFormats/TrackingRecHitSoA/src/alpaka/classes_rocm_def.xml

I'll look into next week

DataFormats/HGCRecHit/src/classes_def.xml

Fixed in #45551

DataFormats/L1Trigger/src/classes_def.xml

Fixed in #45554

smuzaffar · 2024-07-29T20:17:19Z

yes, once this PR is merge then we can start running it for master IB production arch and report packages with policy checks failures

Ok. I'll add then a mode where the checkDictionaryUpdate.py can be run with --baseline only, and then I'd consider this PR complete (barring review feedback, of course).

@makortel , class versions results for IB/baseline are now available ( see the link on Ib page : https://cmssdt.cern.ch/SDT/jenkins-artifacts//class_versions/CMSSW_14_1_X_2024-07-28-2300/el8_amd64_gcc12/class_versions.html ). There are few errors e.g. there are errors like

Parsing [CondFormats/CastorObjects/src/classes_def.xml](https://github.com/cms-sw/cmssw/blob/CMSSW_14_1_X_2024-07-28-2300/CondFormats/CastorObjects/src/classes_def.xml) failed: There is an element 'class' without 'name' attribute.

makortel · 2024-07-29T20:27:38Z

class versions results for IB/baseline are now available ( see the link on Ib page : https://cmssdt.cern.ch/SDT/jenkins-artifacts//class_versions/CMSSW_14_1_X_2024-07-28-2300/el8_amd64_gcc12/class_versions.html ). There are few errors e.g. there are errors like

Thanks! I'll start taking a look. I see they all are about the classes_def.xml having <class pattern="...">. I vaguely recall a campaign to remove those some (many?) years ago.

makortel added 2 commits July 9, 2024 23:37

Replace atol with int

1556458

Add edmDumpClassVersion

e35c790

cmsbuild added this to the CMSSW_14_1_X milestone Jul 10, 2024

cmsbuild added core-pending pending-signatures tests-pending orp-pending code-checks-pending labels Jul 10, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 10, 2024

makortel commented Jul 10, 2024

View reviewed changes

Dr15Jones reviewed Jul 10, 2024

View reviewed changes

FWCore/Utilities/scripts/edmCheckClassVersion Outdated Show resolved Hide resolved

Dr15Jones reviewed Jul 10, 2024

View reviewed changes

FWCore/Utilities/scripts/edmDumpClassVersion Outdated Show resolved Hide resolved

makortel mentioned this pull request Jul 10, 2024

Add new label for DataFormat breaking PRs cms-sw/cms-bot#2245

Open

Use XmlParser via ClassesDefUtils in edmCheckClassVersion

58b6a52

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Jul 23, 2024

cmsbuild added orp-approved and removed orp-pending labels Jul 26, 2024

cmsbuild merged commit ba76dcb into cms-sw:master Jul 26, 2024
11 checks passed

makortel deleted the edmDumpClassVersion branch July 26, 2024 18:31

makortel mentioned this pull request Jul 26, 2024

Move dictionaries (and some classes) defined in SimTracker/TrackTriggerAssociation and SimGeneral/TrackingAnalysis to more proper packages #45567

Merged

cmsbuild mentioned this pull request Jul 27, 2024

First PyTorch tests for TorchScript inference CPU/CUDA #43475

Merged

makortel mentioned this pull request Jul 29, 2024

Remove edmAddClassVersion script as obsolete #45584

Merged

This was referenced Jul 31, 2024

Enforce policies on ROOT dictionaries cms-sw/framework-team#975

Open

Refactor edmCheckClassVersion #45610

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add edmDumpClassVersion and checkDictionaryUpdate.py scripts to check class version and checksum updates #45423

Add edmDumpClassVersion and checkDictionaryUpdate.py scripts to check class version and checksum updates #45423

makortel commented Jul 10, 2024 •

edited

Loading

cmsbuild commented Jul 10, 2024 •

edited

Loading

cmsbuild commented Jul 10, 2024

cmsbuild commented Jul 10, 2024

makortel left a comment

makortel Jul 10, 2024

makortel Jul 10, 2024

makortel Jul 10, 2024

makortel commented Jul 10, 2024

makortel commented Jul 11, 2024

smuzaffar commented Jul 12, 2024

makortel commented Jul 12, 2024

smuzaffar commented Jul 12, 2024

smuzaffar commented Jul 12, 2024 •

edited

Loading

smuzaffar commented Jul 12, 2024 •

edited

Loading

makortel commented Jul 12, 2024

makortel commented Jul 12, 2024

makortel commented Jul 12, 2024

smuzaffar commented Jul 12, 2024

davidlange6 commented Jul 12, 2024

smuzaffar commented Jul 23, 2024

makortel commented Jul 23, 2024

cmsbuild commented Jul 23, 2024

mandrenguyen commented Jul 26, 2024

makortel commented Jul 26, 2024

mandrenguyen commented Jul 26, 2024 •

edited

Loading

makortel commented Jul 26, 2024

mandrenguyen commented Jul 26, 2024

makortel commented Jul 26, 2024

smuzaffar commented Jul 29, 2024

makortel commented Jul 29, 2024

Add edmDumpClassVersion and checkDictionaryUpdate.py scripts to check class version and checksum updates #45423

Add edmDumpClassVersion and checkDictionaryUpdate.py scripts to check class version and checksum updates #45423

Conversation

makortel commented Jul 10, 2024 • edited Loading

PR description:

PR validation:

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

cmsbuild commented Jul 10, 2024 • edited Loading

cmsbuild commented Jul 10, 2024

cmsbuild commented Jul 10, 2024

makortel left a comment

Choose a reason for hiding this comment

makortel Jul 10, 2024

Choose a reason for hiding this comment

makortel Jul 10, 2024

Choose a reason for hiding this comment

makortel Jul 10, 2024

Choose a reason for hiding this comment

makortel commented Jul 10, 2024

makortel commented Jul 11, 2024

smuzaffar commented Jul 12, 2024

makortel commented Jul 12, 2024

smuzaffar commented Jul 12, 2024

smuzaffar commented Jul 12, 2024 • edited Loading

smuzaffar commented Jul 12, 2024 • edited Loading

makortel commented Jul 12, 2024

makortel commented Jul 12, 2024

makortel commented Jul 12, 2024

smuzaffar commented Jul 12, 2024

davidlange6 commented Jul 12, 2024

smuzaffar commented Jul 23, 2024

makortel commented Jul 23, 2024

cmsbuild commented Jul 23, 2024

mandrenguyen commented Jul 26, 2024

makortel commented Jul 26, 2024

mandrenguyen commented Jul 26, 2024 • edited Loading

makortel commented Jul 26, 2024

mandrenguyen commented Jul 26, 2024

makortel commented Jul 26, 2024

smuzaffar commented Jul 29, 2024

makortel commented Jul 29, 2024

makortel commented Jul 10, 2024 •

edited

Loading

cmsbuild commented Jul 10, 2024 •

edited

Loading

smuzaffar commented Jul 12, 2024 •

edited

Loading

smuzaffar commented Jul 12, 2024 •

edited

Loading

mandrenguyen commented Jul 26, 2024 •

edited

Loading