Settings should be referenced via their CONF_XXX name

[keckler]

When we define settings, we almost always go through the trouble of defining its name with a special module-level variable of a particular convention. See, for instance:

armi/armi/settings/fwSettings/globalSettings.py

Lines 34 to 121 in 53ae82e

	# Framework settings
	CONF_NUM_PROCESSORS = "numProcessors"
	CONF_INITIALIZE_BURN_CHAIN = "initializeBurnChain"
	CONF_BURN_CHAIN_FILE_NAME = "burnChainFileName"
	CONF_AXIAL_MESH_REFINEMENT_FACTOR = "axialMeshRefinementFactor"
	CONF_AUTOMATIC_VARIABLE_MESH = "automaticVariableMesh"
	CONF_TRACE = "trace"
	CONF_PROFILE = "profile"
	CONF_COVERAGE = "coverage"
	CONF_COVERAGE_CONFIG_FILE = "coverageConfigFile"
	CONF_MIN_MESH_SIZE_RATIO = "minMeshSizeRatio"
	CONF_CYCLE_LENGTH = "cycleLength"
	CONF_CYCLE_LENGTHS = "cycleLengths"
	CONF_AVAILABILITY_FACTOR = "availabilityFactor"
	CONF_AVAILABILITY_FACTORS = "availabilityFactors"
	CONF_POWER_FRACTIONS = "powerFractions"
	CONF_BURN_STEPS = "burnSteps"
	CONF_BETA = "beta"
	CONF_DECAY_CONSTANTS = "decayConstants"
	CONF_BRANCH_VERBOSITY = "branchVerbosity"
	CONF_BU_GROUPS = "buGroups"
	CONF_BURNUP_PEAKING_FACTOR = "burnupPeakingFactor"
	CONF_CIRCULAR_RING_PITCH = "circularRingPitch"
	CONF_COMMENT = "comment"
	CONF_COPY_FILES_FROM = "copyFilesFrom"
	CONF_COPY_FILES_TO = "copyFilesTo"
	CONF_DEBUG = "debug"
	CONF_DEBUG_MEM = "debugMem"
	CONF_DEBUG_MEM_SIZE = "debugMemSize"
	CONF_DEFAULT_SNAPSHOTS = "defaultSnapshots"
	CONF_DETAIL_ALL_ASSEMS = "detailAllAssems"
	CONF_DETAIL_ASSEM_LOCATIONS_BOL = "detailAssemLocationsBOL"
	CONF_DETAIL_ASSEM_NUMS = "detailAssemNums"
	CONF_DUMP_SNAPSHOT = "dumpSnapshot"
	CONF_PHYSICS_FILES = "savePhysicsFiles"
	CONF_DO_ORIFICED_TH = "doOrificedTH" # zones
	CONF_EQ_DIRECT = "eqDirect" # fuelCycle/equilibrium coupling
	CONF_FRESH_FEED_TYPE = "freshFeedType"
	CONF_GEOM_FILE = "geomFile"
	CONF_START_CYCLE = "startCycle"
	CONF_LOADING_FILE = "loadingFile"
	CONF_START_NODE = "startNode"
	CONF_LOAD_STYLE = "loadStyle"
	CONF_LOW_POWER_REGION_FRACTION = "lowPowerRegionFraction" # reports
	CONF_MODULE_VERBOSITY = "moduleVerbosity"
	CONF_MPI_TASKS_PER_NODE = "mpiTasksPerNode"
	CONF_N_CYCLES = "nCycles"
	CONF_NUM_COUPLED_ITERATIONS = "numCoupledIterations"
	CONF_OPERATOR_LOCATION = "operatorLocation"
	CONF_OUTPUT_FILE_EXTENSION = "outputFileExtension"
	CONF_PLOTS = "plots"
	CONF_POWER = "power"
	CONF_RUN_TYPE = "runType"
	CONF_EXPLICIT_REPEAT_SHUFFLES = "explicitRepeatShuffles"
	CONF_SKIP_CYCLES = "skipCycles"
	CONF_SMALL_RUN = "smallRun"
	CONF_REALLY_SMALL_RUN = "reallySmallRun"
	CONF_STATIONARY_BLOCK_FLAGS = "stationaryBlockFlags"
	CONF_TARGET_K = "targetK" # lots of things use this
	CONF_TRACK_ASSEMS = "trackAssems"
	CONF_VERBOSITY = "verbosity"
	CONF_ZONE_DEFINITIONS = "zoneDefinitions"
	CONF_ACCEPTABLE_BLOCK_AREA_ERROR = "acceptableBlockAreaError"
	CONF_FLUX_RECON = "fluxRecon" # strange coupling in fuel handlers
	CONF_INDEPENDENT_VARIABLES = "independentVariables"
	CONF_HCF_CORETYPE = "HCFcoretype"
	CONF_LOOSE_COUPLING = "looseCoupling"
	CONF_T_IN = "Tin"
	CONF_T_OUT = "Tout"
	CONF_DEFERRED_INTERFACES_CYCLE = "deferredInterfacesCycle"
	CONF_DEFERRED_INTERFACE_NAMES = "deferredInterfaceNames"
	CONF_OUTPUT_CACHE_LOCATION = "outputCacheLocation"
	CONF_MATERIAL_NAMESPACE_ORDER = "materialNamespaceOrder"
	CONF_DETAILED_AXIAL_EXPANSION = "detailedAxialExpansion"
	CONF_NON_UNIFORM_ASSEM_FLAGS = "nonUniformAssemFlags"
	CONF_BLOCK_AUTO_GRID = "autoGenerateBlockGrids"
	CONF_INPUT_HEIGHTS_HOT = "inputHeightsConsideredHot"
	CONF_CYCLES = "cycles"
	CONF_USER_PLUGINS = "userPlugins"
	# Unused by ARMI, slated for removal
	CONF_CONDITIONAL_MODULE_NAME = "conditionalModuleName" # mcfr
	CONF_GROW_TO_FULL_CORE_AFTER_LOAD = "growToFullCoreAfterLoad" # mcnp & gui
	CONF_MEM_PER_NODE = "memPerNode" # unused?
	CONF_NUM_CONTROL_BLOCKS = "numControlBlocks" # dif3d
	CONF_REMOVE_PER_CYCLE = "removePerCycle" # fuel handler, equilibrium, mcnp
	CONF_USE_INPUT_TEMPERATURES_ON_DBLOAD = "useInputTemperaturesOnDBLoad" # th

In theory this should make it easy for us to change the variable name and have that permeated throughout the whole repo automatically. However, all around the codebase people seem to grab settings from the cs via the setting's actual name. Just a random example:

armi/armi/operators/settingsValidation.py

Lines 274 to 279 in 53ae82e

	self.addQuery(
	lambda: not self.cs["loadingFile"],
	"No blueprints file loaded. Run will probably fail.",
	"",
	self.NO_ACTION,
	)

We should instead always be pulling from the cs using the CONF_XXX variables. This means extra imports, yes, but not doing this has caused bugs in the past and is sure to do so again in the future.

[john-science]

My thoughts:

1. This is a good idea.
2. The only reason this problem exists is because someone got lazy each time.
3. This is low priority.
4. This is not API-breaking, and can be done very safely.
5. This can be done piecemeal, and broken into non-heart-breaking chunks.
6. I have seen this before, but was just daunted at HOW OFTEN this problem occurs in ARMI. Chris is 100% right to raise the issue.
7. Thanks, Chris!

[john-science]

A quick look at the size and scope of this problem:

• There are 168 module-level CONF_ variables defined in ARMI, for settings.
• They take place in 10 files:
  • armi/physics/fuelCycle/settings.py
  • armi/physics/fuelPerformance/settings.py
  • armi/physics/neutronics/const.py
  • armi/physics/neutronics/crossSectionSettings.py
  • armi/physics/neutronics/fissionProductModel/fissionProductModelSettings.py
  • armi/physics/neutronics/settings.py
  • armi/physics/thermalHydraulics/settings.py
  • armi/settings/fwSettings/databaseSettings.py
  • armi/settings/fwSettings/globalSettings.py
  • armi/settings/fwSettings/reportSettings.py

[john-science]

@bherlethtwo This ticket would be a good place to start at, as the work is maybe a little monotonous, but would be a quick intro to our tool chain and workflows.

[Till223]

Hello, I would like to contribute to armi. This seems like a pretty straight forward issue that I could get started on. Would that be alright?

[jakehader]

Hello, I would like to contribute to armi. This seems like a pretty straight forward issue that I could get started on. Would that be alright?

Hi @Till223, thank you! I think that we would definitely appreciate the support on this ticket / issue and it would help clean up the code and move away from using strings for setting accessing within the code base.

@john-science, any notes to add here? Is there any good way to "test" for this or enforce this that we could add or is just a style guide in the developer notes?

[john-science]

I would like to contribute to armi. This seems like a pretty straight forward issue that I could get started on. Would that be alright?

This would be a great place to start!

Just take one of the bullet points here and go to the file. Find all the CONF_ at the top, and then look for all instances of that string in ARMI, and replace it with the CONF_ variable. This will probably mean adding imports to other files, etc...

I will be your point-of-contact on this. Probably, since it is your first time contributing to ARMI, most of the work will be getting your environment setup and installed, and learning our GitHub workflows (opening a PR, etc). Don't worry if your PR has a lot of back-and-forth chatter and requested updates, it happens to EVERYONE. People are just trying to be careful and thorough.

Thanks!

[john-science]

Is there any good way to "test" for this or enforce this that we could add or is just a style guide in the developer notes?

There isn't any standard practice for ensure tight use of string constants, no. But once we get the items in this bullet list solved, we will probably add a note in the PR docs or the Dev docs.

[Till223]

Alright, I will start working on this then, thanks for all the pointers! If I have any trouble with the setup I'll message you directly @john-science

Is there any good way to "test" for this or enforce this that we could add or is just a style guide in the developer notes?

Couldn't you use a github action to check for this?

[jakehader]

Maybe a regex check could be used in the code base as a high level unit test within an ARMI application? Thanks @john-science and @Till223!

[mgjarrett]

I fully acknowledge that it is more appropriate to use the defined constants than the literal strings throughout the codebase. I've been bitten by bugs from typos in the setting name. And of course, it's possible to miss some instances of the string when the name of a setting is updated. So there are plenty of benefits, as stated above. So, yes, we should absolutely do this.

But I do want to give voice to the other side for a minute, just to put it out there. Many ARMI developers are also ARMI users. ARMI users usually know the string names of all the settings that they need to use in whatever code they're developing. Nobody knows off the top of their head the CONF_XXX constant name for each setting. So whenever you're developing code, you'd have to constantly cross-reference the files where the CONF_XXX are defined to make sure you're using the one you intend. And if you're working with a lot of settings concurrently, there is potential for mixing up which setting is which. This could lead to a bug, and once it's in there it would be arguably harder to identify than a typo in a string. You would have to infer that the person who wrote the code intended to use a different setting.

Again, I still think it's the right direction, but I would contend that it's not a clear, no-brainer decision.

[keckler]

@mgjarrett I definitely know what you're referring to with your comment. I think that issue typically might arise when there is a discrepancy between the string name and the CONF name.

Like a setting nCycles should be directly translated to CONF_N_CYCLES. But sometimes people make the CONF name something other than the direct translation, for instance they might do CONF_CYCLES.

I think if we were rigorous about translating the setting string names into those CONF variables, there would be no (or less) confusion.

[john-science]

@mgjarrett I mean, you might not know the CONF_ variable name off the top of your head. But the codebase is a decade old, we have had the time to go back and do some search/replace.

I think the only real concern I have about implementing the above is we might accidentally create some import loops, by adding so many from armi.whatever import CONF_X at the top of a ton of files. We'll see how that goes, I think I already spy one I will have to disentangle.

[albeanth]

@mgjarrett quick shameless plug for vscode here. If you use the CONF_XYZ approach, you can hover your cursor over the name and vscode will tell you the string it correlates to. While its displayed, you can even copy-paste the string as you need. And you can alt-click to jump straight to the definition in the right settings file. It's pretty nifty.

[onufer]

now that these are more widely used we need to update our internal repos to use confs also. Before it was 95+% of the time you would grep for the string and it would find all instances. . now its 50/50 depending on if its in the framework or not

[john-science]

Thanks @Till223 !