Skip to content
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

Concurrent Path Processing #15882

Merged
merged 14 commits into from Sep 21, 2016

Conversation

Dr15Jones
Copy link
Contributor

Allow Paths and EndPaths to run concurrently within one Event.
This change properly handles dependencies between modules on different paths (a module on one path will wait until a module on another path has finished). In addition unscheduled modules can now safely depend on scheduled modules.

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

DataFormats/Provenance
FWCore/Concurrency
FWCore/Framework
FWCore/Integration
FWCore/Sources
IOPool/Input

@cmsbuild, @smuzaffar, @Dr15Jones can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @wddgit, @wmtan this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 16, 2016

@Dr15Jones
Copy link
Contributor Author

@smuzaffar the tests actually aborted by CMS-bot didn't seem to notice

@smuzaffar
Copy link
Contributor

@Dr15Jones , tests were aborted after 15 hours. Though I have restarted the tests but I am afraid it will again abort as I see workflows are hanging. I see runTheMatrix timeout after 3 hours.

@Dr15Jones
Copy link
Contributor Author

Hanging is a possibility with this code change. Guess I'll have to re-compile the world myself since I don't want DEVEL that broken :).

@cmsbuild
Copy link
Contributor

Pull request #15882 was updated. @cmsbuild, @smuzaffar, @Dr15Jones, @vanbesien, @dmitrijus can you please check and sign again.

@Dr15Jones
Copy link
Contributor Author

I brought in more of the changes from #15740

@smuzaffar
Copy link
Contributor

smuzaffar commented Sep 19, 2016

please test
(I aborted the previous test)

@cmsbuild
Copy link
Contributor

Pull request #15882 was updated. @perrotta, @smuzaffar, @Dr15Jones, @silviodonato, @dmitrijus, @Martin-Grunewald, @cmsbuild, @fwyzard, @vanbesien can you please check and sign again.

@Dr15Jones
Copy link
Contributor Author

@smuzaffar this fixes a case where the framework would get stuck during RelVal processing. I don't know if this is the last of them though.

@smuzaffar
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 20, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/15279/console

Dr15Jones and others added 6 commits September 21, 2016 08:18
Within one event, process all Paths and EndPaths simultaneously. This includes handling data dependencies between Paths.
Several tests depend on groups of Paths running sequentially. Those were modified to add modules where the data dependencies between modules would force the ordering.
Another test was comparing the output of Tracer which changes because of the concurrent processing order.
When the Path ends early, all modules after the failing filter are
supposed to be notified. However, the code notified all modules after
the filter and the filter itself. If the filter added data to the Event
then when told to skip it would try to add the data again which would
cause an exception.
The FastTimerService does not presently handle the case where one
Stream can use more than one thread, either for modules or paths.
The agreed upon solution for now is to disable it.
Since unscheduled EDProducers can now depend on scheduled modules the
we can now remove all EDProducers from a Path. In addition, if an EDFilter
is marked as cms.ignore, it can also be removed from a Path.

To help add cms.ignore to EDFilters on a Path, we now have the
function ignoreAllFiltersOnPath.
@cmsbuild
Copy link
Contributor

Pull request #15882 was updated. @perrotta, @smuzaffar, @cmsbuild, @silviodonato, @Martin-Grunewald, @Dr15Jones, @fwyzard can you please check and sign again.

@Dr15Jones
Copy link
Contributor Author

@smuzaffar I rebased on top of CMSSW_8_1_X but it didn't complain about any conflicts. Weird. Assuming the rebase did avoid the merge problems I think we are at the point where we should just put this into CMSSW_8_1_DEVEL_X. If it breaks it, we just back it out.

@Dr15Jones
Copy link
Contributor Author

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 21, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/15306/console

@smuzaffar
Copy link
Contributor

Ok, I am merging it for next IB.

@smuzaffar smuzaffar merged commit 2ed9873 into cms-sw:CMSSW_8_1_DEVEL_X Sep 21, 2016
@cmsbuild
Copy link
Contributor

Comparison not run due to Build errors (RelVals and Igprof tests were also skipped)

@cmsbuild
Copy link
Contributor

-1

Tested at: c3f600a

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
d3673cc
f07cdcd
e177082
e3aa8c6
fc6f479
508e5c6
211af7c
64b8eb3
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15882/15306/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15882/15306/git-merge-result

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15882/15306/summary.html

I found follow errors while testing this PR

Failed tests: Build

  • Build:

I found an error when building:

>> Compiling edm plugin /build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_DEVEL_X_2016-09-20-2300/src/IOMC/ParticleGuns/src/FlatEGunASCIIWriter.cc 
>> Compiling edm plugin /build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_DEVEL_X_2016-09-20-2300/src/IOMC/ParticleGuns/src/FlatRandomPtGunProducer.cc 
>> Compiling edm plugin /build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_DEVEL_X_2016-09-20-2300/src/IOMC/ParticleGuns/src/ExpoRandomPGunProducer.cc 
>> Compiling edm plugin /build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_DEVEL_X_2016-09-20-2300/src/IOMC/ParticleGuns/src/FlatRandomPtThetaGunProducer.cc 
/build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_DEVEL_X_2016-09-20-2300/src/IOMC/ParticleGuns/src/BaseRandomtXiGunProducer.cc: In member function 'virtual void edm::BaseRandomtXiGunProducer::endRunProduce(edm::Run&, const edm::EventSetup&)':
/build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_DEVEL_X_2016-09-20-2300/src/IOMC/ParticleGuns/src/BaseRandomtXiGunProducer.cc:87:24: error: no matching function for call to 'edm::Run::put(std::auto_ptr&)'
    run.put( genRunInfo );
                        ^
In file included from /build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_DEVEL_X_2016-09-20-2300/src/IOMC/ParticleGuns/src/BaseRandomtXiGunProducer.cc:12:0:
/build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_DEVEL_X_2016-09-20-2300/src/FWCore/Framework/interface/Run.h:111:5: note: candidate: template void edm::Run::put(std::unique_ptr<_Tp>)
     put(std::unique_ptr product) {put(std::move(product), std::string());}


The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
d3673cc
f07cdcd
e177082
e3aa8c6
fc6f479
508e5c6
211af7c
64b8eb3
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15882/15306/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15882/15306/git-merge-result

@Dr15Jones
Copy link
Contributor Author

This build failure is not from this pull request and will show up in the next IB. The problem is #15096 is not compatible with #15897.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants