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

Refactoring snapshots #1483

Open
boriskovar-m2ms opened this issue Jul 22, 2024 · 25 comments
Open

Refactoring snapshots #1483

boriskovar-m2ms opened this issue Jul 22, 2024 · 25 comments
Assignees
Labels
2024-04-26 orange Design (RHS) dissemination frontend

Comments

@boriskovar-m2ms
Copy link
Collaborator

boriskovar-m2ms commented Jul 22, 2024

Based on #1419

If we want to switch current action based snapshots for state based snapshots, this is how it should work.

Saving snapshot:

  1. We create copy of the state and from the copy we remove everything that is "downloadable" i.e. poses, observations, tags and all computed datasets, job definitions
  2. We send the state json to B/E (in current implementation snapshot entity also has free form json field additional_info so theoretically we can use this one so we do not need to change B/E)

Snapshot restoration:

  1. We download the state (or extract it from additional_info from current snapshot object)
  2. Download all the stuff we stripped away from state during saving
  3. All NGL viewer stuff will be restored by multiple useEffect
  4. All more esoteric stuff like scroll to first visible compound will be also implement by useEffect idiom
  5. Incompatibilities between version will be also handled by useEffect or preprocess pipeline if necessary
  6. In project view where you can switch between snapshots it will be handled in a way that we first apply new NGL view orientation matrix and then apply new state so it will first continuously rotate and after that the changes will be applied (or other way around - don't remember what is done first. If rotation or applying the changes)

PROS:

  1. The biggest pro is that after initial implementation the snapshot saving and restoration will require no additional work (currently you need to specify for every small action: the action object, save behavior, restore behavior, redo and undo behavior). This requires that the NGL viewer will be controlled only using useEffects which will monitor changes in the state related to NGL viewer. Currently it's done imperatively after you lets say press L button we change the state and we tell NGL viewer in the same function what to render. The information about ligand is used only by the rest of the frontend. Now we will implement useEffects which will fully control NGL viewer so NGL viewer will only react to changes in global state and we will never directly control it. The only additional work will be with actions like scroll to first visible compound but historically we do not do this very often.
  2. Waaaaay less code needed therefore less code to maintain and need to understand
  3. Easier to understand code (some parts will definitely not be trivial but on aggregate it will be easier to understand)

CONS:

  1. We will most likely lose Undo/Redo functionality. There is a kind of a hope that we will be able to regain some of the ability to undo redo but it will be undo redo on very atomic/low level so no mass actions nor aggregate actions and definitely no NGL viewer only undo/redo (I don't think they fully work even now). So basically if you press button that selects 20 hits then you will need to press undo 20 times to really undo that one action (yes in this specific case it can be done by updating state in one go and not 20 times when pressing the button but in general it not might be possible - specifically when one action is updating multiple different slices of redux store).
  2. No actions timeline

Not quite sure how long it would take me to implement this but we are definitely talking about several weeks but most likely we are not talking about multiple months.

@Waztom
Copy link
Collaborator

Waztom commented Aug 1, 2024

@boriskovar-m2ms has started to review the time/steps needed for this refcatoring excercise and estimates update next Tuesday.

@boriskovar-m2ms
Copy link
Collaborator Author

SnapshotsRework-Estimations.xlsx
My estimations of refactoring.

@mwinokan
Copy link
Collaborator

Thanks for the estimate @boriskovar-m2ms.

@Waztom and I discussed the refactor and we think you should go ahead on this.

@boriskovar-m2ms boriskovar-m2ms moved this from FragTech to In Progress (DEV) in Fragalysis Aug 15, 2024
boriskovar-m2ms added a commit that referenced this issue Aug 19, 2024
@mwinokan
Copy link
Collaborator

@boriskovar-m2ms has been thinking about the approach to the state-based snapshot resolution, and will begin work imminently.

An update on the time estimate: 18 working days would resolve to 6 weeks as Boris is working 3 days per week on Fragalysis.

@mwinokan
Copy link
Collaborator

@boriskovar-m2ms has completed the state-based rendering of ligands, and is working ligand removal. The rest of the rendering elements are pending. This would round off the NGL changes necessary for the rest of the refactor.

@mwinokan
Copy link
Collaborator

@boriskovar-m2ms hit a roadblock with rendering densities, that only occurs when rendering locally, but it has been fixed. Once this is done, Boris will move on to the RHS, after which the groundwork for the snapshot refactor itself will complete.

@mwinokan
Copy link
Collaborator

mwinokan commented Sep 3, 2024

Last week @boriskovar-m2ms fixed the electron density and atom quality rendering, and will work on RHS rendering ligands, complexes, surfaces and interactions.

@mwinokan
Copy link
Collaborator

@boriskovar-m2ms confirms that state based rendering is complete for the RHS. Now Boris can workshop sending state to the backend

@mwinokan
Copy link
Collaborator

mwinokan commented Sep 17, 2024

@boriskovar-m2ms says he can upload the state to the b/e but there is outstanding work (being skipped for now) regarding the copying the state (have to work around hard limit of 2GB RAM usage e.g. in Chrome), Boris wants to have the end-to-end functionality first and then will craft a copy algorithm that works within the memory limits.

Estimate is three weeks of work required including this week

@mwinokan
Copy link
Collaborator

@boriskovar-m2ms reports that restoring orientations has been more challenging. Some of the effects for opening modals happen in a random order, and some effects require data to be loaded which is not generally true. Currently, Boris is dealing with a too large request header error - hopefully a fresh Docker install will resolve the discrepancy between working Tibor stack and Boris' local build (especially as the Tibor stack header is larger than the local one).

@mwinokan
Copy link
Collaborator

@matej-vavrek reports that @boriskovar-m2ms is almost done, working on restoring scroll functionality, and switching between snapshots. Optimistically, could be all done next week

@phraenquex asks that the NGL view should smoothly interpolate between the two orientations when switching between snapshots

@mwinokan
Copy link
Collaborator

mwinokan commented Oct 1, 2024

@boriskovar-m2ms says that scrolling is working, and he is working on restoring the saving the project functionality (which is very similar to saving snapshots). Boris will also work on restoring the interpolation between snapshots in the projects.

There may be another optimisation to complete but otherwise Boris hopes to be done this week.

@mwinokan
Copy link
Collaborator

mwinokan commented Oct 3, 2024

@boriskovar-m2ms is working on saving and restoring projects, and will move on to interpolating between snapshots within a project

With @boriskovar-m2ms managing his time on other projects he is anticipating the implementation, testing and release can be finished around October 17th/18th. Almost all files from the f/e have been changed but the b/e is untouched

@mwinokan
Copy link
Collaborator

mwinokan commented Oct 8, 2024

@boriskovar-m2ms is unwell but he will finish switching between the snapshots and then the refactor is complete (pending a large merge with all the other f/e changes)

@mwinokan
Copy link
Collaborator

mwinokan commented Oct 15, 2024

@boriskovar-m2ms is back to working on changing between snapshots, after which we will be feature complete and will implement the optimisation which is necessary to allow for states larger than 1GB

ETA is pushed back around 7 days

boriskovar-m2ms added a commit that referenced this issue Oct 17, 2024
@mwinokan
Copy link
Collaborator

mwinokan commented Oct 17, 2024

@boriskovar-m2ms has had to rebase with the latest b/e and f/e images and is dealing with many conflicts which were larger than expected but would always be needed

@boriskovar-m2ms asks if it is possible to upload data (LHS & RHS) to a local stack if ISpyB is down? @alanbchristie says an environment variable might be needed to assign Boris to a specific proposal (without ISpyB) and Alan can help Boris achieve this.

@mwinokan
Copy link
Collaborator

@boriskovar-m2ms has completed merge/rebase and resolved conflicts (lots of phantom errors). Related to using Frank's SSH connection (where there seemed to be some throttling/timeouts) caused some false negatives on f/e bugs.

Work is still in progress to make the snapshot transitions smooth(er). Aims to finish it this week, but there are more difficulties than expected, even after the refactor. The transition actually happens between three states, current, target, and a "phantom" state that preserves some, but not all, of the current state.

@mwinokan
Copy link
Collaborator

@boriskovar-m2ms reports he is 99% that he will be feature-complete by EOD tomorrow. But there have been some deployment issues. Boris is also progressing on the smooth transitions, and on Monday he will do the optimisation (to fix 1GB ram limit) redeploy.

@Waztom
Copy link
Collaborator

Waztom commented Oct 29, 2024

Boris adding save functionality after changes made to the snapshot. Frank suggests continuously capturing changes with "snapshot saved" toast notification (edit or delete) to then link to snapshot editing modal vs. option to save modal.

@mwinokan
Copy link
Collaborator

@boriskovar-m2ms has completed almost everything, the only outstanding work is that changes to an existing snapshot should be saved when working within a project (about 50% complete).

Meanwhile, Boris has also been squashing bugs (some of which are a consequence of the memory optimisation). One bug remains open: when you are logged in and share a snapshot to someone who is also logged in who then tries to open it, it might fail subtly or spectacularly (couple hours of work pending).

@Waztom
Copy link
Collaborator

Waztom commented Nov 5, 2024

@boriskovar-m2ms deployment is stuck...pending is one final functionality, needs to add a button/link to the snapshot save/edit name modal.

@boriskovar-m2ms
Copy link
Collaborator Author

boriskovar-m2ms commented Nov 7, 2024

Feature complete version is now deployed to tibor's stack.

@mwinokan
Copy link
Collaborator

mwinokan commented Nov 7, 2024

@phraenquex reports that the NGL rotation is not restored in Firefox

@boriskovar-m2ms says that he has a very large backlog of work on other projects which he will work on exclusively next week, but we should enumerate a shopping list of bugs to squash for when he's back

@boriskovar-m2ms
Copy link
Collaborator Author

As @phraenquex mentioned NGL view is not restoring orientation from snapshot in Firefox. Same snapshot works fine in Chrome. Needs more investigation.

@phraenquex
Copy link
Collaborator

phraenquex commented Nov 12, 2024

Bugs spotted by @phraenquex:

  • When I create a New Project, don't redraw everything - just make it be a project, and leave the UI as is.
  • When I hit [save], don't put up the name-snashot dialog - just throw me a toaster, with a link to the dialog.
  • [ ]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2024-04-26 orange Design (RHS) dissemination frontend
Projects
Status: Dev Done - Do review (DEV)
Development

No branches or pull requests

4 participants