Merge pull request #41654 from makortel/fixNumberOfThreadsTypeCheck_94x

[9_4_X] Make sure to report an error if process.options.numberOfThreads has wrong type
cms-sw · May 17, 2023 · 2022772 · 2022772
2 parents 0cff4ac + cc7990e
commit 2022772
Show file tree

Hide file tree

Showing 5 changed files with 47 additions and 6 deletions.
diff --git a/FWCore/Framework/bin/cmsRun.cpp b/FWCore/Framework/bin/cmsRun.cpp
@@ -306,12 +306,15 @@ int main(int argc, char* argv[]) {
       {
         if(not setNThreadsOnCommandLine) {
           std::shared_ptr<edm::ParameterSet> pset = processDesc->getProcessPSet();
-          if(pset->existsAs<edm::ParameterSet>("options",false)) {
+          // Note: it is important to not check the type or trackedness in
+          // exists() call to ensure that the getUntrackedParameter() calls
+          // will fail if the parameters have an incorrect type
+          if(pset->exists("options")) {
             auto const& ops = pset->getUntrackedParameterSet("options");
-            if(ops.existsAs<unsigned int>("numberOfThreads",false)) {
+            if(ops.exists("numberOfThreads")) {
               unsigned int nThreads = ops.getUntrackedParameter<unsigned int>("numberOfThreads");
               unsigned int stackSize=kDefaultSizeOfStackForThreadsInKB;
-              if(ops.existsAs<unsigned int>("sizeOfStackForThreadsInKB",false)) {
+              if(ops.exists("sizeOfStackForThreadsInKB")) {
                 stackSize = ops.getUntrackedParameter<unsigned int>("sizeOfStackForThreadsInKB");
               }
               const auto nThreadsUsed = setNThreads(nThreads,stackSize,tsiPtr);
@@ -326,7 +329,7 @@ int main(int argc, char* argv[]) {
           //inject it into the top level ParameterSet
           edm::ParameterSet newOp;
           std::shared_ptr<edm::ParameterSet> pset = processDesc->getProcessPSet();
-          if(pset->existsAs<edm::ParameterSet>("options",false)) {
+          if(pset->exists("options")) {
             newOp = pset->getUntrackedParameterSet("options");
           }
           newOp.addUntrackedParameter<unsigned int>("numberOfThreads",nThreadsOnCommandLine);

diff --git a/FWCore/Framework/src/EventProcessor.cc b/FWCore/Framework/src/EventProcessor.cc
@@ -404,7 +404,7 @@ namespace edm {
     forceESCacheClearOnNewRun_ = optionsPset.getUntrackedParameter<bool>("forceEventSetupCacheClearOnNewRun", false);
     //threading
     unsigned int nThreads=1;
-    if(optionsPset.existsAs<unsigned int>("numberOfThreads",false)) {
+    if(optionsPset.exists("numberOfThreads")) {
       nThreads = optionsPset.getUntrackedParameter<unsigned int>("numberOfThreads");
       if(nThreads == 0) {
         nThreads = 1;
@@ -414,7 +414,7 @@ namespace edm {
        unsigned int nStreams =nThreads;
     */
     unsigned int nStreams =1;
-    if(optionsPset.existsAs<unsigned int>("numberOfStreams",false)) {
+    if(optionsPset.exists("numberOfStreams")) {
       nStreams = optionsPset.getUntrackedParameter<unsigned int>("numberOfStreams");
       if(nStreams==0) {
         nStreams = nThreads;

diff --git a/FWCore/Framework/test/BuildFile.xml b/FWCore/Framework/test/BuildFile.xml
@@ -315,3 +315,8 @@
   <flags   TEST_RUNNER_ARGS=" /bin/bash FWCore/Framework/test run_PrintDependencies.sh"/>
   <use name="FWCore/Utilities"/>
 </bin>
+
+<test name="testFWCoreFrameworkOptionsNumberOfThreadsType" command="cmsRun ${LOCALTOP}/src/FWCore/Framework/test/test_wrongOptionsType_cfg.py -- --name=numberOfThreads --value='cms.untracked.uint32(1)'"/>
+<test name="testFWCoreFrameworkWrongOptionsNumberOfThreadsType" command="run_wrongOptionsType.sh numberOfThreads 'cms.untracked.int32(1)'"/>
+<test name="testFWCoreFrameworkWrongOptionsNumberOfStreamsType" command="run_wrongOptionsType.sh numberOfStreams 'cms.untracked.int32(1)'"/>
+<test name="testFWCoreFrameworkWrongOptionssizeOfStackForThreadsInKBType" command="run_wrongOptionsType.sh sizeOfStackForThreadsInKB 'cms.untracked.int32(1024)'"/>
diff --git a/FWCore/Framework/test/run_wrongOptionsType.sh b/FWCore/Framework/test/run_wrongOptionsType.sh
@@ -0,0 +1,10 @@
+#!/bin/bash
+
+NAME=$1
+VALUE=$2
+
+function die { echo $1: status $2 ; echo === Log file === ; cat ${3:-/dev/null} ; echo === End log file === ; exit $2; }
+
+cmsRun ${LOCALTOP}/src/FWCore/Framework/test/test_wrongOptionsType_cfg.py -- --name=${NAME} --value="$VALUE" > ${NAME}.log 2>&1 && die "cmsRun for ${NAME} succeeded, should have failed" 1 ${NAME}.log
+grep -E "(The type in the configuration is incorrect)|(ValueError type of .* is expected to be .* but declared as)" ${NAME}.log > /dev/null || die "cmsRun for ${NAME} failed for other reason than incorrect configuration type" $? ${NAME}.log
+
diff --git a/FWCore/Framework/test/test_wrongOptionsType_cfg.py b/FWCore/Framework/test/test_wrongOptionsType_cfg.py
@@ -0,0 +1,23 @@
+import FWCore.ParameterSet.Config as cms
+
+import argparse
+import sys
+
+parser = argparse.ArgumentParser(prog=sys.argv[0], description='Test wrong process.options parameter types')
+
+parser.add_argument("--name", help="Name of parameter", type=str)
+parser.add_argument("--value", help="Value of the parameter", type=str)
+
+argv = sys.argv[:]
+if '--' in argv:
+    argv.remove("--")
+args, unknown = parser.parse_known_args(argv)
+
+process = cms.Process("TEST")
+process.source = cms.Source("EmptySource")
+
+process.maxEvents = cms.untracked.PSet(input = cms.untracked.int32(2))
+
+# sizeOfStackForThreadsInKB is checked only if numberOfThreads is set
+process.options = cms.untracked.PSet(numberOfThreads = cms.untracked.int32(1))
+setattr(process.options, args.name, eval(str(args.value)))