This story is a partially therapeutic exercise for the author, and partly a serious attempt to document how bad our Swig technical debt currently is (and some of the trade-offs involved in working around it).
I’m trying to add a new measurement algorithm on DM-5197; this is essentially a reimplementation of an HSC-side algorithm that moves it to a new package and changes the optimizer it uses. The simplest way to do that (for a C++ class, like this one) is to subclass meas::base::SimpleAlgorithm
, which lets you define both a SingleFramePlugin
and a ForcedPlugin
at the same time.
The trouble begins when I discover in testing that the ForcedPlugin
functionality isn’t working, because the measureForced
method of the class (which should have been inherited from SimpleAlgorithm
) isn’t visible in Python. That’s weird; this pattern “just works” all over the codebase. After lots of poking around, I discover that the problem is trivial: my new algorithm class doesn’t actually inherit from meas.base.SimpleAlgorithm
in Python (it just inherits from object
).
Now, Swig can be partially excused for this, because there is user error involved: I hadn’t done an %import "lsst/meas/base/baseLib.i"
, so lacking the information necessary to produce a correct wrapper, Swig decided that the best option was to silently ignore the problem.
Still, mostly my fault. And it’s entirely possible that there’s some Swig pragma I could set to tell it to warn (or even some pragma we’ve already - misguidedly - put somewhere else to tell it not to warn). So maybe it’s not even Swig’s fault there was no warning - but the bottom line is that we don’t get a warning in our build environment right now, and that just wasted way too much of my time.
Anyhow, one would think the natural solution to this would be to just add %import "lsst/meas/base/baseLib.i"
. And that’s a start - of course, one would prefer to just add something like %import "lsst/meas/base/Algorithm.i"
, since we just want the one class. But that’s impossible, given that I want to inherit from the class. I need to %import
the master .i file because it’s the only one that contains the %module
statement, and Swig needs that to do the Python side import. This is at least sometimes not necessary if you’re just using the class, rather than inheriting from it.
Resigned to importing the whole module, I also have to #include "lsst/meas/base.h"
- every header in the meas_base package - in the Swig wrapper, or the generated code will fail to compile. That will slow down my compile times, but it’s something I’m used to.
But that isn’t enough - I’m now seeing compiler errors in involving afw::math
and afw::detection
classes when building the wrappers. It appears that lsst/meas/base.h
ought to include both lsst/afw/detection.h
and lsst/afw/math.h
, since pulling in the meas_base module via an %import
apparently requires those as well - at least in this context (and compiling Swig modules is essentially the only reason we have those per-package headers anymore, though I don’t think we’ve really codified that anywhere). I’m loathe to do that, because there might be other packages that have somehow gotten away with %importing
meas_base without including those afw catch-all headers, and I don’t want to slow down those builds. On the other hand, if I don’t update “lsst/meas/base.h” other people will probably run into this again in the future. Probably better to modify the header, I think, as it’s not really that likely that importing meas_base works without the afw headers in some other context. I certainly don’t have time for an experiment.
I should clarify that much of this problem is clearly ours, because we’re (in hindsight) not using Swig as well as we could: if we refactored all of our Swig code to put ~1 class in each compiled module, instead of a whole package, I wouldn’t have had to import all of meas/base/baseLib.i
or include all of meas/base.h
. And that probably would have removed the need for the other afw headers. But getting from here to there is still a lot of work, and until we do we’ll continue to waste developer (and user) time building code we don’t need, producing godawfully large binaries as a result.
Anyhow, that’s the end of the story: I add the %import
to meas_modelfit, and add the extra afw headers to meas/base.h
, and resign myself to the fact that I’ve just spent four hours making one package build more slowly for the sake of a single class’s inheritance.