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 version history view paginated #6483

Merged
merged 15 commits into from
Sep 26, 2022

Conversation

frcroth
Copy link
Member

@frcroth frcroth commented Sep 21, 2022

URL of deployed dev instance (used for testing):

Steps to test:

  • Create some annotations
  • Check the update history

Issues:


@frcroth frcroth self-assigned this Sep 21, 2022
@fm3
Copy link
Member

fm3 commented Sep 22, 2022

@philippotto this issue became relevant again recently (a user reported data loss and we could not explore older annotation version as the update action log request did not complete in reasonable time). Do you think you have some capacity to adapt the front-end to use pagination? And if so, do you think the API with newestVersion and oldestVersion makes sense? Or would you prefer to send a limit and page number? The front-end should already know the newest possible version of the tracing, right? and the oldest should always be zero. This is not urgent, but if you have some time, maybe you could have a look :)

@philippotto
Copy link
Member

@philippotto this issue became relevant again recently (a user reported data loss and we could not explore older annotation version as the update action log request did not complete in reasonable time). Do you think you have some capacity to adapt the front-end to use pagination? And if so, do you think the API with newestVersion and oldestVersion makes sense? Or would you prefer to send a limit and page number? The front-end should already know the newest possible version of the tracing, right? and the oldest should always be zero. This is not urgent, but if you have some time, maybe you could have a look :)

I'm having a look :)

@frcroth could you do the same for the SkeletonTracingController? I think it makes sense that both version routes use the pagination mechanism.

@philippotto philippotto changed the title Add parameters for updateActionLog: newestVersion, oldestVersion Make version history view paginated Sep 23, 2022
@philippotto
Copy link
Member

philippotto commented Sep 23, 2022

The front-end is ready from my side. For easier testing I set the items per page parameter to 5. However, it could probably be something like 500? I also made the groups auto-expand for the testing round, but I'd revert that, too.

The only thing missing is that the back-end also accepts the new parameters for skeleton tracings.

@fm3
Copy link
Member

fm3 commented Sep 23, 2022

Yes 500 seems reasonable. We can still tweak it again later if we find that with real-world annotations this is still slow

@fm3 fm3 marked this pull request as ready for review September 23, 2022 12:06
@frcroth
Copy link
Member Author

frcroth commented Sep 26, 2022

@philippotto The route for skeleton tracing is implemented

@philippotto
Copy link
Member

@philippotto The route for skeleton tracing is implemented

Thanks!

@fm3 Would you like to test this PR? Maybe you could also review the frontend? In that case, I think it would be easiest to have a quick call where I guide you through it :)

Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

Works for me :) Thanks for guiding me through the code in person. Maybe you can investigate why the first two pages are fetched twice each

frontend/javascripts/oxalis/view/version_list.tsx Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/view/version_list.tsx Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/view/version_entry_group.tsx Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/view/version_list.tsx Outdated Show resolved Hide resolved
@philippotto
Copy link
Member

Maybe you can investigate why the first two pages are fetched twice each

Found the bug! I had to change a || to &&. Will merge now unless you veto :)

@philippotto philippotto merged commit 2dab16e into master Sep 26, 2022
@philippotto philippotto deleted the padingation-for-update-action-log branch September 26, 2022 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pagination for update action log
3 participants