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

populate metadata links file ann #152

Merged
merged 2 commits into from
Dec 19, 2019

Conversation

will-moore
Copy link
Member

@will-moore will-moore commented Aug 16, 2019

This change is needed to allow populate_metadata script to work with the File-picker from ome/openmicroscopy#6096
That PR passes the uploaded FileAnnotation ID to populate-metadata script, but the script expects the FileAnnotation to be linked to the selected object.
This PR links the FileAnnotation to the object, which also has the benefit of not 'losing' the new FileAnnotation with it being unlinked.

To test:

  • Create or find a CSV FileAnnotation for populate metadata. E.g. https://merge-ci.openmicroscopy.org/web/webclient/?show=dataset-4919 (user-3)
  • Note the FileAnnotation ID - (9987) then unlink (don't delete) the Annotation from the Dataset.
  • Run populate_metadata script on the Dataset, entering the File Annotation ID.
  • Check that the populate-metadata works as expected to create an OMERO.table and that the csv becomes linked to the Dataset.
  • If the File_Annotation ID is filled out in the script (and the FileAnnotation is already linked to the object) then script should continue to work as before

@will-moore will-moore changed the title link file_ann to object if not linked populate metadata links file ann Aug 16, 2019
@jburel
Copy link
Member

jburel commented Aug 19, 2019

@will-moore
Copy link
Member Author

will-moore commented Aug 19, 2019

It looks like merge-ci doesn't have the latest version of the script, although it was merged in the last build at snoopycrimecop@b7de9d7

@sbesson
Copy link
Member

sbesson commented Aug 19, 2019

I was surprised by this at first it looks consistent to the introduction of the --shallow flag in ome/devspace@9a99bf1.

@will-moore
Copy link
Member Author

Does that commit need reverting, or some other fix?

@sbesson
Copy link
Member

sbesson commented Aug 19, 2019

Reverting the commit will only apply to newly deployed devspace environment. In the case of the existing merge-ci devspace, I think you want to modify the default value of MERGE_COMMAND in the job configuration and restore the previous behavior unless @jburel remembers a reason not to revert his change.

@jburel
Copy link
Member

jburel commented Aug 19, 2019

I do not remember why we added the shallow flag

@joshmoore
Copy link
Member

For what it's worth, we will very soon no longer have a submodule in ome/openmicroscopy.

@joshmoore
Copy link
Member

Coming back to the content of the PR:

This change is needed to allow populate_metadata script to work with the File-picker from ome/openmicroscopy#6096

We might need to discuss how this type of coupling should affect the semver of these two components.

@will-moore
Copy link
Member Author

Certainly we shouldn't release the change in the script dialog ome/openmicroscopy#6096 before the script changes in this PR.

There are a few places where we have a 'convention' on the naming of script Parameters. E.g. "Data_Type" and "IDs" for data input. These are quite simple convention so we haven't felt the need to define this API anywhere. The "File_Annotation" parameter is also understood by the webclient and can be auto-populated. I assumed File_Annotation would simply use the ID of a File_Annotation, but it has an additional requirement that the Annotation is linked to the Project/Screen for populate_metadata script.

Perhaps we can test and merge this PR first, then consider ome/openmicroscopy#6096

Will that be enough to ensure that user don't get the webclient dialog changes before they get the script changes?

cc @chris-allan

@pwalczysko
Copy link
Member

We still do not have this script on the merge-ci - judging according to me getting the same result of the test as @jburel above #152 - how to improve that ?

@pwalczysko
Copy link
Member

Uploaded the script here manually onto merge-ci. The functionality works as described.

@will-moore
Copy link
Member Author

Closing until #159 is merged to avoid conflicts...

@will-moore will-moore closed this Nov 19, 2019
@will-moore will-moore reopened this Dec 10, 2019
@pwalczysko
Copy link
Member

All works fine here. Tried the suggested table as well as my own, with double columns -> parade

The unattached File is found via the ID as expected, and when one is attached there already, the script still runs as expected.

Ready to merge FMPOV

@pwalczysko
Copy link
Member

Maybe we should test on py3-ci though ? All the tests above were run on merge-ci

@will-moore
Copy link
Member Author

will-moore commented Dec 16, 2019

Uploaded and tested on py3-ci today. Will list for testing there tomorrow (will need to upload script again first). Use https://py3-ci.openmicroscopy.org/web/webclient/?show=dataset-3120 (user-3).

@pwalczysko
Copy link
Member

Tested on py3-ci, user-3 https://py3-ci.openmicroscopy.org/web/webclient/?show=dataset-2270

All good. Ready to merge.

@joshmoore
Copy link
Member

This change is needed to allow populate_metadata script to work with the File-picker from ome/openmicroscopy#6096

But that PR is excluded. Can you explain?

@will-moore
Copy link
Member Author

ome/openmicroscopy#6096 requires this PR, so it is probably excluded until this one is merged, released etc. However, this PR doesn't require that one.

@joshmoore joshmoore merged commit c3ed096 into ome:develop Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants