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.

Sign up for GitHub

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

Make sure to report an error if process.options.numberOfThreads has wrong type #41349

Merged
merged 1 commit into from
Apr 17, 2023
Merged

Make sure to report an error if process.options.numberOfThreads has wrong type #41349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions FWCore/Framework/test/BuildFile.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,3 +376,8 @@ test name="TestFWCoreFrameworkDeleteEarly" command="test_deleteEarly.sh"/>
<test name="testFWCoreFrameworkGetByType_consumesMany_alias" command="cmsRun ${LOCALTOP}/src/FWCore/Framework/test/test_get_by_type_cfg.py -- --useConsumesMany --useEDAlias"/>
<test name="testFWCoreFrameworkGetByType_getterOfProduct" command="cmsRun ${LOCALTOP}/src/FWCore/Framework/test/test_get_by_type_cfg.py"/>
<test name="testFWCoreFrameworkGetByType_getterOfProduct_alias" command="cmsRun ${LOCALTOP}/src/FWCore/Framework/test/test_get_by_type_cfg.py -- --useEDAlias"/>

<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)'"/>
10 changes: 10 additions & 0 deletions FWCore/Framework/test/run_wrongOptionsType.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 ${SCRAM_TEST_PATH}/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

21 changes: 21 additions & 0 deletions FWCore/Framework/test/test_wrongOptionsType_cfg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
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.input = 2

setattr(process.options, args.name, eval(str(args.value)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider making the setting function more recursive? I.e. if it is already set with an 'optional' it enforces the type checking? I'm guessing we didn't do that initially as some older parts wanted to be able to declare different types. But if we made 'optional' smarter to know it could have different types, maybe we could handle that case as well.

In that way optional could always catch type problems at the python level.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if we do what I suggest above, I think we should still do the check you are doing in the PR at the C++ level as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree in being able to catch the problem already in python would be be better (at least for some patterns, I guess we can't do much for del process.options.numberOfThreads; process.options.numberOfThreads = cms.untracked.int64(3)).

A technical complication for stronger enforcement is that the assignment

process.options.numberOfThreads = cms.untracked.int64(3)

executes code in the process.options PSet. So the PSet.__setattr__() would have to be made smarter (it looks doable though).

Another complication is that only one parameter in the process.options PSet is optional, and the others (including numberOfThreads) have a default value set. So I think we'd need to think more how exactly to proceed.

All that sounds like a material for a new issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened an issue #41356

11 changes: 7 additions & 4 deletions FWCore/ParameterSet/src/ThreadsInfo.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,15 @@ namespace edm {
// default values
ThreadsInfo threadsInfo;

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")) {
threadsInfo.nThreads_ = ops.getUntrackedParameter<unsigned int>("numberOfThreads");
}
if (ops.existsAs<unsigned int>("sizeOfStackForThreadsInKB", false)) {
if (ops.exists("sizeOfStackForThreadsInKB")) {
threadsInfo.stackSize_ = ops.getUntrackedParameter<unsigned int>("sizeOfStackForThreadsInKB");
}
}
Expand All @@ -26,7 +29,7 @@ namespace edm {

void setThreadOptions(ThreadsInfo const& threadsInfo, edm::ParameterSet& pset) {
edm::ParameterSet newOp;
if (pset.existsAs<edm::ParameterSet>("options", false)) {
if (pset.exists("options")) {
newOp = pset.getUntrackedParameterSet("options");
}
newOp.addUntrackedParameter<unsigned int>("numberOfThreads", threadsInfo.nThreads_);
Expand Down