What I've Spent the Last 4 Hours Debugging: A Rant

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.

1 Like

Sounds like now would be a good time to do what @RHL has long threatened — get a consultant to advise us on how to use SWIG more effectively.

1 Like

If the advice comes cheap enough, I’m all for that; I have no doubt there’s quite a bit we could learn. But I’m very skeptical of the related proposal that we could get a consultant to actually fix our code for us, and if we’re going to spend time fixing our code ourselves, we should also consider spending that time switching to something other than Swig.

No doubt you’re thinking of a “something other than Swig” that would allow us to make the python more pythonic at the same time…

I’ve been following DMTN 13 and 14 with interest - is it too early to ask whether there are conclusions to be drawn from the work behind them?

I thought the apparent reference to K-pop was strange until I found you meant DMTN-013 and DMTN-014

1 Like

It’s certainly too early to draw conclusions from them, as we want to do a bit more review on the solutions before we try to use them to evaluate the wrapper tools. But you’re certainly welcome to participate in those reviews if you have some thoughts about the details.