Update sequence on ExecutionEventAdded events #4547
Conversation
| .raiseGraphQLErrors | ||
| .map(_.observation.flatMap(_.execution)) | ||
| .attemptPot | ||
| .reRunOnResourceSignals: |
There was a problem hiding this comment.
I switched from resetOnResourceSignals to reRunOnResourceSignals so that the Pot doesn't go to Pending between the signal and the query results. This was causing the table to reload for each step, which wasn't very nice when getting updates from the observation execution.
BundleMonFiles updated (2)
Unchanged files (6)
Total files change +4.03KB +0.16% Final result: ✅ View report in BundleMon website ➡️ |
| for { | ||
| o <- obsEditSubscription(obsId) | ||
| e <- executionEventAddedSubscription(obsId) | ||
| } yield o.merge(e) |
|
Do we still need the Also, can we filter |
Showing acquisition steps is a bit tricky, since there's always a next atom. We have to detect whether it's relevant to show it or not. IIRC, there's logic in observe to do that. There might also be logic here and it may be outdated. Anyway, we can fix that in a following PR. |
We still need to update when the observation changes and we aren't executing. As I mentioned above, we do seem to get a lot of observationEdit events during the execution as well. It still isn't clear to me what gaps the executionEventAdded events fill. Unfortunately, my knowledge of how the sequences and execution work is very limited. |
That's a good idea. It might help with the overlap of observationEdit and executionEventAdded events as well. There is a flood of executionEventAdded events! The events we can filter on are: SEQUENCE, SLEW, ATOM, STEP & DATASET. Would just listening to |
I think that we will be OK just listening to We might want to react to Step start if we want to show which step is currently executing, but I'm not sure that's a requirement. |
| ctx <- useContext(AppContext.ctx) | ||
| given StreamingClient[IO, ObservationDB] = ctx.clients.odb | ||
| obsEditSignal <- | ||
| useResourceOnMount: // Subscribe to observation edits |
| val document = s""" | ||
| subscription($$input: ExecutionEventAddedInput!) { | ||
| executionEventAdded(input: $$input) { | ||
| subscription($$obsId: ObservationId!) { |
There was a problem hiding this comment.
FWIW, a while back I switched to always creating the Input objects in scala because of better type checking. We had an instance of the schema changing and explore compiling just fine, but breaking in dev. Maybe with the schema becoming more stable that will be less of a problem. Also, I guess this particular input probably won't change much.
There was a problem hiding this comment.
Actually, we should be able to check the input against the schema in clue, like we do with the query: gemini-hlsw/clue#665
There was a problem hiding this comment.
That would be great!
Thank you! I'm still working on the PR. Turns out that executing a stream twice is not a good idea, at least when they are queue-backed, like the ones that clue returns. By executing the stream twice, each element ends up only in one of them, since they steal it from the queue. I implemented a way to create streams that replicate an input stream, via topics. Also, there are new handy methods in crystal that I'm using, but the crystal PR is not ready yet. |
rpiaggio
force-pushed
the
sc-4317-show-steps-executed-after-events-are-emitted
branch
from
691f7a5 to
d2c4e56
Compare
|
I ended up inverting the logic. Instead of building update streams for the visits and the sequence, I switched to manually updating them. The Also, the logic to keep the sequence updated has been moved to a custom hook. |
|
I'll merge this for the moment. We are at a point where the sequence is updating on every obs and target edit, and both the sequence and visits are updating whenever a step completes. There are still some glitches regarding the acquisition sequence. Some of these are present in observe too. I'll address those in a separate PR. Also, we are getting a ton of events whenever a step completes. I'll monitor this a bit more and sync up with Shane. |
One thing we are missing here (unless I failed to see it) is a debounce for the updates. With the old methods the "signal" streams had a 2 second (by default) debounce. We could debounce each of the subscription streams here and that would help a lot, but that still means that if we get both an observation edit and an event added event we will query twice. And we seem to get both events quite frequently. The first thing I can think of seems complicated, but you'll probably come up with something simpler. The obs edit and event added streams could update a queue and that could be debounced and used to update the sequence and visit. I don't think the target edit events are as important to sync up because, AFAIK, they only come about from editing the targets. |
|
Good catch. I'll look into that before merging then. I'm thinking maybe we can merge the three signals into a Boolean stream which indicates whether the visits must me updated (the sequence always is). |
|
This might need further work, but I think a got a version working where update signals are throttled. I ended up using Arman's Since we had utils a bit scattered, there's also a refactor that brings all utils extensions into one file. |
|
BTW, I didn't use |
|
|
||
| import cats.effect.Temporal | ||
| extension [F[_]: Temporal, A](stream: fs2.Stream[F, A]) | ||
| def throttle(duration: FiniteDuration): fs2.Stream[F, A] = |
There was a problem hiding this comment.
Isn't this extension in extensions.scala, too?
There was a problem hiding this comment.
Yup, forgot to remove it when I moved it there.
Anyway, I'm deleting from both places since we are not using it after all.
rpiaggio
force-pushed
the
sc-4317-show-steps-executed-after-events-are-emitted
branch
from
2dc14fc to
c0a96ca
Compare
rpiaggio
deleted the
sc-4317-show-steps-executed-after-events-are-emitted
branch
At first I tried getting the visit information with the subscription event and merging it in with the other visits. But, we ARE getting a lot of observationEdit events while the observation is executing, so it made more sense to just include it in with the other signals because those are debounced. There are also a LOT of executionEventAdded events, so the debouncing is good.
This seems to work well, with one caveat. While an observation is executing (for at least one of my observations), an extra set of acquisition steps appears in the sequence, as though we have moved on to a new visit. After a couple more science steps complete, those acquisition steps disappear and everything finishes up normally. I have not determined if those are actually in the sequence we get from the ODB, or if it is some sort of local aberration. They do not appear in the observe UI.
Looking at the first commit it is easier to see the actual changes, since in the second commit I refactored to the monadic hook style.