Re: [BuildStream] Responsive, but not overly verbose UX - not-in-scheduler

[Tristan Van Berkom <tristan vanberkom codethink co uk>]

Hi Jonathan,

> Hi Tristan,
>
> I've taken some time to discuss the implementation of separating the UI 
> from the scheduler 
> > (https://gitlab.com/BuildStream/buildstream/issues/1036), and look at 
> the information that the frontend uses that will no longer be 
> accessible.
>
> Following on from that, I now have a basic plan for how to keep the 
> information required for the UI separate from internals.
> This basic plan is currently just for how to stop the UI from reading 
> the scheduler directly, the next step would be to extend this to storing 
> element state (in _frontend/widget.py we look up Elements in the global 
> state, which won't be available if we fork before loading elements).

Thanks looking at this again.

In the part about element state, it is expected that element state
parts which are essential for messaging will be encoded directly into
the Message objects (this solves both the problem of actually obtaining
the state to log, and also the problem of time skew across processes;
we want to log the state of the element *at the time a log message was
issued*, not at the time it was received in the frontend).

That said, this part of serializing and transferring state I think is
orthogonal and can be done at a later time (either in a later MR after
this one, or as part of the multiple MRs involved in separating the
scheduling process from the frontend process).

> 1. Store all state that the UI needs in a single place.
>
>     To this end, I plan to create a _frontend/uistate.py file, which 
> contains the classes `UIState` (stores all state, owned by App and 
> passed to Status and LogLine so they can read the state), `TaskGroup` 
> (as a proxy for queues, stores its name, and lists of active, processed, 
> skipped and failed tasks), and `Task` (a replacement for the 
> _StatusJob).

I think this is correct, but I don't want this to be in _frontend/.

Rather, I want this to be owned by the core - think of it this way: 

   "This is state which the core offers to any frontend."

Whether or not we eventually develop other frontends than our CLI
frontend, it makes sense to separate the frontend and core this way, so
that Stream is a thin API surface library which allows any frontend to
be easily developped.

So essentially yes, lets manage state some state in our Stream library
for the sake of frontends, but:

  * Lets refer to this as state, not UI state - the core need not know
    exactly what a frontend might do with this state.

  * Lets keep this state in the core as a frontend facing API, probably
    you should be able to *reach* it through Stream() (that is not to
    say it should be implemented *in* _stream.py specifically, just
    that there should be a way to *obtain* this state from/through
    APIs on Stream()).

Does this separation make sense ?

> 2. Define how to change UI state.
>
>     We'll get pretty far by rewriting App._job_started() and 
> App._job_completed() to use the UIState instead.
>     However, since we're now storing which jobs were skipped, processed 
> or failed, we should add to processed or failed depending on the job's 
> status.
>
>     We'll need to add a _job_skipped_callback() to report to the frontend 
> when a job was skipped, since AIUI a skipped job doesn't call 
> _job_started().

That sounds reasonable, but I don't know that this callback needs to be
reported all the way to the frontend.

I.e. the information that the core wants to convey is more about "How
many jobs were skipped in the Fetch queue", not fine grained
information about the jobs themselves (there are explicit messages sent
to that effect anyway for logging purposes).

Instead, I think what we might want is for the Stream to offer a sort
of observer model (where the frontend can register callbacks for
changes in certain subjects, like queue statistics), and we'd only want
to provide to the frontend:

  * A means to read the current number of skipped/processed/failed
    tasks for a given queue
  * A means to register a callback to be notified when those
    statistics change

>     Also, Stream._add_queue() seems to be a good place to add a call to 
> the frontend to create TaskGroups. To that end, I'll add a 
> queue_create_callback that's passed into Stream and called.

Right, just my own little peeve about wording things in the right
way... lets: Lets remember that the Stream does not "call to the
frontend", it does not know or assume that anything such as the
frontend exists at all, it provides a library surface for applications
like the one in _frontend/ to be developped (whether that is public or
not, it's currently private).

Essentially what you are saying worded differently makes sense, but
wording it this way helps to spread clarity through how we comment out
code and how we explain things to new developers, etc.

That aside... note I am not sure that it is exactly correct that task
groups should be notified at that time, or whether we should have a
different API, like a "task groups changed" notification which is
issued directly before the core enters Scheduler.run()... then we might
want to notify final statistics after Scheduler.run() completes before
notifying that "task groups changed" again to clear the groups... and
prepare the internal state for subsequent runs of Scheduler.run().

Either approach may work fine, but a single notification seems more
convenient for the frontend to receive (regardless of frontend
implementation, it also means less processing / adjusting of screen
resources etc).

> Fingers-crossed, I'll be able to put something together that makes sense 
> and have something concrete to show off soon.

Thanks again for thoroughly reading through this thread, I'm quite
interested in this refactor, feel free to try to yank my attention on
IRC at any time I'm available to brainstorm :)

Cheers,
    -Tristan