Remove SubProcess and references in EventProcessor related code #47659

wddgit · 2025-03-21T21:32:50Z

PR description:

Remove from SubProcess from EventProcessor related code.

I've intentionally left uses of SubProcess related to the EventSetup, Services, and module deletion for separate later PRs, but this one gets everything else.

This is the second PR in a series. The first was PR #47565. That PR removed support for SubProcess from the CMS configuration system and made it impossible to run a job with a SubProcess configured. It also removed all SubProcess tests.

Resolves cms-sw/framework-team#1282

PR validation:

Relies on existing tests. There is no new functionality. This is cleaning up code that served no purpose anymore.

cmsbuild · 2025-03-21T21:33:15Z

cms-bot internal usage

cmsbuild · 2025-03-21T21:34:59Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47659/44194

There are other open Pull requests which might conflict with changes you have proposed:
- File FWCore/Common/src/OutputProcessBlockHelper.cc modified in PR(s): Removed -Wconversion warnings in part of framework #47339
- File FWCore/Framework/interface/Schedule.h modified in PR(s): Access PathsAndConsumesOfModules from new signal #47467
- File FWCore/Framework/src/EventProcessor.cc modified in PR(s): Access PathsAndConsumesOfModules from new signal #47467
- File FWCore/Framework/src/Schedule.cc modified in PR(s): Disable ROOT automatic class parsing during finishSchedule() #46858, Access PathsAndConsumesOfModules from new signal #47467
- File FWCore/Framework/src/SubProcess.cc modified in PR(s): Access PathsAndConsumesOfModules from new signal #47467
- File FWCore/TestProcessor/src/TestProcessor.cc modified in PR(s): Access PathsAndConsumesOfModules from new signal #47467

cmsbuild · 2025-03-21T21:35:24Z

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

It involves the following packages:

DataFormats/Provenance (core)
FWCore/Common (core)
FWCore/Framework (core)
FWCore/Integration (core)
FWCore/ParameterSet (core)
FWCore/TestProcessor (core)
IOPool/Common (core)
IOPool/Output (core)

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

cms-bot commands are listed here

wddgit · 2025-03-21T21:36:13Z

enable threading

wddgit · 2025-03-21T21:36:22Z

please test

cmsbuild · 2025-03-22T02:45:44Z

+1

Size: This PR adds an extra 168KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2ccc39/45144/summary.html
COMMIT: 7dc0b16
CMSSW: CMSSW_15_1_X_2025-03-21-1100/el8_amd64_gcc12
Additional Tests: THREADING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/47659/45144/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 2 lines to the logs
Reco comparison results: 10 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3909207
DQMHistoTests: Total failures: 51
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3909136
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
Checked 215 log files, 184 edm output root files, 50 DQM output files
TriggerResults: no differences found

makortel · 2025-03-24T14:09:14Z

Comparison differences are related to #47071

makortel · 2025-03-24T14:13:30Z

FWCore/Framework/src/EventProcessor.cc

    PathsAndConsumesOfModules pathsAndConsumesOfModules;
    pathsAndConsumesOfModules.initialize(schedule_.get(), preg());

    std::vector<ModuleProcessName> consumedBySubProcesses;


Is the consumedBySubProcess something to be cleaned up in a subsequent PR?

Yes that is part of the module deletion code. I was going to tackle that next.

makortel · 2025-03-24T14:18:34Z

FWCore/Framework/src/EventProcessor.cc

-    }) | chain::runLast(std::move(task));
+    ServiceRegistry::Operate op(serviceToken_);
+    schedule_->writeProcessBlockAsync(
+        task, principalCache_.processBlockPrincipal(processBlockType), &processContext_, actReg_.get());


The task could be moved

Suggested change

task, principalCache_.processBlockPrincipal(processBlockType), &processContext_, actReg_.get());

std::move(task), principalCache_.processBlockPrincipal(processBlockType), &processContext_, actReg_.get());

I think we need to not move so that the ServiceRegistry::Operate goes out of scope before the task goes out of scope. I believe there is the extremely unlikely possibility the Services stay on a very long time and cause problems during shutdown, because once we call doneWaiting everything else continues to completion and we are off to the races. IIRC, we actually saw that problem in the past in some rare cases.

Ok. Could you then add a comment here that the lifetime of the task must be longer than the lifetime of op for that reason?

There 10 or 20 places in the code like this in EventProcessor. Should I mark them all? Maybe a good idea. This is something easy to forget about.

I added the two comments you suggested. If there is more related to this I would suggest deferring it until I submit the PR dealing with SubProcess and Services. Let me know about if there is something more you want me to do and I'll get it into the first version of that PR. (That is assuming it isn't something that is going to cause an immediate problem.)

makortel · 2025-03-24T14:19:14Z

FWCore/Framework/src/EventProcessor.cc

-        }
-      }) | chain::runLast(std::move(task));
+      ServiceRegistry::Operate op(serviceToken_);
+      schedule_->writeRunAsync(task, runPrincipal, &processContext_, actReg_.get(), mergeableRunProductMetadata);


The task could be moved

Suggested change

schedule_->writeRunAsync(task, runPrincipal, &processContext_, actReg_.get(), mergeableRunProductMetadata);

schedule_->writeRunAsync(std::move(task), runPrincipal, &processContext_, actReg_.get(), mergeableRunProductMetadata);

Same as previous comment.

makortel · 2025-03-24T14:33:01Z

FWCore/TestProcessor/src/TestProcessor.cc

+        for (unsigned int i = 0; i < preallocations_.numberOfStreams(); ++i) {
+          schedule_->processOneStreamAsync<Traits>(
+              WaitingTaskHolder(taskGroup_, &streamLoopWaitTask), i, transitionInfo, serviceToken_);
+        }


I find the replacement of beginGlobalTransitionAsync() with schedule_->processOneGlobalAsync() fine as such, although one could make an argument against along "if we ever want to have a customization point there, we need to add the beginGlobalTransitionAsync() back".

I'm less certain about the replacement of beginStreamsTransitionAsync() with the loop over streams calling schedule_->processOneStreamAsync(). Or in other words, I'd find some value in keeping the loop over streams abstracted in beginStreamsTransitionAsync().

Further thoughts? @wddgit @Dr15Jones

What ever we do we should keep EventProcessor and TestProcessor using the same code, preferable by sharing the same routines.

The loop over streams is already handled very differently in EventProcessor and TestProcessor. The one in EventProcessor involves complex concurrency code. In the loop over streams in EventProcessor, tasks are inserted into the streamQueues_. For example, this is the loop over streams in EventProcessor::beginRunAsync. beginStreamsTransitionAsync is not called at all. The call to beginStreamTransitionAsync (almost the same but no "s" in the function name) is inside streamBeginRunAsync.

streamQueuesInserter_.push(*holder.group(), [this, status, holder]() mutable { for (unsigned int i = 0; i < preallocations_.numberOfStreams(); ++i) { CMS_SA_ALLOW try { streamQueues_[i].push(*holder.group(), [this, i, status, holder]() mutable { streamBeginRunAsync(i, std::move(status), std::move(holder)); }); } catch (...) { if (status->streamFinishedBeginRun()) { WaitingTaskHolder copyHolder(holder); copyHolder.doneWaiting(std::current_exception()); status->resetBeginResources(); queueWhichWaitsForIOVsToFinish_.resume(); exceptionRunStatus_ = status; } } } });

TestProcessor does not have support for this level of concurrency. Even before this PR, TestProcessor is the only thing in all of CMSSW using beginStreamsTransitionAsync.

I could add the beginStreamTransitionAsync back, but it will simply call directly through to the other function and add no value at all. It seems to me, it is just as likely to get in the way as help if a new version of SubProcess is implemented. And in the meantime removing it simplifies the code.

I added two new functions in TestProcessor, processTransitionForAllStreams and processGlobalTransition. They serve to get rid of some of the code duplication in this part of the code as we discussed.

Dr15Jones · 2025-03-24T14:56:00Z

FWCore/TestProcessor/src/TestProcessor.cc

-                                            emptyList,
-                                            false);
+          for (unsigned int i = 0; i < preallocations_.numberOfStreams(); ++i) {
+            schedule_->processOneStreamAsync<Traits>(


I think this loop has a problem. Each iteration creates a new WaitingTaskHolder, while the old code made 1 which lived for the lifetime of the loop. If the thread running the loop was stalled, then the already started scheduled could finish their jobs an cause all the previously created WaitingTaskHolders to go away thereby starting the held task BEFORE this loop finishes.

In general, I would agree that in our code this pattern would cause problems. But in this case, it is in a FinalWaitingTask section of code. It will finish the loop before it gets to the wait. I think it will be OK in this case.

FinalWaitingTask streamLoopWaitTask{taskGroup_}; using Traits = OccurrenceTraits<LuminosityBlockPrincipal, BranchActionStreamEnd>; for (unsigned int i = 0; i < preallocations_.numberOfStreams(); ++i) { schedule_->processOneStreamAsync<Traits>( WaitingTaskHolder(taskGroup_, &streamLoopWaitTask), i, transitionInfo, serviceToken_, false); } streamLoopWaitTask.wait();

Chris was right about this. Fixed. Thanks.

cmsbuild · 2025-04-09T21:41:25Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47659/44447

There are other open Pull requests which might conflict with changes you have proposed:
- File FWCore/Framework/interface/Schedule.h modified in PR(s): Access PathsAndConsumesOfModules from new signal #47467
- File FWCore/Framework/src/EventProcessor.cc modified in PR(s): Access PathsAndConsumesOfModules from new signal #47467
- File FWCore/Framework/src/Schedule.cc modified in PR(s): Access PathsAndConsumesOfModules from new signal #47467
- File FWCore/Framework/src/SubProcess.cc modified in PR(s): Access PathsAndConsumesOfModules from new signal #47467
- File FWCore/TestProcessor/src/TestProcessor.cc modified in PR(s): Access PathsAndConsumesOfModules from new signal #47467

cmsbuild · 2025-04-14T15:20:25Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47659/44504

cmsbuild · 2025-04-14T15:20:53Z

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

wddgit · 2025-04-14T15:20:59Z

please test

Only change is a rebase on top of the other PR (which was merged) to resolve the conflict. We should remove the hold if the tests pass.

cmsbuild · 2025-04-14T19:49:13Z

+1

Size: This PR adds an extra 76KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2ccc39/45544/summary.html
COMMIT: f2757a0
CMSSW: CMSSW_15_1_X_2025-04-14-1100/el8_amd64_gcc12
Additional Tests: THREADING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/47659/45544/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 1 lines from the logs
Reco comparison results: 10 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3916361
DQMHistoTests: Total failures: 52
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3916289
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
Checked 215 log files, 184 edm output root files, 50 DQM output files
TriggerResults: no differences found

makortel · 2025-04-14T20:46:29Z

Comparison differences are related to #47071

makortel · 2025-04-14T20:47:00Z

+core

wddgit · 2025-04-17T18:46:38Z

@mandrenguyen This PR is not high priority, but I just wanted to make sure it was still on your list of things that need to be done. The other PR was merged and seems OK. I rebased this one already on top of it and this one is ready to merge. I think the hold can be released.

makortel · 2025-04-21T13:28:39Z

unhold

Although I don't think I have the power to do that

mandrenguyen · 2025-04-21T13:31:57Z

unhold
apologies @wddgit I was off for a few days

cmsbuild · 2025-04-21T13:32:16Z

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

mandrenguyen · 2025-04-21T13:33:50Z

+1

cmsbuild added this to the CMSSW_15_1_X milestone Mar 21, 2025

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

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

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

cmsbuild added tests-approved and removed tests-started labels Mar 22, 2025

makortel reviewed Mar 24, 2025

View reviewed changes

Dr15Jones reviewed Mar 24, 2025

View reviewed changes

makortel mentioned this pull request Mar 25, 2025

Non-reproducibility in TrackerPhase2OTL1Track #47071

Open

cmsbuild mentioned this pull request Mar 27, 2025

Move RandomNumberGenerator service base class to FWCore/AbstractServices #47719

Merged

wddgit force-pushed the rmSubProcessInEventProcessor branch from 7dc0b16 to eecd95a Compare April 9, 2025 21:39

cmsbuild added tests-pending code-checks-pending and removed tests-approved code-checks-approved labels Apr 9, 2025

cmsbuild removed the code-checks-pending label Apr 9, 2025

cmsbuild added core-pending tests-pending code-checks-pending and removed code-checks-approved labels Apr 14, 2025

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 14, 2025

cmsbuild added tests-started and removed tests-pending labels Apr 14, 2025

cmsbuild added tests-approved and removed tests-started labels Apr 14, 2025

cmsbuild added core-approved and removed core-pending labels Apr 14, 2025

cmsbuild added fully-signed and removed pending-signatures hold labels Apr 21, 2025

cmsbuild added orp-approved and removed orp-pending labels Apr 21, 2025

cmsbuild merged commit b656ba0 into cms-sw:master Apr 21, 2025
12 checks passed

wddgit mentioned this pull request Apr 22, 2025

Remove code supporting SubProcess and related to module deletion #47930

Merged

	task, principalCache_.processBlockPrincipal(processBlockType), &processContext_, actReg_.get());
	std::move(task), principalCache_.processBlockPrincipal(processBlockType), &processContext_, actReg_.get());

	schedule_->writeRunAsync(task, runPrincipal, &processContext_, actReg_.get(), mergeableRunProductMetadata);
	schedule_->writeRunAsync(std::move(task), runPrincipal, &processContext_, actReg_.get(), mergeableRunProductMetadata);

Remove SubProcess and references in EventProcessor related code #47659

Remove SubProcess and references in EventProcessor related code #47659

Uh oh!

Conversation

wddgit commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR description:

PR validation:

Uh oh!

cmsbuild commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmsbuild commented Mar 21, 2025

Uh oh!

cmsbuild commented Mar 21, 2025

Uh oh!

wddgit commented Mar 21, 2025

Uh oh!

wddgit commented Mar 21, 2025

Uh oh!

cmsbuild commented Mar 22, 2025

Comparison Summary

Uh oh!

makortel commented Mar 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmsbuild commented Apr 9, 2025

Uh oh!

cmsbuild commented Apr 14, 2025

Uh oh!

cmsbuild commented Apr 14, 2025

Uh oh!

wddgit commented Apr 14, 2025

Uh oh!

cmsbuild commented Apr 14, 2025

Comparison Summary

Uh oh!

makortel commented Apr 14, 2025

Uh oh!

makortel commented Apr 14, 2025

Uh oh!

wddgit commented Apr 17, 2025

Uh oh!

makortel commented Apr 21, 2025

Uh oh!

mandrenguyen commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

wddgit commented Mar 21, 2025 •

edited

Loading

cmsbuild commented Mar 21, 2025 •

edited

Loading

mandrenguyen commented Apr 21, 2025 •

edited

Loading