search

ome / omero-scripts

Core OMERO Scripts
https://pypi.org/project/omero-scripts/
12 stars 32 forks source link

bom fix populate metadata #180

Closed will-moore closed 3 years ago

will-moore commented 3 years ago

Fixes #179

To test, see ticket above for how to reproduce (create csv with bom).

cc @pwalczysko

joshmoore commented 3 years ago

The copy of the class is so that the next version of the scripts doesn't depend on a particular version of omero-py?

will-moore commented 3 years ago

Yes. Maybe I'm still thinking of "release of server-side python code is costly", and it's not such a big deal to simply use NamedTemporaryFile in the util class, and release omero-py. But is there a way we can make the omero-scripts depend on the updated version of omero-py?

pwalczysko commented 3 years ago

Tested on

  1. Dataset
    • no BOM
    • with BOM
  2. Plate
    • no BOM
    • with BOM

Works as expected. Ready to merge fmpov. Thank you @will-moore

joshmoore commented 3 years ago

But is there a way we can make the omero-scripts depend on the updated version of omero-py?

Not directly, no. To get new scripts one will (almost certainly) need to download a new OMERO.server.zip. In those instructions, we already say to pip install -U but we could be more explicit about it. Perhaps the script could catch the import exception and say as much?

will-moore commented 3 years ago

I opened https://github.com/ome/omero-py/pull/274 so when that's released, we could revert the duplication here. Not sure how this script would check for that change. No imports changed etc. In due course, when we remove the duplicated code from the script, the script could handle the returned file NOT providing a temp.name and return a suitable error ("please upgrade omero-py").

pwalczysko commented 3 years ago

In due course, when we remove the duplicated code from the script, the script could handle the returned file NOT providing a temp.name and return a suitable error ("please upgrade omero-py").

This is kind of a similar topic which was pointed out https://github.com/ome/omero-guide-upload/pull/13#discussion_r561761470 by @manics - the script is dependent on omero-metadata plugin as well.

jburel commented 3 years ago

I opened ome/omero-py#274 so when that's released, we could revert the duplication here. Not sure how this script would check for that change. No imports changed etc. In due course, when we remove the duplicated code from the script, the script could handle the returned file NOT providing a temp.name and return a suitable error ("please upgrade omero-py").

A new version of omero-py has been released

will-moore commented 3 years ago

That last commit should detect if omero-py 5.9.1 is not installed and tell users to upgrade. Screenshot from running with omero-py 5.9.0 (upgrade message) then running with omero-py 5.9.1 (works):

Screenshot 2021-03-25 at 11 27 51

will-moore commented 3 years ago

Tests are failing since they appear to still be using omero-py 5.9.0 or older?

pwalczysko commented 3 years ago

Tests are failing since they appear to still be using omero-py 5.9.0 or older?

As far as I understand it, the tests are pulling omero-server from https://hub.docker.com/layers/openmicroscopy/omero-server/latest/images/sha256-1947bf3cb3a92171455193c970b6d98cc50027d2f40363b1a94a839c126280f2?context=explore and this image was not updated since 1 month. Thus imho it cannot contain the omero-py 5.9.1 from 22 March and is using omero-py 5.9.0. But maybe I am wrong cc @joshmoore