api experimenters groups

[will-moore]

Porting BlitzGateway code from ome/openmicroscopy#5315

This adds options for NOT loading the groupExperimenterMap when loading Groups and Experimenters.
Using the opts argument:

groups = conn.getObjects("ExperimenterGroup", opts={'load_experimenters': False})
experimenters = conn.getObjects("Experimenter", opts={'load_experimentergroups': False})

Providing more control over loading of groups/experimenters was also part of proposed performance improvements in ome/openmicroscopy#6081 (not merged).

This will allow us to support experimenters and groups URLs e.g. api/v0/m/experimenters/1/ without unnecessary loading of groups - see #196

Tests added at ome/openmicroscopy#6224

[sbesson]

The introduction of an option to switch between various behaviors of the API is certainly very useful. Has there been any discussion about not modifying the default behavior i.e. constructing this change as a backwards-compatible API addition that downstream clients could consume rather than a breaking change?

[manics]

Would this be released as omero-py 6.0.0? Alternatively the gateway could be broken out into a separate package.

Are there any other objects that might benefit from a similar option? If so it'd be nice to avoid option keys being added adhoc, it's difficult enough as it is to find out about the extra options to some BlitzGateway methods.

[will-moore]

I'd like to change the default behaviour to make it more consistent with other parts of the API.
E.g. conn.getObject("Project", id).name doesn't load Datasets.
Currently, conn.getObject("Experimenter', id).firstName loads all the Groups which is unexpected and can be slow if user is in lots of groups.
Same for conn.getObject("ExperimenterGroup', id).name. We don't want that to load lots of users by default.
I guess if we're not ready to release a breaking change, then the default load_experimenters and load_experimentergroups could be True. Then we can flip the default at the time of the next major release.

We have similar flags for Wells (load_images), Images (load_pixels, load_channels) and ROIs (load_shapes).

These options are mentioned in the _getQueryString() method of each class, e.g. https://downloads.openmicroscopy.org/omero/5.5.1/api/python/omero/omero.gateway.html#omero.gateway._ImageWrapper._getQueryString but certainly documentation of these could be improved.

[will-moore]

Now we're loading groupExperimenterMap by default (no breaking change). Description updated.
And tests updated at ome/openmicroscopy#6224

[joshmoore]

👍 for the non-breakingness. The contortions that were needed (with the hot swapping, etc) make me thing that this could all use a refactoring at some point in the future, but it looks like the impact of this PR is as intended.

Generally, good to go though I note that the tests weren't included from ome/openmicroscopy#6224 due to:

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

I've relaunched, but you might want to recheck.

[will-moore]

All tests currently passing, including the new tests in this PR: https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/350/testReport/OmeroWeb.test.integration.test_api_experimenters_groups/

[will-moore]

Travis failing with

INTERNALERROR>     sugar_reporter = SugarTerminalReporter(standard_reporter)
INTERNALERROR>   File "/home/travis/build/ome/omero-py/.tox/py36/lib/python3.6/site-packages/pytest_sugar.py", line 214, in __init__
INTERNALERROR>     self.writer = self._tw
INTERNALERROR> AttributeError: can't set attribute
ERROR: InvocationError for command /home/travis/build/ome/omero-py/.tox/py36/bin/pytest -n4 -m 'not broken' --reruns 5 -rf test (exited with code 3)

[manics]

Travis tests are still broken in this repo #198

[joshmoore]

Yup, more hands/eyes trying to figure #198 out welcome.

[will-moore]

Fixed a bug found when adding more tests to ome/openmicroscopy#6224

[joshmoore]

Not sure what the travis failure was. Relaunched.

[will-moore]

I used to be able to re-launch jobs from within travis, but I don't see that option now so I just have to close and open the PR...

[will-moore]

Google-ing for the tox failure found someone who fixed it like this: googleapis/google-cloud-python#1103 (comment)

[joshmoore]

Re-opening with #216 merged.

[joshmoore]

@will-moore @manics : re-opening with #216 in fixed the issue.

[jburel]

Looking at the implementation methods like getObject(Experimenters) it might be a good idea to think of pushing the logic to AdminService e.g. public List<Experimenter> lookupExperimenters() and offer similar flexibility to the method so both Java/Python will have similar implementation.

[will-moore]

getObject() uses the same logic for all the different types it handles. It would be quite tricky and messy to use the Admin Service for some queries and the Query Service for others.
But nothing prevents us from also adding functionality to the Admin Service.

[jburel]

I understand that but I think using the relevant service when possible instead of directly using the "queryService" is an approach that simplify the maintenance.

[will-moore]

using the relevant service when possible instead of directly using the "queryService"

This is not the approach we've taken with the BlitzGateway.
conn.getObject() and conn.getObjects() return different subclasses of BlitzObjectWrappers. Each class knows the query needed by iQuery to load the underlying omero.model objects. The base query from each object uses the correct syntax (e.g the selected object itself is obj) so that the query can be extended, e.g. filtering or ordering by obj.attribute. The query returned by each wrapper class is passed to the query service.

If every class used a different service for loading different objects, there would be no consistency and much more complex maintenance.

[jburel]

If every class used a different service for loading different objects, there would be no consistency and much more complex maintenance.

I am not convinced by that. A change in the model as we have seen in the past implies multiple locations to update: the service and the "iquery" usage.
I am not suggesting to change the approach but the "adminService" methods should not be forgotten.

[will-moore]

So is this good to merge or any suggested changes?

[jburel]

The default behaviour is preserved (load experimenter). Could you add an issue for AdminService so we keep a record and work on maybe adding the functionality to the AdminService.
I will check with him tomorrow to see if @sbesson wants to include this PR in the release he is preparing.

[sbesson]

Assuming this PR is approved, I am happy to move forward with anomero-py 5.7.0 release including this API addition as well as the chown gateway one. I will review other mergeable PRs tonight.

[will-moore]

Created issue at ome/openmicroscopy#6238

[sbesson]

Thanks @will-moore. @jburel okay to get this merged with the issue created?

[jburel]

Happy to include it in the coming release

[sbesson]

@will-moore can you prepare a CHANGELOG PR for all merged PRs in the 5.7.0 milestone and we can work together on the release of omero-py?