-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Specify separate directories to run external instances in ExternalGeneratorFilter #34173
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
Specify separate directories to run external instances in ExternalGeneratorFilter #34173
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34173/23382
|
|
A new Pull Request was created by @colizz (Congqiao Li) for master. It involves the following packages: GeneratorInterface/Core @SiewYan, @mkirsano, @cmsbuild, @GurpreetSinghChahal, @agrohsje, @alberto-sanchez can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
| auto curDir = current_path(); | ||
| auto newDir = path("thread"s + std::to_string(id_)); | ||
| create_directory(newDir, ec); | ||
| current_path(newDir, ec); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned that any possible errors are silently ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, shall this work?
| current_path(newDir, ec); | |
| current_path(newDir, ec); | |
| if (ec) { | |
| throw cms::Exception("ExternalFailed") | |
| << "failed entering the working dir for process " << id_ << ": " << ec.message(); | |
| } |
(Actually the previous code was following this example
cmssw/GeneratorInterface/LHEInterface/plugins/ExternalLHEProducer.cc
Lines 481 to 483 in c5d2673
| auto newDir = path("thread"s + std::to_string(id)); | |
| create_directory(newDir, ec); | |
| current_path(newDir, ec); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about letting these functions to throw the exceptions? (i.e. remove the ec altogether, assuming the messages of those exceptions would be clear-enough)
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34173/23397
|
|
Pull request #34173 was updated. @SiewYan, @mkirsano, @cmsbuild, @GurpreetSinghChahal, @agrohsje, @alberto-sanchez can you please check and sign again. |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6b5e54/16129/summary.html Comparison SummarySummary:
|
|
+generators |
|
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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
…eratorFilter (10_6_X) Backport cms-sw#34173 to 10_6_X Additional change: replace std::filesystem with boost::filesystem
PR description:
In the ExternalGeneratorFilter module, external cmsRun instances are launched by the main instance in the same working directory.
As some types of generator will release intermediate files (e.g. H7, Sherpa), we execute the commands in different directories (specifed as
thread0,thread1, ...) to avoid overlapping.PR validation:
Validated over all possible GeneratorFilter-based modules as summarised in #34146 (comment).
Physics results show good consistency.