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

Limit number of FileAnnotations retrieved for OMERO.web UI #519

Merged
merged 8 commits into from
Dec 19, 2023

Conversation

knabar
Copy link
Member

@knabar knabar commented Dec 8, 2023

Addresses issue #517

The current "Choose attachment" dialog retrieves a list of every FileAnnotation not currently linked to the selected object(s) for linking, causing poor performance or failures on systems with a large number of FileAnnotations.

The goal of this PR is to address the potential performance impact only; any other improvements e.g. to the UI should be addressed in separate PRs.

Main discussion points:

API changes

  1. getFilesByObject now returns a tuple of the number of total files and a list of FileAnnotationShims; previously the method returned a single list of FileAnnotations.
  2. In case of no objects passed to the controller, getFilesByObject previously returned all FileAnnotations, and will now raise a ValueError. This also means listFileAnnotations is no longer used and could be deprecated.

Testing

There are currently no stated expectations to the exact behavior of the "Choose attachment" dialog or the getFilesByObject method. The UI also does not expose all of the available options of the method. With the large number of permutations of options and object types (and multiple object types, which are not handled), exhaustive testing will be difficult. Current testing is likely minimal.

  • We should document the expected behavior of the "Choose attachment" dialog in the possible situations.
  • A testing plan is needed.

Potential pitfalls:

  • It is currently possible for multiple users to link the same annotation to the same parent object in a read-annotate group

Discussion

  • What should be the limit on number of FileAnnotations returned? Currently set to 100, but suggested to increase to several hundred.
  • How should FileAnnotations be sorted? Currently by descending updateEvent.time, but suggested to use name as previously.
  • Indication in the dialog that the results are truncated; space is currently limited, but some indication that not all available annotations are shown is likely needed.

chris-allan and others added 2 commits December 7, 2023 21:14
Refactors the stack which produces the list of existing FileAnnotations
for selection in the "Choose attachments" dialog.
@will-moore
Copy link
Member

With a bunch of FileAnnotations created with https://github.com/will-moore/python-scripts/blob/main/create_file_annotations.py this looks like:

Screenshot 2023-12-11 at 09 47 48

If I add a bunch of FileAnnotations, then these are correctly excluded from the dialog next time I open it and the count goes down accordingly.

Do we want to boost the page size and sort alphabetically as discussed?

@knabar
Copy link
Member Author

knabar commented Dec 11, 2023

Do we want to boost the page size and sort alphabetically as discussed?

@will-moore I'm working on that today.

@knabar knabar requested review from will-moore and jburel December 12, 2023 09:39
@knabar knabar self-assigned this Dec 12, 2023
@knabar knabar added the enhancement New feature or request label Dec 12, 2023
@knabar knabar added this to the 5.24.0 milestone Dec 12, 2023
@knabar knabar requested a review from pwalczysko December 12, 2023 09:40
@will-moore
Copy link
Member

I think this PR is failing tests at https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/lastCompletedBuild/testReport/OmeroWeb.test.integration.test_annotate/TestFileAnnotations/test_batch_add_fileannotations_2_/

The test is failing to exclude an annotation previously linked to the selected Projects.
I tested in merge-ci webclient and found the same thing with FileAnnotations previously created by upload. This may be because they don't have a namespace??

Copy link
Member

@pwalczysko pwalczysko left a comment

Choose a reason for hiding this comment

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

the feature works, but I am being offered in the dialog attachments which I already attached. Basically concurring with @will-moore ' testing results

@knabar
Copy link
Member Author

knabar commented Dec 12, 2023

@will-moore @pwalczysko I added the NULL check for the namespace to bring the new query more in line with the old one

<span>Select the attachments to add</span>
<span>
Select the attachments to add
(total: {{ total_files }}{% if total_files > 500 %}, showing 500{% endif %})
Copy link
Member

Choose a reason for hiding this comment

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

Minor point: hard-coding 500 here and in container.py feels a bit brittle.
I guess you could make a MAX_FILES variable and add it into the context, or add 'file_count': len(files) to the page context and then do {% if total_files > file_count %}, showing {{ file_count }} {% endif %}

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @will-moore, I wanted to do exactly that and then promptly forgot about it. Fixed now.

@will-moore
Copy link
Member

Working fine:

Screenshot 2023-12-13 at 15 37 55

@pwalczysko
Copy link
Member

Works as expected on merge-ci. lgtm

Copy link
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

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

Changes look good - tested locally (merge-ci unavailable just now, but had tested there earlier) - all working.

@knabar knabar merged commit 66a1c0c into ome:master Dec 19, 2023
9 checks passed
@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/unable-to-add-attachment-using-omero-web/84146/22

@knabar knabar deleted the fix-fileannotation-performance branch May 21, 2024 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants