Enforcement of ARMI app version with metdata:versions

[drewj-usnctech]

Some reading of #1064 // #1064 has me curious about the metadata:versions setting. Digging through the settings implementation, the version from the file is attached as inputVersion

armi/armi/settings/settingsIO.py

Line 222 in 9d6a405

self.inputVersion = tree["metadata"][Roots.VERSION]

and self.liveVersion is set to armi.meta.__version__

But, if they differ, there isn't an explicit error. Any mismatch is noted if there is an invalid setting if an InvalidSettingsStopProcess is raised

armi/armi/utils/customExceptions.py

Lines 103 to 122 in 9d6a405

	class InvalidSettingsStopProcess(SettingException):
	"""Exception raised when setting file contains invalid settings and user aborts or process is uninteractive"""
	def __init__(self, reader):
	msg = "Input settings file {}".format(reader.inputPath)
	if reader.liveVersion != reader.inputVersion:
	msg += (
	'\n\twas made with version "{0}" which differs from the current version "{1}." '
	'Either create the input file with the "{1}", or switch to a development version of ARMI.'
	"".format(reader.inputVersion, reader.liveVersion)
	)
	if reader.invalidSettings:
	msg += (
	"\n\tcontains the following {} invalid settings:\n\t\t"
	"{}"
	"".format(
	len(reader.invalidSettings), "\n\t\t".join(reader.invalidSettings)
	)
	)
	SettingException.__init__(self, msg)

which is created in SettingsReader._checkInvalidSettings

armi/armi/settings/settingsIO.py

Lines 226 to 244 in 9d6a405

	def _checkInvalidSettings(self):
	if not self.invalidSettings:
	return
	try:
	invalidNames = "\n\t".join(self.invalidSettings)
	proceed = prompt(
	"Found {} invalid settings in {}.\n\n {} \n\t".format(
	len(self.invalidSettings), self.inputPath, invalidNames
	),
	"Invalid settings will be ignored. Continue running the case?",
	"YES_NO",
	)
	except RunLogPromptUnresolvable:
	# proceed with invalid settings (they'll be ignored).
	proceed = True
	if not proceed:
	raise InvalidSettingsStopProcess(self)
	else:
	runLog.warning("Ignoring invalid settings: {}".format(invalidNames))

So long as I don't have any invalid settings in my different ARMI version, then I would never be alerted that my version differs from the one specified in the settings file.

The second question that follows is making this application specific. Is this something that could be picked up by the app version? Our ARMI application has different version numbers from ARMI, so if we wanted to leverage this feature, we would want to tie the version from the input not to the version of the armi library, but to our ARMI application

[john-science]

Yeah, the "metadata" version was put there for legal/process reasons. We don't really use or enforce it heavily. It is interesting that the warning isn't even printed out though.

Do you think we should use or enforce it strongly?

On occasion, people ask me about the versions of their Plugin, Interface, or App, and if they can track all of those in ARMIs code. While certainly anything is possible, not everyone imports and runs ARMI from an "App" (for instance, if you are using ARMI just to open up a DB from a previous run). But mostly I just think it seems inside-out for ARMI code to do version checking for code it can't see in some other repository. But, perhaps, that's just my gut feeling.

What do you think of this solution?

Perhaps your Plugin / App could have it's own bespoke logic for handling versions:

from myapp import __version__

VERSION_SETTING = "myPluginVersion"


class MyPlugin(ArmiPlugin):

    @staticmethod
    @plugins.HOOKIMPL
    def defineSettings():
        return [
            Setting(
                VERSION_SETTING,
                default="uncontrolled",
                label="stuff",
                description="things",
            )
        ]

    @staticmethod
    @plugins.HOOKIMPL
    def defineSettingsValidators(inspector):
        return [
            settingsValidation.Query(
                lambda: inspector.cs[VERSION_SETTING] != __version__,
                "Oh noes! The MyPlugin version doesn't match!",
                "Do you want to force the version to match?",
                lambda: inspector._assignCS(VERSION_SETTING, True),
            )
        ]

The above settingValidation being in your own repo also gives you a lot of power to handle versions that are meaningful to you. For instance, maybe all you care about is that the major version matches: "4.x", and you could add some validation to allow "4.1.3" to be fine. Or all over your plugins could validate their own versions, to stop your developers from having mix-and-match version issues.

I'm just spit-balling here @drewj-usnctech . Thoughts?

[drewj-usnctech]

Should I take away from this that metadata:version is a setting that shouldn't be used? I understand needing at least to parse it and put it in input files for QA / record keeping. But, from a user's perspective, it doesn't do what it's advertised to do.

But, to your point, to make ARMI do version enforcement on things that are outside it's scope is not a fair ask. On the assuming y'all have an ARMI application built off ARMI, likely w/ it's own versioning scheme too, would it make sense to allow plugins / apps to define validators for this metadata section? So the settings file could look something like

metadata:
  framework version: 0.2.5  # ARMI version
  custom app version: x.y.z

and then it's the responsibility of the custom app to validate x.y.z

Version validation is finicky because, like you pointed out, what if we need to force a looser version 4.x vs. a more explicit version 4.9.1? I would say that it depends on the intent behind putting versions in a settings file in the first place. If it really is intended to enforce that a case cannot be ran without a specific version, then literal matching would suffice in my book.

In the spirit of ARMI plugins, I really like the idea of allowing apps / plugins to do their own validation. More trying to nail down where the user is expected to put this. Using your example, if someone read the docs, they could have a settings file like

metadata:
  version: uncontrolled  # to avoid ARMI errors
settings:
  myPluginVersion: 4.9.1

which feels weird? If someone was reading this without the knowledge of what metadata:versions and settings:myPluginVersions meant, what should they take away?

[john-science]

Give me a hot minute to think about this.

IF we're okay with enfocing the 1.2.3 versioning for ARMI, and related plugins/apps, then I could write some helper functions that could easily tell if 4.1.3 meets the requirement 4.1, and similar.

Then we could have any plugin add a new setting (say to the metadata section) and a validator that just calls my function.

That would mean that ARMI would contain the code to do what you want, but ARMI wouldn't be responsible for understanding outside plugins, and it would be the plugin developer who would be responsible. I think that would meet your needs.

Our current solution to this problem, I suppose, is that the ARMI App we create points to ultra-specific commits to each plugin (in its own repo), including a repo for data files. And we have CI to test that all these versions/commits work together. So I believe the trustworthiness of that process is WHY we don't have this feature you suggest all flushed out. But maybe just adding a couple of helper methods for you would be low-effort enough I might as well do it.

Thoughts?

[drewj-usnctech]

I'm very much on board for

Then we could have any plugin add a new setting (say to the metadata section) and a validator that just calls my function

That would mean that ARMI would contain the code to do what you want, but ARMI wouldn't be responsible for understanding outside plugins, and it would be the plugin developer who would be responsible

we could then have our "primary" plugin do the validation on our app version, ARMI validates ARMI's version, and any other plugins (hosted outside our "core" app repo) could do their own version validation

[john-science]

Okay, this seems like a light-weight solution to me.

I opened a ticket to build said helper function.