-
Notifications
You must be signed in to change notification settings - Fork 33
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
Support omero download Fileset:ID #298
Conversation
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.
Thanks for kicking this off, @will-moore. One concern I had thinking about this yesterday is if this won't end up becoming a full omero-downloader
. One way to balance that that I was considering was not to introduce Project & Dataset explicitly, but to let "filename" become the target download. Users would produce a list or mapping of project/dataset paths to image ids:
# Legacy: download the single file for an image (this makes things tricky)
omero download Image:1 output.tif
# Download single or multiple file to directory if it exists
omero download Image:1 output/
# Usage for Project/Dataset hierarchies, creating directories
omero download Image:1 --parent my-project/my-dataset/
One thing we would probably want to re-open would be Colin's work on printing these hierarchies: ome/openmicroscopy#4777
Edit: One other issue that these strategies will have is that they may download the same original files multiple times. I suggest we ignore that.
Doing: Is the main objection to
If the code as it is will work for the user who needs it, but we don't want to add it and maintain it here, then it could be a separate user script (gist) instead? |
42c3980
to
4e9e625
Compare
As discussed this morning, added support for:
This is now used by:
PR description updated |
Initially testing looks good on the One other nice to have might be to clean up the file on cancellation and/or support restart, but that's getting a bit ahead of ourselves for now. Edit: https://github.com/willmcgugan/rich was the other library for nicer CLIs (see #217) |
This is failing a bunch of tests for various reasons:
|
Could the |
Tested images:
Download only ndpi Download and reimport vsi: All went fine |
Bug:
|
@pwalczysko That "bug" should be fixed by eeed465 to fail as it did before, which is a bit more graceful. |
thanks, I will rebuild and retry |
Indeed, now the failure is much nicer
|
In summary, tested all 5 commands in the header of this PR on different file formats. All went okay. |
All except 1 test fixed above (and changes to the tests at ome/openmicroscopy#6288) EDIT: test fixed at will-moore/openmicroscopy@4d50eff |
Tests passing (I added one to test download of |
Thanks @will-moore, overall I am very much in favor of releasing features added in this PR as they lift a long-standing limitation of the plugin when downloading original files associated with images. Allowing complete fileset download has the classical pitfalls associated large download that we have seen previously. I assume this feature respects the policy restrictions set up server-side? As suggested by the description of this PR and is more clearly exposed in the accompanying integration tests, this PR effectively changes the semantics for a single file image download. Previously I have been pondering about the breaking nature or not of this API change and I am increasingly inclined to qualify the previous behavior as incorrect and prone to data corruption. This is related to the fact that the command gives full control over the target filename which is for instance at odds with the fact that the original file extension can be a requirement for Bio-Formats and other translational libraries. Taking the FITS format as a representative example , with the current implementation, the following round-tripping will fail with the current version of
Happy to see this released as a minor version increment under these terms. |
It has no choice 😉 |
@sbesson There are tests for the download policy which are testing the Image download - see #298 (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.
Merging and releasing given no objection has been raised to the release proposal above.
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/download-full-projects-with-their-datasets/56434/16 |
See https://forum.image.sc/t/download-full-projects-with-their-datasets/56434/
This adds support for:
Files are downloaded to the named directory. If original files have a path that is longer than the fileset template prefix,
that path will be used to create additional directories inside the named directory.
The same behaviour is used for Images (this fixes the previous limitation of only a single-file for an Image):
Other functionality to test for regressions: