Explore immutable settings

[john-science]

Currently, there are many places in the codebase where we call getMasterCs() and read in all the settings. Slightly more troubling, there are many places in ARMI and in the plugins where we change settings mid-run.

Purpose: explore how much havoc would be caused by making settings immutable.

Method: change the settings API a bit to raise a DeprecationWarning every time a setting is altered. Then test various real-world use cases to see how often (and where) this warning pops up.

Results: report back on the prevalence and where these warnings pop up.

[john-science]

@youngmit Could you assign this to me, please? Thanks!

[youngmit]

Done!

[john-science]

Before I go about the business of refactoring our code to make the settings immutable, I have outlined below all the places in the ARMI framework we mutate settings.

From this list, I will generate all the sections of the codebase and workflows that need to be refactored.

Num	File	Method	Is Valid Mutation?
0	armi/bookkeeping/snapshotInterface.py	SnapshotInterface.activateDefaultSnapshots()	Y
1	armi/cases/case.py	Case.writeInputs()	N✚
2	armi/cases/inputModifiers/neutronicsModifiers.py	NeutronicConvergenceModifier.__call__()	Y
3	armi/cases/suiteBuilder.py	SuiteBuilder.buildSuite()	Y
4	armi/operators/operator.py	Operator.setStateToDefault()	N✚
5	armi/operators/settingsValidation.py	Inspector._inspectSettings()	N✚
6	armi/operators/snapshots.py	OperatorSnapshots.setStateToDefault()	N✚
7	armi/physics/fuelCycle/settings.py	getFuelCycleSettingValidators()	Y
8	armi/reactor/converters/parameterSweeps/generalParameterSweepConverters.py	SettingsModifier.convert()	Y✚
9	armi/reactor/converters/parameterSweeps/generalParameterSweepConverters.py	NeutronicConvergenceModifier.convert()	Y✚
10	armi/scripts/migration/m0_1_0_settings.py	_modify_settings()	Y
11	armi/scripts/migration/m0_1_6_locationLabels.py	_modify_settings()	Y
12	armi/settings/caseSettings.py	Settings.loadFromInputFile()	Y
13	armi/settings/caseSettings.py	Settings.loadFromString()	Y
14	armi/settings/caseSettings.py	Settings.temporarilySet()	N
15	armi/settings/caseSettings.py	Settings.unsetTemporarySettings()	N
16	armi/settings/settingsRules.py	addToDumpSnapshots()	Y

NOTE: ✚ These should use the new Settings.modified().

NOTE: There are several places where we do cs.caseTitle = "scripted-case". My working assumption is that since caseTitle is not a setting, but an instance attribute, these are all okay. Tell me if I'm wrong.

[youngmit]

1 is one of those places where it should probably be making a new settings object with the new settings. Especially since it's making some decisions about where the blueprints and geom file [theses are borderline defunct and probably need deprecated, too, at some point. going to make an issue to this effect].

4: Not even sure what we need this for. Very odd... 6 is doing something similar. I am suspicious of this pattern.

5: The whole point of settings validation is to do a bunch of plugin-extensible checks and massage settings before they are even really interpreted for real. I still think the semantics of the settings validation should be that a Settings object A goes into the validation box and a new Settings object B pops out the other side.

8, 9 are fine, since the whole point is to twiddle settings from a base input and generate new inputs. As with validation, i think the fully-immutable approach makes the most sense here; the modified settings should be completely new objects, and the "base" Settings object should never be changed.

12, 13 is where settings come from in the first place, so deffo need to be able to modify/initialize the state of the settings.

14 is bad. This is the whole antipattern of settings modification, given a name. At least it will undo its modifications, but anywhere it's being used I would bet money there's another, cleaner way to do it. Either by not actually trafficking in Settings at all, or by making a new, temporary Settings object with your modified settings, then throwing it away when you're done. If there are global side effects of the temporary setting modifications, refactoring around that might be a bit tricky, but pretty high priority.

16. Settings rules are essentially another approach to the Inspector settings validation, so also pretty fine. "dump snap shot" is a list of CCCNNN strings specifying cycle (C) and node (N) to perform snapshot analysis on. Providing one to the user when it's not specified is probably not a great idea anyways.

[john-science]

4: Not even sure what we need this for. Very odd... 6 is doing something similar. I am suspicious of this pattern.

Fun Fact: These two methods setStateToDefault are not used in ARMI. Though there are ~32 places in the code that assumes cs["runType"] exists, and depends on the value. It is, apparently, a very important setting, with an entire file devoted to providing the options (armi/operators/runTypes.py).

Okay, turns out they are subclassed and used in three different plugins. Looking at it, I can see why runType is used, but not really why people need to be able to change what's in the YAML file. This seems like the exact use-case for the new .modified() method.

Thanks for all your input!

[john-science]

A note on progress. We are 75% of the way through making settings immutable. Much progress was made.

The major thing left to do is remove the fact that someone sneaky can still do: cs['mySetting'] = 1 as long as cs['mySetting'] already exists and has some value.

I initially thought this would be a minor change, but whoa was I wrong. This breaks half the unit tests in ARMI, and I have personally seen a dozen plugins that depend on this functionality. (Though I believe at least part of the reason for this could be solved by removing all settings that only belong to one plugin. That would be a big lift on it's own though.)

[john-science]

In the past ~9 months, the changes we've made to making Settings more immutable have stabilized and become standard.

They are not 100% immutable yet, sadly. As making that happen would require a hard backwards-compatibility-breaking rewrite of a huge swath of the codebase. And stopping people from doing things in Python is notoriously hard.

So what we have now appears to be the accepted middle-ground. And while I will always keep my eye on this, I will close the ticket for now.