[KED-3002] Live update runs list from subscription #703
Conversation
limdauto
force-pushed
the
feat/subscription
branch
from
e786b89 to
518ad68
Compare
limdauto
requested review from
studioswong,
tynandebold,
rashidakanchwala and
antonymilne
|
@tynandebold thanks. I have adjusted the PR based on your comment. |
| yield [ | ||
| format_run( | ||
| run.id, | ||
| json.loads(run.blob), | ||
| data_access_manager.runs.get_user_run_details(run.id), | ||
| ) | ||
| for run in new_runs | ||
| ] |
There was a problem hiding this comment.
Why not use format_runs here?
There was a problem hiding this comment.
yes, such great point. I forgot.
| : window.location.host; | ||
|
|
||
| const wsLink = new WebSocketLink({ | ||
| uri: `ws://${wsHost}/graphql`, |
There was a problem hiding this comment.
If I go to ws://localhost:4141/graphql does that do anything interesting?
There was a problem hiding this comment.
no, not in dev. no graphql server there.
| self._db_session_class() # type: ignore | ||
| .query(RunModel) | ||
| .order_by(RunModel.id.desc()) | ||
| .all() | ||
| ) | ||
| if all_runs: |
There was a problem hiding this comment.
When does get_all_runs get called? Just wondering why this is the right place to update the last_run_id.
There was a problem hiding this comment.
e.g. it already gets updated in runs_added.
There was a problem hiding this comment.
It's the first call when user visits experimentation tracking tab.
| @@ -71,6 +76,16 @@ def get_user_run_details(self, run_id: str) -> Optional[UserRunDetailsModel]: | |||
| .first() | |||
| ) | |||
|
|
|||
| @check_db_session | |||
| def get_new_runs(self) -> Optional[Iterable[RunModel]]: | |||
There was a problem hiding this comment.
Minor: is there some neater way of writing this or combining with get_all_runs, given that when self.last_run_id is None, it's identical to get_all_runs. Do we want two separate functions, get_all_runs and get_new_runs rather than something like get_runs(new_only=False)? No problem with how it is now though, just wondering.
There was a problem hiding this comment.
I prefer to keep these interfaces separate in case we need to add more logic to each use case. It's an old habit, e.g. if we end up caching all runs somewhere we won't need to go to the DB to fetch them. We will almost always have to do it for new runs.
RELEASE.md
Outdated
| @@ -11,6 +11,7 @@ Please follow the established format: | |||
| ## Major features and improvements | |||
|
|
|||
| - Create the toggle-bookmark journey that allows bookmarking runs and displaying them as a separate list. (#689) | |||
| - Auto-update experiment runs list whenever user has a new Kedro run (#703) | |||
There was a problem hiding this comment.
suggestion:
| - Auto-update experiment runs list whenever user has a new Kedro run (#703) | |
| - Set up web socket and subscription endpoint for auto update of experiment runs list on new Kedro runs (#703) |
There was a problem hiding this comment.
I'd really like to leave out implementation details from release note. IMHO user doesn't care whether it uses websocket or anything else.
There was a problem hiding this comment.
haha yea I kinda knew you would mention this - the main reason I am suggesting this is more for Kedro-viz devs in differentiating this from the previous auto-reload feature, for referencing this release incorporating the actual setup of the web socket mechanism and subscription endpoint.
We can leave out web socket but would still be good to mention enabling subscription in this case ( which web sockets is implied I guess)
There was a problem hiding this comment.
Good point. Updated.
Description
This PR adds the capability for the runs list to be automatically updated whenever there is a new Kedro run by the user.
Development notes
QA notes
Checklist
RELEASE.mdfile