-
Notifications
You must be signed in to change notification settings - Fork 10
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
Parallelize the set command #33
Conversation
86bc15c
to
c5cd1e0
Compare
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.
Few code review comments. Haven't started functional testing yet. (Integration tests likely to be tricky I assume)
src/omero_cli_render.py
Outdated
img.setDefaultZ(def_z - 1) | ||
if def_t: | ||
img.setDefaultT(def_t - 1) | ||
if args.batch > 1: |
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.
I could imagine using the futures method even if the batch is 1 to keep things simpler.
src/omero_cli_render.py
Outdated
iid = future.result() | ||
iids.append(iid) | ||
except Exception as exc: | ||
print('%d generated an exception: %s' % (img.id, |
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 should be a call to self.ctx.err
(or possibly ctx.out
if there's nothing on stdout that anyone might use).
src/omero_cli_render.py
Outdated
for img in self.load_images(self.gateway, args.object, batch=1): | ||
iid = self._set_rend(args, data, img, cindices, greyscale, | ||
rangelist, colorlist) | ||
iids.append(iid) | ||
|
||
if not iids: | ||
self.ctx.die(113, "ERROR: No images found for %s %d" % |
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.
Perhaps also use self.ctx.die (with a different error number?) if any exceptions are thrown.
@@ -414,7 +420,7 @@ def _lookup(self, gateway, type, oid): | |||
self.ctx.die(110, "No such %s: %s" % (type, oid)) | |||
return obj | |||
|
|||
def render_images(self, gateway, object, batch=100): | |||
def load_images(self, gateway, object, batch=100): |
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.
Can you explain your thoughts on this rename?
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.
Because the method doesn't render any images, it just loads the images. I found the name render_images
totally confusing every time I looked into the omero_cli_render code.
@@ -396,6 +397,11 @@ def _configure(self, parser): | |||
help="Local file or OriginalFile:ID which specifies the " | |||
"rendering settings") | |||
|
|||
set_cmd.add_argument( | |||
"--batch", type=int, default=1, |
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.
Have you considered a higher default?
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.
Could do. Just didn't want to change the default behaviour if the new option is omitted.
Wait for #34 to be merged. |
Adds a
--batch
option to the set command.Test:
Get rendering settings from an image:
Then apply it to a whole Dataset, Project or whatever (with compatible images).
Example:
Needs ome/omero-py#129 to work.