Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

task: persist simulation states in the UI #1653

Merged
merged 19 commits into from
Aug 10, 2023

Conversation

blanchco
Copy link
Contributor

@blanchco blanchco commented Aug 1, 2023

Description

  • This PR has a few assumptions included, but its a basis to integrate the intermediate results into the HMI
  • Feat/pika pyciemss-service#9 Looking at this PR it looks like the progress that is going to be a float between 0 and 1 AND for now only on the calibrate functionality in pyciemss.
  • For now I'm assuming we will get a progress attribute when we get the job status, this can be changed once this is more fleshed out
  • We can now persist the simulation/calibration state when we leave and come back to a workflow. This is done by adding a new attribute called simulationsInProgress on the node state which is an array of simulationIds. When queuing/running these simulations they will be added to the simulationsInProgress node. On mounting the component it will poll for the jobs. When the simulations are complete, they will be deleted from the simulationsInProgress array.

As @mwdchang mentioned this polling will be changed for an event sourcing architecture - will be looking into websockets.

Screen.Recording.2023-08-08.at.9.49.11.AM.mov
Screen.Recording.2023-08-08.at.9.50.48.AM.mov

References #(issue)
#1634

@blanchco blanchco self-assigned this Aug 1, 2023
@blanchco blanchco changed the title task: init draft commit task: add intermediate results in hmi Aug 2, 2023
@blanchco blanchco marked this pull request as ready for review August 8, 2023 14:15
@blanchco blanchco requested review from Tom-Szendrey and mj3cheun and removed request for YohannParis and mwdchang August 8, 2023 14:17
@mwdchang
Copy link
Member

mwdchang commented Aug 8, 2023

There might be some confusion here:

  • Polling for simulation result is fine, results do not leverage message queues
  • Intermediates uses message queues and we want to use event-sourcing to listen to updates

@blanchco blanchco changed the title task: add intermediate results in hmi task: persist simulation states in the UI Aug 8, 2023
@blanchco
Copy link
Contributor Author

blanchco commented Aug 8, 2023

Just to clarify this PR a bit - it was initially an initiative to add intermediate results. Since the BE isn't prepared yet the added changes are just a refactor around polling as well as persisting simulation results in the UI.

@YohannParis
Copy link
Member

I do believe we should use SSE for the this, as multiple window/user could look at this.

@blanchco
Copy link
Contributor Author

blanchco commented Aug 8, 2023

I do believe we should use SSE for the this, as multiple window/user could look at this.

I discussed with @mwdchang a little earlier today - I'm looking into this PR this file specifically and how it can be used for the intermediate results here

@blanchco
Copy link
Contributor Author

blanchco commented Aug 8, 2023

I do believe we should use SSE for the this, as multiple window/user could look at this.

I discussed with @mwdchang a little earlier today - I'm looking into this PR this file specifically and how it can be used for the intermediate results here

But just an FYI this PR is not for the intermediate results but instead the persisting of simulation jobs. Intermediate results will follow

@mwdchang
Copy link
Member

mwdchang commented Aug 8, 2023

Two things

  1. You have some cases where the functional parameters have variable types plus defaults. e.g. string | string[] = '' ... personally I think this makes things a lot more difficult to comprehend than necessary - I prefer functions to be more restrictive than being flexible, makes it a lot easier to use, to test and to maintain. In these cases I think we should either reduce it down to string, or string[] ... let me know your thoughts.

  2. Your return patterns are weird. handleSimulationsInProgress returns "state", but then the return is fed into "getStatus" which expect a list of runIds. These two things are not compatible, am I missing something here?

@blanchco
Copy link
Contributor Author

blanchco commented Aug 8, 2023

Two things

  1. You have some cases where the functional parameters have variable types plus defaults. e.g. string | string[] = '' ... personally I think this makes things a lot more difficult to comprehend than necessary - I prefer functions to be more restrictive than being flexible, makes it a lot easier to use, to test and to maintain. In these cases I think we should either reduce it down to string, or string[] ... let me know your thoughts.
  2. Your return patterns are weird. handleSimulationsInProgress returns "state", but then the return is fed into "getStatus" which expect a list of runIds. These two things are not compatible, am I missing something here?

@mwdchang

  1. Makes sense - It will now always take in a string[] (run ids) and have simplified the add/delete function to only accommodate this.
  2. Agreed. It worked but that pass-thru function was a little too flexible to make any sense. I've fixed this up so I hope it makes more sense now. Add/Delete return a state, Query returns a list of run ids

@blanchco
Copy link
Contributor Author

blanchco commented Aug 8, 2023

I've also stopped the poller when unmounting the component

@blanchco blanchco requested a review from mwdchang August 8, 2023 21:10
Copy link
Member

@YohannParis YohannParis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All those node having the same progress-bar and code is in big need of refactoring.
Why not leveraging the tera-worflow-node.vue

@YohannParis YohannParis marked this pull request as draft August 9, 2023 17:49
@blanchco
Copy link
Contributor Author

blanchco commented Aug 9, 2023

All those node having the same progress-bar and code is in big need of refactoring. Why not leveraging the tera-worflow-node.vue

@YohannParis I agree that this is in need of a refactor here, but I think we would need a new component to encapsulate all of the simulation/calibration/ensemble nodes since a lot of the code is shared between those components only. Workflow nodes such as model & dataset do not need this progress/polling functionality within them so Im not sure it would make sense to put it on the tera-workflow-node.

Do you think I should include this refactor as a part of this PR? I'm a little worried of doing too much in this PR alone. (Persisting simulation status + Intermediate results + refactor of simulation/calibration/ensemble nodes)

@mwdchang mwdchang marked this pull request as ready for review August 10, 2023 00:55
@mwdchang
Copy link
Member

@YohannParis @blanchco Let's leave progress-bar refactor for another time, arguably it might have been introduced a tad too early 😄

  • It isn't clear that all the engined (julia/ciemss), or even individual operations will have the same behaviour when it comes to relaying progress information
  • Given some of the conversations we have had in the last 2 days or so, I think there are still some confusion over the notions of progress, 'simulation-poll', and intermediates. Both intermediate and simulations-polls can denote some sense of "progress" and we should sync up on what we want to convey before undertaking large refactoring efforts.

@mwdchang
Copy link
Member

Didn't test all the cases, but looks fine in general, thanks

@blanchco blanchco merged commit cc5452e into main Aug 10, 2023
5 checks passed
@blanchco blanchco deleted the task-add-intermediate-results-in-hmi branch August 10, 2023 13:13
shawnyama pushed a commit that referenced this pull request Aug 14, 2023
Co-authored-by: Cole Blanchard <cblanchard@Coles-MacBook-Pro.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants