Tag removal performance

[will-moore]

What this PR does

A couple of performance improvements for removing tags from images.
See https://trello.com/c/gPkTUuw5/23-tags-removal-takes-too-long-on-200-images

We now use the batch conn.deleteObjects() with wait=False to not wait on async delete, instead of doing a single conn.deleteObject() for each link in turn.

Testing this PR

1. Select many images, add Tag to all of them and Save.
2. Remove tag either with (-) icon on tag itself OR via Tag dialog (moving tag into left column)
3. In both cases this should be much improved performance.

[mtbc]

Good to merge.

[jburel]

Can you add some tests?
it will be good to see the performance improvement in a test
i.e. delete object by object as before and "batch" delete
Also we need to be sure that all objects are correctly deleted with the new approach

[will-moore]

@jburel We already have some robot tests for adding & removing tags with Tag Dialog. I've added more tests for removing tags with the (-) button on the Tag itself (all these tests check that the tag was actually removed successfully).
It's hard to see how we can test the performance improvement in a test since we will not have the 'before' and 'after' to compare. Also, performance is notoriously variable for robot tests so a test that fails when it's slow is going to be very flaky.

[will-moore]

New test passing at https://ci.openmicroscopy.org/view/Failing/job/OMERO-DEV-merge-robotframework/402/robot/Web%20&%20Web/Web_1/Tagging%20Test/

[gusferguson]

Tested with http://web-dev-merge.openmicroscopy.org/webclient/ user-3

Tag addition and deletion works as expected - single and batch.
Didn't try a huge number, but seems speedy!

[jburel]

Sorry for not replying earlier, for performance I was not thinking of a robot test
but of an integration test using the remove method and using directly self.conn.deleteObject(al._obj) with let say with 50 objects

[will-moore]

@jburel For performance testing of removing a tag from 50 objects, how slow is too slow? In my experience, performance tests are either very flaky if the limit is set too short (and therefore ignored) or are set so long as to be meaningless.
The slow times before were due to some inefficient logic in web, which is unlikely to reappear in this spot again (no more vulnerable to a performance regression than any other part of the webclient).

[mtbc]

It does seem that the usual way we fix a flaky performance test is to disable it or to increase its patience. Variations in CI deployment make them tricky. In this case though, can maybe make sure it's no more than thrice longer than removing a tag from 10, or something?

[jburel]

e.g. #4821

[mtbc]

from #4821 -

// marked as 'broken', works fine locally, but flaky on CI

sigh yes, indeed

[jburel]

but at least we can evaluate it

[will-moore]

To test last commits:

• check tests are green
• Try adding a lot of tags (E.g. 20 tags) to 200 images:
  • Check that tags are added and removed as expected
  • In batch annotate panel, check tags are all listed and tooltips show correct numbers "Can remove tag from 200 images" etc.

[jburel]

./components/tools/OmeroWeb/test/integration/test_tags.py:191:9: F841 local variable 'img' is assigned to but never used

[will-moore]

This is conflicting with spw grid view so if this is "good to merge" then it would be good to get it in soon. cc @pwalczysko

[pwalczysko]

Both robot and integration tests are green.

[pwalczysko]

The performance is pretty speedy if you remove one tag from 200 images. If you remove 18 from 200, it is slower obviously, but still acceptable.
Ready to merge.

[jburel]

Thanks