-
Notifications
You must be signed in to change notification settings - Fork 100
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
render.py: new CLI plugin for dealing with rendering settings (manual rebase onto metadata52) #4542
Conversation
Update with a generic yaml/json file util |
It adds unnecessary complexity
Without this the content ("binary") is ignored
Tests added. One is Error in the Python test library fixed. |
Re yaml vv json: I can't help feeling that at some point it will be useful to support json in some format - simply because it is so ubiquitous and is the format used in the web API. I could also imagine grabbing rendering settings from an image I'm looking at in web and doing something like:
|
TBD? |
I assume |
and this is presumably work in progress? |
Would it be useful to get the Object:id forms back for things like RenderDef and Channel? Either in this command or this command with an option? (It is possible |
@will-moore : I had thought about this, but 2 issues:
|
@ximenesuk : the TBDs can mostly be assumed mine. Probably best to focus on if any of the implemented functionality is incorrect or any of the design needs further discussion. (@will-moore, I like the idea of auto-supporting multiple formats based on the file ending. I think https://github.com/openmicroscopy/openmicroscopy/pull/4542/files#diff-7a4e03af84e54b56d8c2247958cb1998 should make that seamless, but we'd have to check with @manics) |
@joshmoore understood, though documenting some of the TODOs may help in their not being forgotten. |
@manics do you have an example of a minimal YAML file that should work? This minimal JSON file does what I expect it to do:
This similar YAML file doesn't.
I get this error
I may be misunderstanding the YAML syntax, though a few variations give the same error. Other than this problem it looks like it does happily deal with both file formats by extension /cc @will-moore @joshmoore One further suggestion would be having |
Other than the YAML glitch above (which may be my misunderstanding) the plugin looks like it would be a useful addition. It certainly works as expected where implemented. |
All items mentinoed captured in https://trello.com/c/8F8QMGr7/276-render-py-followup I'd encourage any follow-up work on this PR to assume that it will be rebased on top of this PR before hitting the mainline. |
For mitocheck, support for "greyscale" (see line 192) is needed in the yaml file (unless I'm overlooking it) |
@will-moore @joshmoore Correct, it chooses between yaml or json depending on mimetype if available (
@will-moore @ximenesuk I thought about this to enable a round-trip, but wanted some feedback on the general design before devoting more time. If everyone's happy I can see
You need a space after the
@joshmoore Not implemented yet |
Happy for that to be outwith this PR (in general, ok to merge this?) but it will likely be needed for next week. |
@joshmoore If this isn't blocking anything I'll do further development here, if not merge it and I'll open another PR. |
Nope, merging isn't blocking anything. |
There's lot's of potential for refactoring in the CLI, including using |
@@ -28,7 +28,7 @@ | |||
from omero.model import Image | |||
from omero.model import Plate | |||
from omero.model import Screen | |||
from omero.util import pydict_text_readers | |||
from omero.util import pydict_text_io |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks https://github.com/openmicroscopy/infrastructure/pull/61/files#diff-bf4435d4dde93c8ad0a4566ee4315a52R14 -- to be corrected in a follow-up PR.
Tested the latest changes with today's build (67). |
--rebased-to #5215 |
This was the same as gh-4385 gh-4430 but rebased onto
metadata52
. Now with additional commits.See the
bin/omero render -h
output and the test for expected behavior. Primary driver for the creation ofrender.py
was scripting the rendering settings along with the imports so that everything could be re-done if necessary:See IDR/idr-metadata#75 for information on the intended workflow for
omero render edit
.Perhaps the main discussion point is the use of yaml/json for these settings, and the implementation of
pydict_text_readers.py
.load
is designed to transparently handle:/local/file/path
s or remoteOriginalFile:Id
sobj
/tree
plugin CLI: graph extensions design#31 in which casepydict_text_readers.py
should be renamed.Additional commits implement
omero render edit
to allow rendering settings to be set on an image.Too many open files
errors on the server, and I'm not sure if it's an error in this code, a system misconfiguration, or an OMERO.server bug