Paths to object well image fix#4933
Conversation
|
--rebased-to #4934 |
|
All checks have passed. The json output in the orphaned-image-on-second-page shows that the image is on second page |
|
Ready to merge fmpov. |
|
could you please add tests to |
|
@will-moore as #4934 was merged could you rebase last commit please |
|
Is the test outcome reported in CI anywhere? |
|
Aha, thank you! I was confused through not seeing it at https://ci.openmicroscopy.org/job/OMERO-DEV-merge-integration-Python/lastCompletedBuild/testReport/. |
chris-allan
left a comment
chris-allan
left a comment
There was a problem hiding this comment.
Based on past experience, I'm fairly concerned here about the number of objects that are being joined in the WellSample-Well-Plate-Screen hierachy being queried. This query has the potential of taking many seconds to return and should be evaluated, potentially in isolation, on a Plate and/or Screen of realistic size.
/cc @joshmoore, @aleksandra-tarkowska, @jburel
| from WellSample wellsample | ||
| left outer join wellsample.details.owner wsowner | ||
| left outer join wellsample.plateAcquisition acquisition | ||
| left outer join wellsample.details.owner aowner |
There was a problem hiding this comment.
The same object wellsample.details.owner is being applied to two separate aliases (wsowner and aowner) here. As details are always present and the base of your JOINs is wellsample you also don't need left outer here.
Maybe add a failing test case for this condition and then fix the bug? Or is this just completely irrelevant and can be removed from the query?
| left outer join wellsample.plateAcquisition acquisition | ||
| left outer join wellsample.details.owner aowner | ||
| join wellsample.well well | ||
| left outer join well.plate plate |
There was a problem hiding this comment.
plate is a required element of well as is details as above so do not need to be left outer joined.
|
@chris-allan this PR is fixing the issue where unnecessarily image in the well were tested as an orphaned image. @will-moore changed the order of the logic identifying images in a well. HQL contently needs improvement as you pointed out, but lets not block this PR as there are no changes to HQL. We will review that in a following up PR to keep it inline with metadata52 branch. Would that be ok with you? |
|
Completely understood, @aleksandra-tarkowska. Your call on the scope, I don't have enough visibility there. |
|
follow up in a card https://trello.com/c/VEXIxsT1/237-review-paths-to-objects-hql-queries |
|
Either option is fine from my perspective. |
|
@chris-allan A sample query from IDR http://idr-demo.openmicroscopy.org/webclient/api/paths_to_object/?image=696619 takes about 150 ms (even with a cache-busting |
|
That's pretty reasonable then @will-moore. |
|
Let's fix queries in another PR, since we now have a trello card for that. Smaller PRs are generally best! |
|
@chris-allan: any more comments? since the "red" change requested is still on |
6 participants
What this PR does
This fixes a bug found during IDR testing today and reproduced on develop:
https://www.openmicroscopy.org/qa2/qa/feedback/17421/
Introduced in #4774
To reproduce / test:
webclient/api/paths_to_object/?image=IDwhich previously failed if the image is in a Well (not orphaned) AND there are more than 1 page of orphaned images.webclient/api/paths_to_object/?image=IDfor the Image in Well returns json that includes the WellIDCheck that orphaned pagination still works
webclient/api/paths_to_object/?image=IDfor this image has correct pagination details returned.The bug was because we try to find the index of orphaned image in list of ALL orphaned images (to know which page it is on) BEFORE checking whether the image is in a Well.
This was also very slow on IDR because it has > 10,000 orphaned images.
Now, we only try to load ALL orphaned images to find page AFTER we've checked to see if image is in a Well.
cc @joshmoore @aleksandra-tarkowska @manics