Add temporary failure element to Framework Job Report #41124

Dr15Jones · 2023-03-21T19:05:05Z

PR description:

Until the job report finishes, we add a FrameworkError element to denote that if the job suddenly ends the error will be present. When job completes successfully or with another error, that temporary element is not written.

The new error was added to edm::errors.

PR validation:

Code compiles and framework unit tests pass.

cmsbuild · 2023-03-21T19:13:10Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41124/34762

This PR adds an extra 36KB to repository

cmsbuild · 2023-03-21T19:13:37Z

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

FWCore/MessageLogger (core)
FWCore/MessageService (core)
FWCore/Utilities (core)
IOPool/Common (core)

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

cms-bot commands are listed here

Dr15Jones · 2023-03-21T19:23:12Z

please test

makortel · 2023-03-21T21:30:00Z

FWCore/MessageLogger/src/JobReport.cc

+  static char const* const kJobReportEndElement = "</FrameworkJobReport>\n";
+  static constexpr int kEndElementSize = 22;


Would it be possible and feasible to use std::string_view and take the number of elements from its size() (which, I see, should be constexpr)?

makortel · 2023-03-21T21:31:53Z

FWCore/MessageLogger/src/JobReport.cc

+      if ((endpos - pos) > kEndElementSize) {
+        //need to add some padding so use a comment element
+        auto padding = (endpos - pos) - (kEndElementSize + kMinSizeOfComment);
+        *(impl_->ost_) << "<!--";


Could you add a comment here explaining briefly why an XML comment is used instead of white space? (to remind us the next time we need to look at this code)

makortel · 2023-03-21T21:39:57Z

Not really part of this PR, but while we are here, I looked where temporarilyCloseXML() is called, and noticed that

cmssw/FWCore/MessageLogger/src/JobReport.cc

Lines 668 to 685 in 4c4bee9

    
           void JobReport::reportPerformanceSummary(std::string const& metricClass, 
        
                                                    std::map<std::string, std::string> const& metrics) { 
        
             if (impl_->ost_) { 
        
               std::ostream& msg = *(impl_->ost_); 
        
               msg << "<PerformanceReport>\n" 
        
                   << "  <PerformanceSummary Metric=\"" << metricClass << "\">\n"; 
        
               typedef std::map<std::string, std::string>::const_iterator const_iterator; 
        
               for (const_iterator iter = metrics.begin(), iterEnd = metrics.end(); iter != iterEnd; ++iter) { 
        
                 msg << "    <Metric Name=\"" << iter->first << "\" " 
        
                     << "Value=\"" << iter->second << "\"/>\n"; 
        
               } 
        
               msg << "  </PerformanceSummary>\n" 
        
                   << "</PerformanceReport>\n"; 
        
               temporarilyCloseXML(); 
        
             } 
        
           }

cmssw/FWCore/MessageLogger/src/JobReport.cc

Lines 687 to 706 in 4c4bee9

    
           void JobReport::reportPerformanceForModule(std::string const& metricClass, 
        
                                                      std::string const& moduleName, 
        
                                                      std::map<std::string, std::string> const& metrics) { 
        
             if (impl_->ost_) { 
        
               std::ostream& msg = *(impl_->ost_); 
        
               msg << "<PerformanceReport>\n" 
        
                   << "  <PerformanceModule Metric=\"" << metricClass << "\" " 
        
                   << " Module=\"" << moduleName << "\" >\n"; 
        
               typedef std::map<std::string, std::string>::const_iterator const_iterator; 
        
               for (const_iterator iter = metrics.begin(), iterEnd = metrics.end(); iter != iterEnd; ++iter) { 
        
                 msg << "    <Metric Name=\"" << iter->first << "\" " 
        
                     << "Value=\"" << iter->second << "\"/>\n"; 
        
               } 
        
               msg << "  </PerformanceModule>\n" 
        
                   << "</PerformanceReport>\n"; 
        
               temporarilyCloseXML(); 
        
             } 
        
           }

do not lock the mutex. Currently they are called only in ActivityRegistry's postEndJob signal that we seem to be calling serially. Should we nevertheless lock the mutex in these functions?

cmsbuild · 2023-03-22T01:52:15Z

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a928c2/31496/summary.html
COMMIT: fb249dd
CMSSW: CMSSW_13_1_X_2023-03-21-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/41124/31496/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

@perrotta Reduce non necessary sqrt computations in RecoTauTag/HLTProducers #41100

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a928c2/31496/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a928c2/31496/git-merge-result

Unit Tests

I found errors in the following unit tests:

---> test PrimaryVertex had ERRORS
---> test testPVPlotting had ERRORS
---> test trackerMaterialAnalysisPlots had ERRORS
---> test createDBObjecs had ERRORS
and more ...

Comparison Summary

Summary:

You potentially added 4 lines to the logs
Reco comparison results: 14 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3550915
DQMHistoTests: Total failures: 116
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3550777
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 213 log files, 164 edm output root files, 49 DQM output files
TriggerResults: no differences found

makortel · 2023-03-22T13:19:16Z

Another incarnation of #39803 and #39754

Until the job report finishes, we add a FrameworkError element to denote that if the job suddenly ends the error will be present. When job completes successfully or with another error, that temporary element is not written. The new error was added to edm::errors.

cmsbuild · 2023-03-23T16:39:20Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41124/34821

This PR adds an extra 16KB to repository

cmsbuild · 2023-03-23T16:39:49Z

Pull request #41124 was updated. @cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please check and sign again.

makortel · 2023-03-23T16:51:38Z

@cmsbuild, please test

makortel · 2023-03-23T16:53:34Z

Regarding #41124 (comment), for future reference, we decided not to add mutex there because the functions are (and should be) called only at endJob transition, and we do not foresee adding concurrency there.

cmsbuild · 2023-03-23T22:45:04Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a928c2/31569/summary.html
COMMIT: 243f7d4
CMSSW: CMSSW_13_1_X_2023-03-23-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/41124/31569/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 10 lines to the logs
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3552750
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3552728
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 213 log files, 164 edm output root files, 49 DQM output files
TriggerResults: no differences found

makortel · 2023-03-24T19:53:08Z

+core

cmsbuild · 2023-03-24T19:53:33Z

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. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2023-03-26T20:54:25Z

+1

cmsbuild added this to the CMSSW_13_1_X milestone Mar 21, 2023

cmsbuild added core-pending pending-signatures tests-pending orp-pending code-checks-pending labels Mar 21, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 21, 2023

cmsbuild added tests-started and removed tests-pending labels Mar 21, 2023

makortel reviewed Mar 21, 2023

View reviewed changes

cmsbuild added tests-rejected and removed tests-started labels Mar 22, 2023

Dr15Jones force-pushed the fjrTempError branch from fb249dd to 243f7d4 Compare March 23, 2023 16:31

cmsbuild added tests-pending code-checks-pending and removed tests-rejected code-checks-approved labels Mar 23, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 23, 2023

cmsbuild added tests-started and removed tests-pending labels Mar 23, 2023

cmsbuild added tests-approved and removed tests-started labels Mar 23, 2023

cmsbuild mentioned this pull request Mar 24, 2023

Add upper cut on jet time to HLTJetTimingFilter #41089

Closed

makortel mentioned this pull request Mar 24, 2023

Maintenance tasks of week 12 cms-sw/framework-team#513

Closed

4 tasks

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Mar 24, 2023

cmsbuild added orp-approved and removed orp-pending labels Mar 26, 2023

cmsbuild merged commit 94b8d97 into cms-sw:master Mar 26, 2023

Dr15Jones deleted the fjrTempError branch March 27, 2023 15:09

makortel mentioned this pull request Mar 28, 2023

Make framework job report to contain failure unless the job succeeds up do "end of main()" cms-sw/framework-team#514

Closed

2 tasks

makortel mentioned this pull request Apr 6, 2023

Switch to std::chrono::steady_clock #41291

Merged

This was referenced Aug 15, 2023

Add temporary failure element to Framework Job Report [13_0] #42576

Merged

Add temporary failure element to Framework Job Report [12_6] #42577

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add temporary failure element to Framework Job Report #41124

Add temporary failure element to Framework Job Report #41124

Dr15Jones commented Mar 21, 2023

cmsbuild commented Mar 21, 2023

cmsbuild commented Mar 21, 2023

Dr15Jones commented Mar 21, 2023

makortel Mar 21, 2023

makortel Mar 21, 2023

makortel commented Mar 21, 2023

cmsbuild commented Mar 22, 2023

makortel commented Mar 22, 2023

cmsbuild commented Mar 23, 2023

cmsbuild commented Mar 23, 2023

makortel commented Mar 23, 2023

makortel commented Mar 23, 2023

cmsbuild commented Mar 23, 2023

makortel commented Mar 24, 2023

cmsbuild commented Mar 24, 2023

perrotta commented Mar 26, 2023

		static char const* const kJobReportEndElement = "</FrameworkJobReport>\n";
		static constexpr int kEndElementSize = 22;

Add temporary failure element to Framework Job Report #41124

Add temporary failure element to Framework Job Report #41124

Conversation

Dr15Jones commented Mar 21, 2023

PR description:

PR validation:

cmsbuild commented Mar 21, 2023

cmsbuild commented Mar 21, 2023

Dr15Jones commented Mar 21, 2023

makortel Mar 21, 2023

Choose a reason for hiding this comment

makortel Mar 21, 2023

Choose a reason for hiding this comment

makortel commented Mar 21, 2023

cmsbuild commented Mar 22, 2023

Unit Tests

Comparison Summary

makortel commented Mar 22, 2023

cmsbuild commented Mar 23, 2023

cmsbuild commented Mar 23, 2023

makortel commented Mar 23, 2023

makortel commented Mar 23, 2023

cmsbuild commented Mar 23, 2023

Comparison Summary

makortel commented Mar 24, 2023

cmsbuild commented Mar 24, 2023

perrotta commented Mar 26, 2023