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

Make it possible to cancel an in-progress simulation #1032

Merged
merged 5 commits into from
Jan 31, 2024

Conversation

mattdailis
Copy link
Collaborator

We previously implemented the ability to cancel pending simulations. As of a week or two ago, the backend now supports canceling in-progress simulations. This PR moves around a few if statements in the UI code to:

  • mark the simulation state as Canceled if simulationDataset.canceled
  • display the cancel buttons for in-progress simulations
  • calculate the simulation extent (if available) for a canceled simulation using the reason field
Screen.Recording.2023-11-29.at.3.58.07.PM.mov

@AaronPlave
Copy link
Contributor

This will be so useful! Not something we need to do in this PR but wonder if we should have a visual indicator on the timeline for when a sim was canceled so that it's obvious that the lack of data in certain places is due to sim cancelation and not some sort of gap? Though perhaps if you cancel a sim you don't really care about sim results? If we decide to do anything about this then it should also play into whatever we do for streaming sim results..

Copy link
Collaborator

@duranb duranb left a comment

Choose a reason for hiding this comment

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

This is awesome! My one concern is choice of icon to cancel or maybe its placement. To me it looks like it's a "close" button and clicking it would get rid of the current simulation from the UI altogether. Maybe if you hover over "in progress", it shows "cancel"?

@AaronPlave
Copy link
Contributor

AaronPlave commented Nov 30, 2023

This is awesome! My one concern is choice of icon to cancel or maybe its placement. To me it looks like it's a "close" button and clicking it would get rid of the current simulation from the UI altogether. Maybe if you hover over "in progress", it shows "cancel"?

We could also try using a proper cancel button, like a circle with a slash through it? Would need to add such an icon to Stellar. Or a stop icon
image

And one note - the close icon was actually there before this PR since it also applies to canceling pending sims.

@duranb
Copy link
Collaborator

duranb commented Nov 30, 2023

This is awesome! My one concern is choice of icon to cancel or maybe its placement. To me it looks like it's a "close" button and clicking it would get rid of the current simulation from the UI altogether. Maybe if you hover over "in progress", it shows "cancel"?

We could also try using a proper cancel button, like a circle with a slash through it? Would need to add such an icon to Stellar. Or a stop icon image

And one note - the close icon was actually there before this PR since it also applies to canceling pending sims.

Ooooh. My bad. I still think that the icon might be misleading, but I'll let you @mattdailis decide whether or not that gets updated in this PR or another effort or if it's worth updating at all.

src/stores/simulation.ts Outdated Show resolved Hide resolved
@AaronPlave
Copy link
Contributor

One thing I'm noticing which could just be how the backend works now is that if i queue up a bunch of simulations (past the number of available workers) the pending sims will stay pending until a worker completes a sim and then all pending sims except for the latest one will be marked as failed. I'm assuming this is somewhat similar to the old behavior where pending sims that were outdated would be auto canceled? In any case, seems odd to mark them as failed in the UI here, wondering how we should handle this?

@AaronPlave
Copy link
Contributor

Another issue that could be in a separate PR is that sims are cancelable during plan snapshot preview

@mattdailis mattdailis self-assigned this Dec 21, 2023
@mattdailis mattdailis force-pushed the feat/cancel-in-progress-simulation branch from 946002b to 1a6c915 Compare January 11, 2024 19:55
@lklyne
Copy link
Contributor

lklyne commented Jan 11, 2024

This is awesome! My one concern is choice of icon to cancel or maybe its placement. To me it looks like it's a "close" button and clicking it would get rid of the current simulation from the UI altogether. Maybe if you hover over "in progress", it shows "cancel"?

We could also try using a proper cancel button, like a circle with a slash through it? Would need to add such an icon to Stellar. Or a stop icon image

And one note - the close icon was actually there before this PR since it also applies to canceling pending sims.

I can make this icon!

@mattdailis mattdailis force-pushed the feat/cancel-in-progress-simulation branch from 1a6c915 to 62daa74 Compare January 18, 2024 17:38
Copy link
Collaborator

@duranb duranb left a comment

Choose a reason for hiding this comment

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

Other than the feedback that @AaronPlave left, I think this works great! Thank you!

@duranb
Copy link
Collaborator

duranb commented Jan 19, 2024

@mattdailis also if you want to disable the button if it's in plan preview, there's a planReadOnly store boolean that we utilize in stores/plan.ts

Copy link
Collaborator

@duranb duranb left a comment

Choose a reason for hiding this comment

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

Just left a small pedantic CSS comment. Feel free to ignore it if you don't think it's worth updating. This looks good to me otherwise! Great work!

@mattdailis mattdailis force-pushed the feat/cancel-in-progress-simulation branch from f145d7e to 810a194 Compare January 31, 2024 00:35
@mattdailis mattdailis merged commit cfd8f6b into develop Jan 31, 2024
4 checks passed
@mattdailis mattdailis deleted the feat/cancel-in-progress-simulation branch January 31, 2024 00:44
JosephVolosin pushed a commit that referenced this pull request Aug 20, 2024
* Make it possible to cancel an in-progress simulation

* Handle simulations canceled after finishing

* Update stellar and tweak button css

* Fix package.json lint error

* Disable cancel buttons when plan is read only
JosephVolosin pushed a commit that referenced this pull request Oct 21, 2024
* Make it possible to cancel an in-progress simulation

* Handle simulations canceled after finishing

* Update stellar and tweak button css

* Fix package.json lint error

* Disable cancel buttons when plan is read only
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants