Skip to content

CLI duplicate command#4574

Merged
joshmoore merged 22 commits intoome:developfrom
ximenesuk:duplicate-command
May 16, 2016
Merged

CLI duplicate command#4574
joshmoore merged 22 commits intoome:developfrom
ximenesuk:duplicate-command

Conversation

@ximenesuk
Copy link
Copy Markdown
Contributor

This PR adds a basic duplicate command to the CLI, this should duplicate simple objects and hierarchies. It includes integration tests for simple objects.

Testing

See:

$ omero duplicate --help
...
Examples:

    # Duplicate a dataset
    omero duplicate Dataset:50
    # Do the same reporting all the new duplicate objects
    omero duplicate Dataset:50 --report

    # Do a dry run of a duplicate reporting the outcome
    # if the duplicate had been run
    omero duplicate Dataset:53 --dry-run
    # Do a dry run of a duplicate, reporting all the objects
    # that would have been duplicated
    omero duplicate Dataset:53 --dry-run --report

Multiple IDs and objects should also work:

$ omero duplicate Dataset:263,264
$ omero duplicate Dataset:263 Dataset:264

In addition hierarchies (P/D/I for example) and other complex objects should be duplicated. Create a simple P/D/I structure or import a Screen, then:

$ omero duplicate Project:123 --report
$ omero duplicate Screen:321 --report

the --report option is useful here to check the graph duplication.

To do

Once this PR is merged @pwalczysko plans to add further tests for multiple arguments, simple hierarchies (P/D/I) and complex objects (Screen).

The command itself needs improving to deal with the options in @mtbc's underlying command.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo (Datset)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least that is in my code and not @pwalczysko's! I noticed it when copying it to the PR description but thought I could sneak it through/leave it in case there are other changes.

@joshmoore
Copy link
Copy Markdown
Member

Probably the only other question from my side is whether or not to include this in 5.2.3 and/or hide it via OMERO_DEV_PLUGINS awaiting ome/design#31 discussions.

@ximenesuk
Copy link
Copy Markdown
Contributor Author

Reopening with @pwalczysko being identified as a contributor.

@ximenesuk ximenesuk reopened this Apr 14, 2016
@ximenesuk
Copy link
Copy Markdown
Contributor Author

@joshmoore re #4574 (comment) I'd say it works as a stand-alone command even though it may be that another command eventually subsumes it. However, at the moment it is limited by being an all or nothing duplicate. I'd vote for allowing it in to 5.2.3 as a useful utility but, for now, hiding it from non-developers.

self.ctx.out(" %s:%s" % (k, objIds[k]))

try:
register("duplicate", DuplicateControl, HELP)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still needs the DEV marker: see affa152

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add today - I hadn't realised you had 👍ed my comment as I don't seem to get notifications of reactions.

@mtbc
Copy link
Copy Markdown
Member

mtbc commented Apr 19, 2016

omero duplicate --help now doesn't seem to work. Is there a special trick since the OMERO_DEV_PLUGINS change?

@ximenesuk
Copy link
Copy Markdown
Contributor Author

Have you added OMERO_DEV_PLUGINS to your env?

@mtbc
Copy link
Copy Markdown
Member

mtbc commented Apr 19, 2016

No. What value do I put in it? (Is this documented anywhere?)

@ximenesuk
Copy link
Copy Markdown
Contributor Author

ximenesuk commented Apr 19, 2016

No value, just export OMERO_DEV_PLUGINS= will do.

@mtbc
Copy link
Copy Markdown
Member

mtbc commented Apr 19, 2016

Goodness, so it does, thank you!

@ximenesuk
Copy link
Copy Markdown
Contributor Author

I don't think there is any documentation yet other than its use in

affa152

/cc @joshmoore

@manics
Copy link
Copy Markdown
Member

manics commented Apr 19, 2016

It's for dev/internal use, so we probably don't want to document it.

@sbesson
Copy link
Copy Markdown
Member

sbesson commented Apr 19, 2016

Tested using the daily DEV merge build. All worked as expected both with and without --dry-run mode. Also tested in the permissions context while duplicating other's data. Integration tests are passing. This is ready to merge.

On the OMERO_DEV_PLUGINS topic, I would be inclined to list it under ENV_HELP with sufficient warnings about its usage but I can see @manics point about not publicizing it. Looking at the list of CLI plugins, others could be classified under this category as well (perf, testengine) if not even deprecated.

@joshmoore
Copy link
Copy Markdown
Member

(perf, testengine) if not even deprecated.

This is beyond this PR but as described in https://trello.com/c/tVSjHnee/55-add-warnings-for-windows-end-of-support there are several plugins (perf, testengine, submit) which should be deprecated and removed. In comparison, childre, duplicate, render, are "experimental" and the API will be decided on soon for inclusion. (Another way of doing this would be to have them in a separate repo and versioned low, e.g. 0.1.0, and then install them via pip in a virtualenv.)

@manics
Copy link
Copy Markdown
Member

manics commented Apr 19, 2016

On the OMERO_DEV_PLUGINS topic, I would be inclined to list it under ENV_HELP with sufficient warnings about its usage but I can see @manics point about not publicizing it.

The alternative is to not include these plugins in the mainline, so I think it's best to treat them as non-existent. Originally they were only in the metadata branch, but since they require changes to other files outside of plugins this would lead to conflicts as the two branches diverged even more. We could add just a subset of files but this makes things more complicated, and difficult to test.

@joshmoore
Copy link
Copy Markdown
Member

On the OMERO_DEV_PLUGINS topic, I would be inclined to list it under ENV_HELP ...

👍 Minimally that should help the team remember what the secret invocation was. (Can be external to this PR)

params.addId(did)
query = ("select p from Project as p where exists"
"(select l from ProjectDatasetLink as l where "
"l.child.id=:id and l.parent=p.id)")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might simply be something like,
SELECT DISTINCT parent FROM ProjectDatasetLink WHERE child.id = :id

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion @mtbc, @pwalczysko may have adapted his query from elsewhere in our codebase?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @ximenesuk suggested, I indeed did adapt this code from the import test https://github.com/openmicroscopy/openmicroscopy/blob/develop/components/tools/OmeroPy/test/integration/clitest/test_import.py#L189.
@mtbc : Would it be okay to merge this as it is and return to the cleanup of the queries in the tests later ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, not blocking, just commenting. Be warned that there are some quite exciting examples of HQL around our codebase. 😃

@jburel
Copy link
Copy Markdown
Member

jburel commented Apr 28, 2016

Reading the comments, especially #4574 (comment)
it looks like we need to scope "pip install plugins", we will need the same for web. This could be done under Facilities/Institutions management. Card created https://trello.com/c/KUGBzOdm/22-pip-install-plugins

Better to hold off for 5.2.3

@mtbc
Copy link
Copy Markdown
Member

mtbc commented May 15, 2016

What more needs doing here -- review tests?

@joshmoore
Copy link
Copy Markdown
Member

My understanding is that this was just waiting for 5.2.3 to be released.

@ximenesuk
Copy link
Copy Markdown
Contributor Author

ximenesuk commented May 16, 2016

Yes, that was my understanding too. If any further tests were to be needed @pwalczysko would open a PR directly against develop once this is merged,

@pwalczysko
Copy link
Copy Markdown
Member

So should this be relisted ? Would give me a boost if this could go in really :)

@joshmoore
Copy link
Copy Markdown
Member

Well in that case .... merging. I will organize a general discussion on the de-DEV-PLUGIN-ification in the summer as we try to get the IDR work back on the mainline. Thanks all!

@joshmoore joshmoore merged commit 991c41b into ome:develop May 16, 2016
@sbesson sbesson added this to the 5.3.0 milestone Sep 10, 2016
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.

7 participants