Having KatSpec as a singleton seems to cause random test failures
Approximately 6 months ago I changed KatSpec
to use a singleton pattern to make it easy to register new LIGO-specific components in finesse-ligo
and have them be globally available. The changes I made back then however seem to have created some intermittent failures in the test pipeline when it gets run in a random order (see failed random order jobs on git.ligo.org pipeline). The reason is likely that the mess some of the parser tests make to the KatSpec
object isn't fully cleaned up before the next test. There is code in the test suite to delete the singleton in the datastore so that it gets freshly reinstantiated upon the next test, but there's probably some reference hanging around in one or more tests that's causing havoc. This sort of problem is a common reason why lots of organisations (e.g. Google) discourage use of singletons.
Thinking again about this today in the context of this comment on issue #476, I wondered whether we should ditch the singleton pattern and just rely on Python's import system not re-importing something that's already been imported. We could change KatSpec
in finesse.script.spec
to _KatSpec
(to discourage direct import in scripts) and define KAT_SPEC = _KatSpec()
there. Then scripts wishing to use or register components in the spec would import the shared KAT_SPEC
object. Simple!? Not sure why I didn't think of doing it this way before - there could well be a good reason, but I can't remember. At very least it makes it a bit more difficult for the test suite, which needs to reset that imported object between tests in case the previous test has changed it (pytest doesn't trigger re-imports of any modules between tests), but this is exactly what pytest's monkeypatch tools are for.
In any case, it's worth someone (me) making a branch to test this approach - it's not nice knowing the current implementation has a subtle memory leak that will rear its head at some point sooner or later...