-
Notifications
You must be signed in to change notification settings - Fork 8
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
Provide further command-line options. #12
Conversation
|
Not sure what that is supposed to do. I thought the duplicate by default does that anyway (duplicate a project implies duplicating all its Datasets and all the images in those datasets. What am I missing here ? |
I would suggest replacing the work |
Wonder when would I use that ? An example would be handy. Aha, sorry, it is there, in the last example |
Could we maybe go away from the word |
By default duplicate links duplicate images into the datasets, not the originals. "something more precise" goes way off the end of the line, that's okay? I was trying to do what I could while keeping it short. "classes" to match the description of the other options like "--include", they seem to be classes elsewhere in CLI-speak. "objects" I'd be more likely to interpret as being instances rather than kinds. |
More okay imho than to leave room for unclarity. |
Trying to link some annotations on the original to the duplicates in read-only group leads to failure, as kind of expected ... We need to think about this case at least for the webclient though @will-moore (in the example below, user-8 is trying to duplicate user-2's Project and link the Tags from the original to the duplicate by passing
|
All taken on #12 (comment). Re-reading the help, I think my main tripping point is that I did not get the
|
Problem when tried to duplicate a dataset with exclusion of ROIs and FileAnnotations.
|
That one's a tricky one. In read-annotate you can reference the attachments but in read-only you may need the blunt instrument of |
That will be hard to explain to users. I actually do not understand myself why that should be the case. Is it possible to change it ? Also, the behaviour is actually erratic then - or at least it appears so. I have first run the command with exclusion of Roi only, and it did not error, it was just not returning for a long time. |
Not easily changed. Could be worth an issue but there is already a ton of graphs-related RFE (if we include Trello cards). The issue is basically one of lookahead and class distinctions. RoIs don't have a separate kind of linking object at all so they don't run into this issue, you should be able to In read-annotate at least one can fall back to referencing instead so the duplicate link still has somewhere to point but the class-based selectors and same-link-kinds mean that for read-only one can ignore all links from datasets to annotations or none. |
suggested by @pwalczysko
Tried to do just that, but that failed too, see below
|
Try |
@mtbc yes, thank you. I think I got it finally, see below please. Although finally successful, we should think how to give this as an example in the help text, because no way it is intiutive or even deducible.
|
src/omero/plugins/duplicate.py
Outdated
" omero duplicate Project:15 --reference-classes=Annotation " | ||
"--duplicate-classes=CommentAnnotation,LongAnnotation\n" | ||
""" | ||
Group permissions can prevent simply referencing an Image or Annotation. |
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.
Group permissions can prevent simply referencing an Image or Annotation. | |
Group permissions can prevent simply linking an Image or Annotation. |
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.
Maybe this is not optimal suggestion.
How would "referencing for linking" sound ? Or maybe Group permissions can prevent simply referencing an Image or Annotation because the subsequent linking is not possible.
?
One formulation suggestion, otherwise looks good and works as expected. |
The last two commits look good as well. |
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 CLI usability comments.
src/omero/plugins/duplicate.py
Outdated
@@ -61,6 +72,38 @@ def cmd_type(self): | |||
import omero.all | |||
return omero.cmd.Duplicate | |||
|
|||
def _pre_objects(self, parser): | |||
parser.add_argument( | |||
"--duplicate-classes", |
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 see dropping --classes
here and perhaps adding a short-cut, -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.
It's largely the default anyway.
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 don't assume there's a way to list what the server will do by default, is there?
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 is basically what --dry-run
does but, generally, the name of the subcommand is a big hint.
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.
Hmm... I don't know.
omero duplicate --dry-run Image:160 --report
omero.cmd.Duplicate Image:160 Dry run performed
...
Duplicates
Arc:107
Detector:22
DetectorSettings:22
...
doesn't really give me a sense of the default parameters to know what I can change from "duplicate" to "reference". Are there any that default to "reference" (or even "ignore")?
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.
Nothing's jumping to my mind apart from things like how details.owner
are effectively ignored but that can't be overridden with --duplicate
anyway. Generally the only "do a different thing by default" going on with graphs operations is when it switches to deleting, e.g., removing somebody else's comments when you move an image into a private group (also not overridable), but duplicate never switches to deletion.
src/omero/plugins/duplicate.py
Outdated
def _pre_objects(self, parser): | ||
parser.add_argument( | ||
"--duplicate-classes", | ||
help=("Modifies the given option by specifying kinds of object to " |
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 see dropping "Modifies the given option by". Perhaps, "Specify kind of object to duplicate"
src/omero/plugins/duplicate.py
Outdated
help=("Modifies the given option by specifying kinds of object to " | ||
"link to instead of duplicate")) | ||
parser.add_argument( | ||
"--ignore-classes", |
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.
These could conceivable be marked nargs="+"
to handle -R Foo -R Bar
.
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.
That one's new to me so have created ome/omero-py#253 as a start.
src/omero/plugins/duplicate.py
Outdated
help=("Modifies the given option by specifying kinds of object to " | ||
"duplicate")) | ||
parser.add_argument( | ||
"--reference-classes", |
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.
choices = tuple([x[:-1] for x in dir(omero.model) if x.endswith("I")])
or similar would be able to detect typos in the arguments.
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.
Unfortunately that includes classes like Event
which aren't permitted and omits the example's Annotation
which is; I don't think we have an easy way here to get a good list of choices.
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.
Encoding that logic along with the other base graph methods would probably produce something maintainable:
def get_graph_targets():
- load all classes
- remove events and other known problems
- add "Annotations" and other necessary base classes
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.
If not asking the server then probably best done with some codegen adjustments. Marker interfaces like IAnnotationLink
are also available.
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.
Noted in ome/omero-blitz#107.
src/omero/plugins/duplicate.py
Outdated
@@ -28,15 +28,10 @@ | |||
|
|||
from omero.cli import CLI, GraphControl | |||
|
|||
HELP = """Duplicate OMERO data. | |||
HELP = ("""Duplicate OMERO data. |
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 3 word intro could likely be dropped in favor of the next sentence:
usage: ./.venv3/bin/omero duplicate [-h] [--wait WAIT] [--include INCLUDE]
[--exclude EXCLUDE] [--ordered] [--list]
[--report] [--dry-run] [--force]
[--duplicate-classes DUPLICATE_CLASSES]
[--reference-classes REFERENCE_CLASSES]
[--ignore-classes IGNORE_CLASSES]
[obj [obj ...]]
Duplicate OMERO data.
Duplicate entire graphs of data based on the ID of the top-node.
" omero duplicate Project:15 --reference-classes=Annotation " | ||
"--duplicate-classes=CommentAnnotation,LongAnnotation\n" | ||
""" | ||
Group permissions can prevent a duplicated link from referencing another |
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'm skeptical that this paragraph is going to be understandable by most. Is there any chance of detecting this particular error and then saying, "you will need to modify your parameters"?
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.
Or perhaps at least, I'd begin with a more friendly text describing things in general, and then prefix this paragraph with Warning/Details/Notice etc.
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.
https://trello.com/c/F4cAE0k3/358-graphexception-subclasses may be the best approach there though it'd also require analysis to figure what else can cause the same condition.
src/omero/plugins/duplicate.py
Outdated
help=("Modifies the given option by specifying kinds of object to " | ||
"duplicate")) | ||
parser.add_argument( | ||
"--reference-classes", |
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.
Add metavar="CLASS"
(or "TYPE")
suggested by @joshmoore
For testing one can adjust the |
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 minor comments but the help is generally looking much more usable. Thank you.
src/omero/plugins/duplicate.py
Outdated
By default, a whole subtree of OMERO model objects is duplicated. One | ||
may opt to have duplicate objects reference original parts of the | ||
subtree instead of also duplicating those, see the "--reference" option | ||
below. More strongly, one may "--ignore" given kinds of model object |
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.
model object
seemed odd here in the singular.
@@ -61,6 +78,44 @@ def cmd_type(self): | |||
import omero.all | |||
return omero.cmd.Duplicate | |||
|
|||
def _pre_objects(self, parser): | |||
parser.add_argument( | |||
"--duplicate", |
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.
You decided against the short-cuts?
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.
Yes, given that we shortened from --duplicate-classes
and I imagined you might have initially thought --duplicate
would be used more often than I'd guess, given that it overrides overrides to put them back to default. We haven't much done it elsewhere, e.g., for --include
or --dry-run
or whatever. Could still do it if it brings enough value, maybe capital letters to reduce conflicts with non-graph-command options.
src/omero/plugins/duplicate.py
Outdated
"--duplicate=CommentAnnotation,LongAnnotation\n" | ||
""" | ||
Group permissions can prevent a duplicated link from referencing another | ||
user's Image or Annotation. However, note that ignoring a linked-to |
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.e. it will throw an exception?
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.
@joshmoore Yes, see #12 (comment)
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.
Sorry, I'm pointing out we may want to say that to the user.
""" | ||
" # Duplicate a project, linking to the original annotations " |
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.
Is there a reason for the extra """
and separately quoted lines here?
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.
Minor point - shouldn't we use Project
instead of project
to refer to objects? Also it's more consistent with the commands themselves.
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.
Stops flake8 complaining about the long line.
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'm trying to be consistent with the other CLI graphs command help text, this PR was originally just adapted from GraphControl
methods which is why some of the suggested changes here I've mirrored back into omero-py. Certainly open to reworking it all but perhaps could be a separate effort given how many related subcommands there are.
I like the new wording of the help. I would
|
Oops, now more commits than ome/omero-blitz#100. 😆 |
For most people familiar with the command line, it should be understood that the equals is optional. |
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.
Today's help looks good. The issue with the short arguments (e.g. -D
) can be addressed later. Release (0.4.0?) will need to be coordinated.
Fixes #7 and aids ome/design#20 by adding command-line options to cover those of omero::cmd::Duplicate.