[Documentation] ResultChannels and Sinks

[hartytp]

ndscan has pretty good API documentation, but is lacking a bit of the bigger picture "how should I think about this?" stuff.

I'm going to use this issue to post a few bits of a conversation (these aren't directly suitable for copying into the docs, but putting here as a note to self about some of the points that weren't obvious to me) that @dnadlinger and I had recently, with the intention of spinning up a PR when I have time to add this into the ndscan docs.

A fragment is not allowed to care about what happens to its results any more than a function can’t care about its return values. This allows for flexible/optimised execution of scans, subscans, etc.
In other words, a fragment is “morally” really just like a function call (though the implementation is necessarily different, as one e.g. can’t return arrays in ARTIQ Python)
self.my_result_channel.sink should never be accessed in fragment code.
The proper design is to take care of whatever needs access to all values “one layer up”, i.e. in the code that runs the fragment as a (sub)scan, etc.
For some instances where one combines the results of several subfragments (say, sideband thermometry), this could be simplified by adding functionality to “wrap” all the result channels of a given child fragment. I think there is a discussion on an issue somewhere.

...

Having a way to absorb the result channels into the thermometry fragment, effectively doing self.{r, b}sb.p.sink = LastValueSink() such that it is under control of the thermometry fragment. The original results can still be re-exposed as result channels in the parent fragment if desired

...

Maybe the best way of putting it is that channels/sinks are push-based, rather than pull-based. There conceptually isn’t a storage location to refer back to; the values only live as long as the push() call. The sink is just a way from the outside, if you want to put it like that, set what the push() function that is called actually is. There does not need to be a sink at all; it could be None. Also, note that the ResultsSink interface only has a push() method, so calling get_last() without having set the sink yourself isn’t allowed, as you’d need to, using the parlance of a strongly typed language, downcast to a concrete implementation having that method. Accessing results through the API of a fragment would be like wanting to directly ask a function what the return value of an earlier call was, rather than saving them yourself.

Now – sticking with the function terminology – for subscans the (implicit) scan runner is of course the caller, and there is nothing wrong with the subscan code keeping track of the callee’s return value to make accessible after the fact. Client code using subscans just shouldn’t be using the sinks on the re-exported results channels for that (as usually set by whatever higher-level scan runner); that would be like a function trying to forward-reference its own return value slot somehow.

The point behind why “absorbing” the result channels is cleaner is also easy to explain in the analogy to function calls. This would be like function A calling function B in its implementation. If the implementation wants to access the return value of B, it’d better assign it to a temporary value, rather than saying return b(). (We have an auto-magical system for collecting all the return values across the fragment tree, of course, such that the default state for experiment code is for everything to be observable.)

So yes, it should always be “absorbed”, but the subscan runner could do that automatically, or there could be tools in ndscan around that.

Having push-based output interfaces isn’t exactly an uncommon pattern in software in general (though splitting results up into automatically aggregated scalar channels with their own push() calls instead of a big tuple is just a mess, but that’s because of ARTIQ compiler limitations).

One particular reason for the push-based design was also that on the kernel, we might not have any place to store results in at all (e.g. when pushing a list, not even a pre-allocated memory slot would do; the value can only live within push()).

Currently, push() itself is already an async RPC, but the design intent was definitely to make this usable on-kernel too. I think I just ran into compiler bugs making the polymorphism work there.

I also want to support things like persistent kernels (where a fragment is launched in the background to allow then invoking it from straight-line control flow on the host side with low latency), where on the fragment side there is similar no meaningful concept of “last value”.

[dnadlinger]

Thanks for copying it over – as I mentioned, it would be a good idea to try to keep such discussions in a public place to begin with (though they'd still need to be crystallised into actual documentation). To that end, I've also activated the GitHub Discussions feature for this project, though I have little experience with it.