Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extenstions to populate metadata 52 #4426

Merged
merged 97 commits into from Feb 25, 2016

Conversation

joshmoore
Copy link
Member

A rebase to 5.2 of @emilroz' port of all metadata PRs involving populate_metadata.


This includes:


Description of cherry-picking:

Extensions to the populate metadata functionality.

Cherry-picked from:

To ensure no merge conflicts also picked a commit 6ed797d from #3875

#4251 and #4240 are conflicting so added extra commit from @joshmoore at the end here.

Because of some weirdness trying to git merge Simon's branch for #4240 was also causing some merge conflicts but they looked absurd so I cherry-picked the branch.

joshmoore and others added 30 commits January 21, 2016 16:23
Under various circumstances the source CSV may be populated with rows
that have no corresponding data in OMERO.  Those were hard failures
during `Well Name` column resolution without this commit.  We are now
more graceful at handling these conditions when the source CSV is less
than ideally formed.
For a server who's policy restricts the download of plates,
populate_roi.py can no longer get access to the results file.
For the moment, we're detecting files based on their file
ending and permitting the download.
@ximenesuk
Copy link
Contributor

The help for populate looks a bit odd:

$ omero metadata populate -h
usage: /Users/colin/Work/repos/openmicroscopy/dist/bin/omero metadata populate
       [-h] [--report] [-n | -f] [--context {csv,bulkmap,deletemap}]
       [--file FILE | --fileid FILEID [--cfg CFG | --cfgid CFGID] [--attach]
       obj

The square brackets don't look balanced. Is it possible to use the form:

$ omero metadata populate Plate:1
...
$ omero metadata populate Plate:1

and if so what is expected to be present for the command to succeed? Could the help be improved here?

Similarly with:

$ omero metadata populateroi Plate:1

what is expected? Incidentally,

ls30778:openmicroscopy colin$ omero metadata populateroi Plate:1
Using session 2f18cac9-b7be-4032-973f-b08c78d7f443 (user-0@localhost:4064). Idle timeout: 10 min. Current group: pg-0
Traceback (most recent call last):
  File "/Users/colin/Work/repos/openmicroscopy/dist/bin/omero", line 125, in <module>
    rv = omero.cli.argv()
  File "/Users/colin/Work/repos/openmicroscopy/dist/lib/python/omero/cli.py", line 1438, in argv
    cli.invoke(args[1:])
  File "/Users/colin/Work/repos/openmicroscopy/dist/lib/python/omero/cli.py", line 952, in invoke
    stop = self.onecmd(line, previous_args)
  File "/Users/colin/Work/repos/openmicroscopy/dist/lib/python/omero/cli.py", line 1029, in onecmd
    self.execute(line, previous_args)
  File "/Users/colin/Work/repos/openmicroscopy/dist/lib/python/omero/cli.py", line 1111, in execute
    args.func(args)
  File "/Users/colin/Work/repos/openmicroscopy/dist/lib/python/omero/plugins/metadata.py", line 482, in populateroi
    ctx = factory.get_analysis_ctx(md.get_id())
  File "/Users/colin/Work/repos/openmicroscopy/dist/lib/python/omero/util/populate_roi.py", line 566, in get_analysis_ctx
    plate_id)
omero.util.populate_roi.MeasurementError: Unable to find suitable analysis context for plate: 1

Is a Plate the expected object type?

@ximenesuk
Copy link
Contributor

The various query subcommands all look okay with some simple checks but it would be good to have better help for the subcommands mentioned above.

@joshmoore
Copy link
Member Author

Pushed fixes.

@joshmoore joshmoore force-pushed the extenstions_to_populate_metadata_52 branch from c4b2768 to 2bf894d Compare February 12, 2016 08:55
@manics manics mentioned this pull request Feb 12, 2016
@joshmoore joshmoore force-pushed the extenstions_to_populate_metadata_52 branch from 2bf894d to 387c33e Compare February 12, 2016 11:14
@joshmoore
Copy link
Member Author

The square brackets don't look balanced.

Weird! I've found some related bug reports, but I haven't yet been able to find a workaround. I'll discuss options with @manics tomorrow.

@joshmoore
Copy link
Member Author

Based on discussion with @manics concerning @ximenesuk's concerns in #4426 (comment) and #4426 (comment), I'm disabling the metadata.py plugin. Argparse's add_mutually_exclusive_group seems to have issues with multiple such groups. We could handle this in code, but then the help would be even more confusing than it currently is. The arguments to metadata.py, however, need discussion anyway, so the best course of action is to not release it for 5.2.2.

In the interim, if someone wants to make use of the plugin, use:

OMERO_DEV_PLUGINS=1 bin/omero metadata

(Suggestions for alternatives welcome)

This plugin needs further API design work.
Rather than deleting it, this disables it
unless an environment variable is set.
@will-moore
Copy link
Member

fs ls plugin seems to work OK:

# first, lookup some filesets...
$ bin/omero fs sets
 #  | Id    | Prefix                          | Images | Files | Transfer 
----+-------+---------------------------------+--------+-------+----------
 0  | 10201 | will_3/2016-02/11/10-40-52.775/ | 16     | 139   |          


$ bin/omero fs ls 10201
will_3/2016-02/11/10-40-52.775/siRNA/001-3657000/results/2008-09-18-14h37m07s.txt
...

The help options for this seem to be clear enough too.

The metadata plugin disabling seems to work

$ bin/omero metadata             # gives 'invalid choice' message

$ OMERO_DEV_PLUGINS=1 bin/omero metadata      # gives regular 'usage' message

I presume the functionality of the plugin has been tested & discussed above and is still a "work in progress"?

I don't see any testing or discussion of the "workaround for binary download" as asked for in the description?

@joshmoore
Copy link
Member Author

I don't see any testing or discussion of the "workaround for binary download" as asked for in the description?

True, thanks for bringing that up, @will-moore. What we ran into was that .csv-like files weren't accessible to scripts if download was turned off. This was probably the easiest change to make the populate_metadata.py etc. scripts work, but it's not ideal. Options I can think of:

  • remove this and manage it on metadata
  • extend this list of exceptions and make it configurable
  • add some other kind of smarts into BinaryPolicy

@will-moore
Copy link
Member

It looks like the download restriction is only ignoring .txt files, not .csv-like files?

@joshmoore
Copy link
Member Author

It looks like the download restriction is only ignoring .txt files, not .csv-like files?

Yup, that was the extension that we were working with with but would fail for other use cases.

@will-moore
Copy link
Member

I see that my changes to populate_metadata.py, to support json encoding of column descriptions is included in this PR. I also have those commits in #4482 and @manics and I were discussing whether we want them in the mainline and whether to document them / expose this as an API? (Also no tests for this yet).

@jburel
Copy link
Member

jburel commented Feb 23, 2016

I have modified the test_screen.csv file attached to the screen Bulk_annotation_test (eel, user-4 read-only-1)
to have "Plate Name" instead of "Plate" for the column. ("Plate" leads to an expected warning)

Traceback (most recent call last):
  File "./script", line 130, in <module>
    message = populate_metadata(client, conn, scriptParams)
  File "./script", line 86, in populate_metadata
    ctx.parse_from_handle(file_handle)
  File "/opt/hudson/workspace/OMERO-DEV-merge-deploy/src/dist/lib/python/omero/util/populate_metadata.py", line 494, in parse_from_handle
    self.post_process()
  File "/opt/hudson/workspace/OMERO-DEV-merge-deploy/src/dist/lib/python/omero/util/populate_metadata.py", line 569, in post_process
    plate = columns_by_name['Plate'].values[i]
KeyError: 'Plate'

cc @pwalczysko @will-moore

Edit: I removed by accident the Plate column. Both columns are now present and no error. but I woul have assumed that plate column was not required.

@joshmoore
Copy link
Member Author

Re: .txt download: see follow-up https://trello.com/c/iwYk0pJi/24-download-restriction-part2 which may need to be moved to 5.2.3.

@jburel
Copy link
Member

jburel commented Feb 25, 2016

cf. my last comment. I decided not to adjust the script to only support "plate name"

@jburel
Copy link
Member

jburel commented Feb 25, 2016

Still some todos. but major improvement so we can certainly have that in and clearly indicate in the history that it is better but not 100% complete cc @hflynn

@jburel
Copy link
Member

jburel commented Feb 25, 2016

Merging this PR. Thanks all

jburel added a commit that referenced this pull request Feb 25, 2016
…ata_52

Extenstions to populate metadata 52
@jburel jburel merged commit d929501 into ome:develop Feb 25, 2016
@sbesson sbesson mentioned this pull request Feb 27, 2016
@joshmoore joshmoore deleted the extenstions_to_populate_metadata_52 branch February 29, 2016 09:46
@jburel jburel added this to the 5.2.2 milestone Feb 29, 2016
@sbesson
Copy link
Member

sbesson commented May 13, 2016

Indirectly confirmed the effect of this PR while reviewing the BinData codegen PR. For our MIAS dataset, an extra set of ROIs is now populated from the measurement file in addition of the ROIs parsed by the Bio-Formats reader. /cc @pwalczysko

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants