search

ome / omero-scripts

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

Populate metadata Datasets and Projects #148

Closed manics closed 5 years ago

manics commented 5 years ago

Requires the omero-metadata plugin. Otherwise reverts to the old behaviour and includes a deprecation warning in the script description.

Updated 20190703: New functionality requires https://github.com/ome/omero-metadata/pull/19

pwalczysko commented 5 years ago 
Traceback (most recent call last):
  File "./script", line 140, in <module>
    run_script()
  File "./script", line 132, in run_script
    message = populate_metadata(client, conn, script_params)
  File "./script", line 88, in populate_metadata
    ctx.parse_from_handle(file_handle)
AttributeError: 'ParsingContext' object has no attribute 'parse_from_handle'

tested locally

pwalczysko commented 5 years ago

@manics I have run the pip list to find the omero-metadata in the venv where my omero.web is installed (localhost). Maybe I should be installing the omero-metadata somewhere else ?

joshmoore commented 5 years ago

cc: @emilroz

manics commented 5 years ago

@pwalczysko This should be working now

pwalczysko commented 5 years ago

First test (locally) on idr0021 passes.

Tested on merge-ci, where the omero-metadata is not installed. The script correctly defaults to just showing the dropdown with Screen, Plate only in this case. Then run through the scenario test on Screen and Plate and got 

WARNING:omero.util.populate_metadata:PlateColumn is unimplemented

Edit: the problem with empty values in the tables when the test scenario is run on Screen https://docs.openmicroscopy.org/internal/testing_scenarios/BulkAnnotations.html reported in this comment above is not a regression - was able to reproduce it precisely with the old script (script as it was prior to this PR) on the same server (merge-ci, user-3)

Edit2: the problem is only on Screens, not on orphaned Plates - again, this is not a regression and has nothing to do with this PR - this PR seems to preserve the functionality of the old script correctly.

pwalczysko commented 5 years ago

on merge-ci, after I did pip install omero-metadata and redeployed the server, I get no warning in the UI about the outdated plugin which is used (as expected) and the dropdown has now also the Project and Dataset items (as expected).

Two occasions of the below error though, both logged in as

user-2

  1. https://merge-ci.openmicroscopy.org/web/webclient/?show=project-3059
  2. https://merge-ci.openmicroscopy.org/web/webclient/?show=plate-3001
Traceback (most recent call last):
  File "./script", line 159, in <module>
    run_script()
  File "./script", line 151, in run_script
    message = populate_metadata(client, conn, script_params)
  File "./script", line 107, in populate_metadata
    ctx.parse_from_handle_stream(data)
  File "/home/omero/workspace/OMERO-server/omero-virtualenv/lib/python2.7/site-packages/populate_metadata.py", line 964, in parse_from_handle_stream
    self.create_file_annotation(table)
  File "/home/omero/workspace/OMERO-server/omero-virtualenv/lib/python2.7/site-packages/populate_metadata.py", line 1090, in create_file_annotation
    original_file = table.getOriginalFile()
  File "/home/omero/workspace/OMERO-server/OMERO.server/lib/python/omero_Tables_ice.py", line 1038, in getOriginalFile
    return _M_omero.grid.Table._op_getOriginalFile.invoke(self, ((), _ctx))
Ice.ObjectNotExistException: exception ::Ice::ObjectNotExistException
{
    id = 
    {
        name = Table-0127863d-28d8-4628-9255-15fe91d1f0be
        category = dacc667e-ee69-44d5-bfd7-dfb1f4dec17d
    }
    facet = 
    operation = getOriginalFile
}
joshmoore commented 5 years ago

This sounds quite similar to https://github.com/openmicroscopy/openmicroscopy/pull/6066

pwalczysko commented 5 years ago

When I run the script on Screen with mias/frans plate in it, as per scenario https://docs.openmicroscopy.org/internal/testing_scenarios/BulkAnnotations.html

user-2, https://merge-ci.openmicroscopy.org/web/webclient/?show=screen-3001

I get

WARNING:omero.util.populate_metadata:Screen is missing plate: NameOfThePlate
WARNING:omero.util.populate_metadata:PlateColumn is unimplemented
Traceback (most recent call last):
  File "./script", line 159, in <module>
    run_script()
  File "./script", line 151, in run_script
    message = populate_metadata(client, conn, script_params)
  File "./script", line 106, in populate_metadata
    ctx.preprocess_from_handle(data_for_preprocessing)
  File "/home/omero/workspace/OMERO-server/omero-virtualenv/lib/python2.7/site-packages/populate_metadata.py", line 938, in preprocess_from_handle
    self.preprocess_data(reader)
  File "/home/omero/workspace/OMERO-server/omero-virtualenv/lib/python2.7/site-packages/populate_metadata.py", line 1028, in preprocess_data
    self.post_process()
  File "/home/omero/workspace/OMERO-server/omero-virtualenv/lib/python2.7/site-packages/populate_metadata.py", line 1154, in post_process
    plate = columns_by_name["plate"].values[i]
IndexError: list index out of range
pwalczysko commented 5 years ago

@joshmoore I did not detect this problem locally, running the script on a 5.5.0 release server.

joshmoore commented 5 years ago

If you're referring to the getOriginalFile error, then yes, that's to be expected. If this is so pervasive, then perhaps we have to consider this a breaking change. The line https://github.com/ome/omero-metadata/blob/master/src/populate_metadata.py#L1090 is the risky one. Note: this also points to a leak though so I'm hesitate to say we should do something.

pwalczysko commented 5 years ago

@joshmoore

If you're referring to the getOriginalFile error, then yes, that's to be expected. If this is so pervasive, then perhaps we have to consider this a breaking change. The line https://github.com/ome/omero-metadata/blob/master/src/populate_metadata.py#L1090 is the risky one. Note: this also points to a leak though so I'm hesitate to say we should do something.

Yes, I do refer to the getOriginalFile error. FMPOV, it would be better to have this PR here in and having the user have a way to create a table from a csv than fixing the complex non-closing problematics.

This PR here is blocked atm by the inability to test its performance on merge-ci because of https://github.com/ome/scripts/pull/148#issuecomment-504477060

pwalczysko commented 5 years ago

@manics https://github.com/ome/scripts/pull/148#issuecomment-504478246 seems to be a Screen-specific, separate problem which repeats every time when the script is run on a Screen. But again, running on a Screen did not work in the first place in the old script... nevertheless, with a different output.

will-moore commented 5 years ago

As discussed with @pwalczysko the issue at https://github.com/ome/scripts/pull/148#issuecomment-504478246 was the csv Plate column still had NameOfThePlate placeholder values. When we replaced those with the actual name of the Plate, this error went away (with the csv attached to the Screen).

pwalczysko commented 5 years ago

On outreach server, as expected, the script as edited by this PR works fine. Again, OMERO.parade is having problems ot open the resulting table, but this is fixed by @will-moore in https://github.com/ome/omero-parade/pull/55

pwalczysko commented 5 years ago

Opened https://github.com/manics/scripts/pull/1 to fix the problem mentioned in https://github.com/ome/omero-metadata/pull/23#issuecomment-508146327 cc @joshmoore

Witch https://github.com/manics/scripts/pull/1 and https://github.com/ome/omero-metadata/pull/23 and https://github.com/ome/omero-metadata/pull/19 all works on merge-ci as epected

joshmoore commented 5 years ago

I was going to say:

Merging for an initial build of 5.5.1 that will be deployed to pub-omero before proper release.

but then I noticed https://github.com/ome/scripts/pull/148/commits/b268cda87f1745f7db0bf12f48ace495005517ad

Can anyone explain this change further? Is this not a breaking change?

cc: @manics @pwalczysko @will-moore

will-moore commented 5 years ago

That was my suggestion which doesn't change the usage of the script in the UI but simply means that you're not going to get an invalid String when you're really wanting an ID. I guess it could be considered a breaking change if you're running the script via code. I didn't consider that because the whole point of the script is to provide a UI entry point to the underlying populate_metadata script, so I don't expect any code to be passing a string here, but I guess it's possible. Happy for it to be reverted.

joshmoore commented 5 years ago

Reverted. The change certainly makes sense, but if we want to make it, we probably need either a bigger bump or to deprecate File_Annotation in favor of File_Annotation_ID (or similar). I don't know if we have a clear deprecation mechanism for script properties though.

joshmoore commented 5 years ago

Today's build was green. Including this under the assumption that everything still works (to be re-confirmed by @pwalczysko). I'll get a 5.5.1 build together and deployed so that we have some time to test.

pwalczysko commented 5 years ago

@joshmoore still works as expected with omero-metadata==0.4.0 on https://latest-ci.openmicroscopy.org/web/webclient/ user-3 - tested both Plate and Project

pwalczysko commented 5 years ago

This is good now, and we have some progress. As discussed with @will-moore , it would be great to have this script to create tables with numbers where there are numbers in the csv. Atm, the script always creates strings.