Api wells by will-moore · Pull Request #5047 · ome/openmicroscopy

will-moore · 2017-01-18T13:14:55Z

What this PR does

Adds support for Wells to web api.
NB: Needs ome/omero-marshal#31. requirements.txt points to that branch of omero-marshal to allow testing of this PR. Once that is merged, we can revert requirements.txt.

Testing this PR

Added tests in test_api_wells.py
Browse to /plates/
- List of /plates/ - each Plate will have link to 'url:wells': 'plates/:id/wells'
- Single plate /plates/:id also links to 'wells'
- Browse to plates/:id/wells (or /wells/?plate=:id ) to filter wells by Plate
- /wells/ will be ordered by Column & Row and include WellSamples, PlateAcquisitions & Images.
- plate/:id/wells/ will also include any Wells that have no WellSamples & Images
- Each Well in /wells/ will link to a single /wells/:id/
- /wells/:id shows single Well with WellSamples, Images and Pixels loaded.
- Images listed at /wells/ and /wells/:id/ will have link to 'url:image': 'images/:id/'

Related reading

As discussed at ome/design#65 Wells are the next object we need to support. This now allows browsing Screen -> Plate -> Well -> Image starting at /api/.

Also tests ome/omero-marshal#31.

cc @chris-allan @jburel @aleksandra-tarkowska

To consider for follow-up:

Filter wells in plate by PlateAcquisition (only load images from single plateAcquisition)
Don't marshal separate PlateAcquisition for every WellSample. Normalize!
/plate/:id/ needs min/max wsIndex

will-moore · 2017-01-19T11:12:51Z

Discussed in api meeting today:

Listing wells, load ws, image, plateAcquisition (NB: need plateAquisition support in omero-marshal)
Showing single Well, also load pixels.
When listing Wells, DO return wells with no images.

This allows usage of non-string attributes. E.g. 'obj.column, obj.row' for wells. But now you need to specify the 'obj' in the order_by. E.g. 'lower(obj.name)' for images etc.

will-moore · 2017-01-24T09:38:33Z

Tests failing because of requirement for Wells support in omero-marshal: https://ci.openmicroscopy.org/view/Failing/job/OMERO-DEV-merge-integration-Python27/437/testReport/

jburel · 2017-01-24T20:28:57Z

components/tools/OmeroPy/src/omero/gateway/__init__.py

+                     " left outer join fetch wellSamples.plateAcquisition"\
+                     " as plateAcquisition"
+        if load_pixels or load_channels:
+            query += ' left outer join fetch image.pixels pixels' \


I will add as pixels to match the rest of the query, easier to read

The section to load pixels and channels is also in def _getQueryString(cls, opts=None): ( line 7159)
I think we should have helper method to generate part of the query instead of having the same code in various locations of the files
If we make a change to the query, we will certainly forget to update one query

jburel · 2017-01-24T20:45:27Z

components/tools/OmeroWeb/test/integration/test_api_containers.py

 from omero_marshal import get_encoder
-from omero.model import DatasetI, ProjectI, ScreenI, PlateI, ImageI
-from omero.rtypes import rstring, unwrap
+from omero.model import DatasetI, ProjectI, ScreenI, PlateI, ImageI, \


one per line and in alphabetical order

jburel · 2017-01-24T20:46:49Z

components/tools/OmeroWeb/test/integration/test_api_wells.py

+import pytest
+from omero.gateway import BlitzGateway
+from omero_marshal import get_encoder
+from omero.model import PlateI, WellI, WellSampleI, ImageI, LengthI


alphabetical order and one per line

jburel · 2017-01-24T20:47:28Z

components/tools/OmeroWeb/test/integration/test_api_containers.py

        screen = get_update_service(user1).saveAndReturnObject(screen)
        plate = get_update_service(user1).saveAndReturnObject(plate)
+
+        # Add well to first plate


i will add a plate acquisition to the test data since it is what will happen by default with a plate import

I've added PlateAcquisition testing to test_api_wells.py but not duplicated this in test_api_containers.py because this test is really concerned with testing urls: Screen -> Plate -> Wells and we don't need to cover PlateAcquisitions twice. Also, it's nice to test that we can load Wells with NO Plate Acquisitions which we're not testing anywhere else now.

I will check both cases to be on the safe side
i.e. with plate acquisitions and without

I will update test_api_wells.py to test with & without PlateAcquisitions, but it doesn't make sense for every test to test everything. Otherwise we will never stop writing tests and all the tests will be so large that you can't understand them.

jburel · 2017-01-24T20:49:15Z

components/tools/OmeroWeb/test/integration/test_api_wells.py

+    return connection
+
+
+def cmp_name_insensitive(x, y):


probably time to have helper methods since this one for example is now in all the test files

jburel · 2017-01-24T20:50:39Z

components/tools/OmeroWeb/test/integration/test_api_wells.py

+    @pytest.fixture()
+    def user1(self):
+        """Return a new user in a read-annotate group."""
+        group = self.new_group(perms='rwra--')


testing other permissions will be useful

I'm not sure what you're wanting here. Testing that queries still work in a read-only or private group? Tests in test_api_projects.py do various permissions & cross-group queries so I don't think we need to replicate this in other tests that are focussed on different objects?

as far as I can tell only read-annotate and read-only are covered in test_api_projects.py
Also with the introduction of new roles, test with various levels of permissions need to be added (admin, owner etc).
This can be done in a follow up PR to solidify what is currently in place

We have seen previously problems when loading different objects i.e. what was true for projects
was not for, for example, image.

jburel · 2017-01-24T20:51:07Z

components/tools/OmeroWeb/test/integration/test_api_wells.py

+                well.column = rint(col)
+                well.row = rint(row)
+                well.plate = PlateI(plate.id.val, False)
+                # Only wells in first Column have well-samples etc


jburel · 2017-01-24T20:52:00Z

components/tools/OmeroWeb/test/integration/test_api_wells.py

+
+    @pytest.fixture()
+    def small_plate(self, user1):
+        """


same here: I will add plate acquisition to match reality

Done in 84ad329

jburel · 2017-01-26T14:52:57Z

./components/tools/OmeroPy/src/omero/gateway/__init__.py:149:1: E302 expected 2 blank lines, found 1

will-moore · 2017-01-26T15:00:11Z

@jburel - I don't understand that travis failure. I have the same version of flake8 locally but don't see any failure. Also the line 149 is 2 blank lines!

will-moore · 2017-01-26T15:00:53Z

$ flake8 components/tools/OmeroPy/src/omero/gateway/__init__.py
$ pip freeze | grep flake8
flake8==2.4.0

will-moore · 2017-01-26T15:03:07Z

That flake8 failure was fixed by the last commit: 78a5a94. Will try to re-run...

jburel · 2017-01-26T15:25:35Z

The changes look good, waiting on travis now

will-moore · 2017-01-26T16:30:12Z

@jburel Travis is good, but we really need the omero-marshal PR in since without it we still have failing tests etc.

jburel · 2017-01-26T20:04:57Z

of course we need to have the tests passing before we merge anything

atarkowska · 2017-01-26T20:48:32Z

why not to add omero-marshal merge build to CI and then pip install from snoopy? Exactly as we handle other web plugins

snoopycrimecop · 2017-01-27T03:33:51Z

Conflicting PR. Removed from build OMERO-DEV-merge-push#576. See the console output for more details.
Possible conflicts:

PR Annotation count web #5023 dominikl 'Annotation count web'
- components/tools/OmeroPy/src/omero/gateway/init.py

will-moore · 2017-01-30T11:06:40Z

With requirements.txt now pointing at ome/omero-marshal#31 browsing of SPW is working on merge build, E.g. (user-3)
http://web-dev-merge.openmicroscopy.org/api/v0/m/plates/2077/wells/
and tests are passing at https://ci.openmicroscopy.org/view/Failing/job/OMERO-DEV-merge-integration-Python27/441/testReport/OmeroWeb.test.integration.test_api_wells/

chris-allan · 2017-01-30T21:45:12Z

components/tools/OmeroWeb/omeroweb/api/views.py

        return super(ApiView, self).dispatch(*args, **kwargs)

-    def add_data(self, marshalled, request, **kwargs):
+    def add_data(self, marshalled, request, urls={}, **kwargs):


Watch out for argument constructs like this. urls={} does not create a new dictionary on every method invocation so if you modify it, add new keys, etc. they are going to get picked up and used in subsequent invocations. If you want to be on the safe side, set the default to None and create the dictionary in the method on None.

/cc @jburel

dominikl · 2017-02-10T08:53:19Z

Is this PR going to be merged? Then I could rebase my annotation count PR #5023 (merge conflicts with this PR).

chris-allan · 2017-02-10T11:58:22Z

Now that we have ome/omero-marshal#31 merged I think what we need to do here is revert the change to requirements-common (we can't really have hacks like in), ensure all the tests pass, and then go ahead and merge.

This reverts commit 852c3ed.

atarkowska · 2017-02-13T08:42:01Z

components/tools/OmeroWeb/omeroweb/api/views.py

+    # Urls to add to marshalled object. See ProjectsView for more details
+    urls = {
+        'url:wells': {'name': 'api_plate_wells',
+                      'kwargs': {'plate_id': 'OBJECT_ID'}}


if that is placeholder why OBJECT_ID not just None ?

OBJECT_ID is a unique identifier that this should be replaced with the ID of the current object. It's possible that other IDs could be used in future. Using None doesn't explicitly say what the placeholder should be replaced with.

jburel · 2017-02-13T10:47:28Z

tmp change has been reverted.
it is now pointing to the master branch

https://ci.openmicroscopy.org/view/Failing/job/OMERO-DEV-merge-integration-Python27/451/testReport/OmeroWeb.test.integration.test_api_wells/ is green
merging

jburel added the develop label Jan 18, 2017

will-moore closed this Jan 18, 2017

will-moore mentioned this pull request Jan 19, 2017

Initial support for Well-WellSample-Image hierarchy ome/omero-marshal#31

Merged

will-moore added 13 commits January 19, 2017 11:48

Add /wells/ ordered by column, row NOT name

7c632bb

Blitz buildQuery() remove lower(obj.%s) from order_by

1843443

This allows usage of non-string attributes. E.g. 'obj.column, obj.row' for wells. But now you need to specify the 'obj' in the order_by. E.g. 'lower(obj.name)' for images etc.

ObjectsView order_by uses 'obj.'

f26b10e

Blitz Well.getQueryString() supports filter by 'plate'

02bd39b

Filter Wells by ?plate=id or /plates/id/wells

e630df5

Add wells_url to /plates/:id/ json

51bc36c

Blitz Well.getQueryString() supports 'load_images'

7b785cc

Blitz Well.getQueryString() supports load_pixels & load_channels

9139d69

add_data(urls={}) can be used with specified urls

6fec387

Add WellView with add_data() to add urls to Images

bf22ebb

flake8 fixes

43f5971

Use 'url:wells' for wells in Plate json

b4fe99f

Load images for /wells/ and pixels for /wells/:id

f2d4fbf

will-moore reopened this Jan 19, 2017

will-moore force-pushed the api_wells branch from 2ec5d78 to f2d4fbf Compare January 19, 2017 11:59

will-moore added 5 commits January 19, 2017 15:06

Also add 'url:image' to Images in /wells/

af784b9

New test_api_wells.py tests lising /wells/ and /wells/:id/

9b8d9a0

Blitz Well getQueryString() also loads plateAcquisition with wellSample

d645be8

flake8 fixes

ff4a187

Blitz Well _getQueryString uses 'left outer join' for plateAcquisition

7a5ca34

will-moore force-pushed the api_wells branch from c11c1b2 to 7a5ca34 Compare January 23, 2017 10:16

will-moore added 3 commits January 23, 2017 13:33

Added 'url:well' test to test_api_containers.py

d474461

Merge remote-tracking branch 'origin/develop' into api_wells

88637ca

flake8 fixes

3bc0096

jburel reviewed Jan 24, 2017

View reviewed changes

will-moore closed this Jan 26, 2017

will-moore reopened this Jan 26, 2017

dominikl mentioned this pull request Jan 27, 2017

Annotation count web #5023

Merged

Temp use chris' well-wellsample-image branch of omero-marshal

852c3ed

snoopycrimecop mentioned this pull request Jan 30, 2017

Api save whitelist #5059

Merged

chris-allan approved these changes Jan 30, 2017

View reviewed changes

Don't use urls={} in ApiView.add_data()

024a188

will-moore closed this Jan 31, 2017

will-moore reopened this Jan 31, 2017

This was referenced Feb 2, 2017

Api wells2 #5069

Merged

Api pagination #5067

Merged

Api filter by children #5088

Merged

Revert "Temp use chris' well-wellsample-image branch of omero-marshal"

8eb3ad2

This reverts commit 852c3ed.

atarkowska reviewed Feb 13, 2017

View reviewed changes

jburel merged commit f9255a2 into ome:develop Feb 13, 2017

jburel added this to the 5.3.0 milestone Mar 29, 2017

will-moore deleted the api_wells branch February 18, 2019 04:15

Conversation

will-moore commented Jan 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Testing this PR

Related reading

To consider for follow-up:

Uh oh!

will-moore commented Jan 19, 2017

Uh oh!

will-moore commented Jan 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jburel Jan 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jburel commented Jan 26, 2017

Uh oh!

will-moore commented Jan 26, 2017

Uh oh!

will-moore commented Jan 26, 2017

Uh oh!

will-moore commented Jan 26, 2017

Uh oh!

jburel commented Jan 26, 2017

Uh oh!

will-moore commented Jan 26, 2017

Uh oh!

jburel commented Jan 26, 2017

Uh oh!

atarkowska commented Jan 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snoopycrimecop commented Jan 27, 2017

Uh oh!

will-moore commented Jan 30, 2017

Uh oh!

chris-allan Jan 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dominikl commented Feb 10, 2017

Uh oh!

chris-allan commented Feb 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jburel commented Feb 13, 2017

Uh oh!

Reviewers

will-moore commented Jan 18, 2017 •

edited

Loading

jburel Jan 25, 2017 •

edited

Loading

atarkowska commented Jan 26, 2017 •

edited

Loading

chris-allan Jan 30, 2017 •

edited

Loading