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

Having an option to keep or replace generated pv images when a new request is made (backend) #1176

Closed
kswang1029 opened this issue Aug 4, 2022 · 10 comments
Assignees
Labels
enhancement New feature or request requiring frontend this backend issue requires work in both frontend and backend
Milestone

Comments

@kswang1029
Copy link
Contributor

kswang1029 commented Aug 4, 2022

user feedback from v3 beta test

With v3 release, when a new request made in the pv generator or moment generator, generated images from the previous request will be removed. Users would like to have an option to keep or replace the pv or moment images.

The companion backend issue is CARTAvis/carta-frontend#1951

@pford
Copy link
Collaborator

pford commented Sep 7, 2022

I think for simplicity this should be a boolean "overwrite" field in the PvRequest message. What should the name of the second PV image be?

Update: field changed to boolean "keep" so that default (false) is current behavior.

@kswang1029
Copy link
Contributor Author

I think for simplicity this should be a boolean "overwrite" field in the PvRequest message. What should the name of the second PV image be?

Shall we append a number?

@pford
Copy link
Collaborator

pford commented Sep 7, 2022

Starting with 1 (pv, pv1, pv2) or 2 (pv, pv2, pv3)?

@kswang1029
Copy link
Contributor Author

Starting with 1 (pv, pv1, pv2) or 2 (pv, pv2, pv3)?

I prefer option 1 but either way is fine. 🙂

@kswang1029
Copy link
Contributor Author

Starting with 1 (pv, pv1, pv2) or 2 (pv, pv2, pv3)?

I prefer option 1 but either way is fine. 🙂

As users may close one of the in-ram PV images at any time, we can just keep increasing the index and don’t re-use the index.

@pford pford assigned pford and unassigned markccchiang Sep 7, 2022
@pford
Copy link
Collaborator

pford commented Sep 8, 2022

I just noticed the moment part! I think there should be separate branches and PRs (frontend, backend, and protobuf) for the Moment changes.

@crocka and I have been chatting about this issue, with keep = append new PV image with new name, replace = close PV image and append new PV image with same name. But when multiple PV images exist and replace is used, which image or images are closed and replaced?

@kswang1029
Copy link
Contributor Author

I just noticed the moment part! I think there should be separate branches and PRs (frontend, backend, and protobuf) for the Moment changes.

@crocka and I have been chatting about this issue, with keep = append new PV image with new name, replace = close PV image and append new PV image with same name. But when multiple PV images exist and replace is used, which image or images are closed and replaced?

how about we close all generated images associated with the source image when “replace” mode is selected? The idea is similar to how we handle “load image” and “append image”. When it is load image we close all loaded images first then load the new image. When it is append image we do not close any loaded images and append the image.

@pford
Copy link
Collaborator

pford commented Sep 9, 2022

I just noticed the moment part! I think there should be separate branches and PRs (frontend, backend, and protobuf) for the Moment changes.
@crocka and I have been chatting about this issue, with keep = append new PV image with new name, replace = close PV image and append new PV image with same name. But when multiple PV images exist and replace is used, which image or images are closed and replaced?

how about we close all generated images associated with the source image when “replace” mode is selected? The idea is similar to how we handle “load image” and “append image”. When it is load image we close all loaded images first then load the new image. When it is append image we do not close any loaded images and append the image.

Ok, that makes sense. In this case would the new PV image be simply "pv" (reset index to 0 and future images 1, 2, etc.), or continue incrementing the previous index?

@kswang1029
Copy link
Contributor Author

I just noticed the moment part! I think there should be separate branches and PRs (frontend, backend, and protobuf) for the Moment changes.
@crocka and I have been chatting about this issue, with keep = append new PV image with new name, replace = close PV image and append new PV image with same name. But when multiple PV images exist and replace is used, which image or images are closed and replaced?

how about we close all generated images associated with the source image when “replace” mode is selected? The idea is similar to how we handle “load image” and “append image”. When it is load image we close all loaded images first then load the new image. When it is append image we do not close any loaded images and append the image.

Ok, that makes sense. In this case would the new PV image be simply "pv" (reset index to 0 and future images 1, 2, etc.), or continue incrementing the previous index?

Maybe we can reset the index if that makes sense to you too. I don’t have preference though.

@pford pford added this to the v4.0-b1 milestone Sep 13, 2022
@pford pford mentioned this issue Sep 14, 2022
6 tasks
@pford pford changed the title Having an option to keep or replace generated moment or pv images when a new request is made (backend) Having an option to keep or replace generated pv images when a new request is made (backend) Sep 28, 2022
@pford
Copy link
Collaborator

pford commented Sep 28, 2022

@kswang1029 @crocka I created a separate backend issue #1202 for keeping moment images.

@pford pford closed this as completed Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request requiring frontend this backend issue requires work in both frontend and backend
Projects
None yet
Development

No branches or pull requests

3 participants