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

Provide an import summary (see #12506). #2950

Merged
merged 4 commits into from Aug 25, 2014
Merged

Conversation

bpindelski
Copy link

This PR is an initial attempt at fixing http://trac.openmicroscopy.org.uk/ome/ticket/12506. It adds a summary line after the CLI import finishes. The output should be self-descriptive.

To test:

--no-rebase

@sbesson
Copy link
Member

sbesson commented Aug 15, 2014

@bpindelski: on the Python side I think we could simply add a test to https://github.com/openmicroscopy/openmicroscopy/blob/develop/components/tools/OmeroPy/test/integration/clitest/test_import.py checking the stderr.

@bpindelski
Copy link
Author

I have a hunch, that a single line summary similar to that from Homebrew would be more useful (i.e.

==> Summary
/usr/local/Cellar/findbugs/3.0.0: 156 files, 11M, built in 5 seconds

@ximenesuk
Copy link
Contributor

It works in so much as:

SUMMARY
-------
- Files in source location: 6
- Files selected and uploaded: 4
- Files imported: 3

This set contained one failed import (corrupt dv log) - it failed before upload. Should the number of failures/errors be somehow noted so that someone can go back through captured logs to fin the error?

Secondly, what it meant here by Files imported: 3 I assume this is the good dv and the two good tiffs. Should the dv log file that was uploaded be considered "imported"? Maybe the image count would be better here, Images imported: 3 or something like that?

In the case of a folder of lei files,

SUMMARY
-------
- Files in source location: 548
- Files selected and uploaded: 535
- Files imported: 141

the disparity of files uploaded to images here is even greater and the final number is the correct image count.

@ximenesuk
Copy link
Contributor

I'd agree that a one line summary might be better. I'd say the number of files in the original location is not that useful (I can always ls -a afterwards. I'd add the number of imports (or filesets) and as above the number of failed imports. Something like:

==> Summary
535 files uploaded, 3 filesets created, 141 images imported, 0 errors in 5 minutes  

@bpindelski
Copy link
Author

@ximenesuk Thanks for your suggestions. I was exploring the options with the first implementation and now have almost a design spec 👍 I also agree, that conveying the concept of companion files or MIFs isn't easy, especially in one summary line. More commits coming!

@bpindelski
Copy link
Author

@sbesson I've added an initial Python test. Happy to extend it into cases where the values extracted from the message are compared to some expectations based on number of files imported etc.

@ximenesuk I've also changed the message format. I'd be great if you could use your corrupted files to see if exceptions are being counted correctly. Thanks.

@ximenesuk
Copy link
Contributor

@bpindelski Sorry, I missed your IM. data_repo/test_images_bad contains a badly formed dv file with log. I used that pair of files to force an import error.

@bpindelski
Copy link
Author

@ximenesuk This should be the final set of changes. The reported time is in the ISO8601-like format (H:m:s.S). This seems universal enough and doesn't force me to stitch together "hour", "minute" and "second" strings. Also the singular/plural form of the nouns should be correct.

@ximenesuk
Copy link
Contributor

==> Summary
4 files uploaded, 3 filesets created, 3 images imported, 1 error in 0:00:19.685jrs-macbookpro-25031:ome cblackburn$ 
...
535 files uploaded, 7 filesets created, 141 images imported, 0 errors in 0:03:29.275jrs-macbookpro-25031:ome cblackburn$

I think a line feed might be helpful :-)

Otherwise looking good.

@ximenesuk
Copy link
Contributor

Is a summary after a Plate import planned? For instance, this is a folder containing four Images and a Plate:

==> Summary
60 files uploaded, 5 filesets created, 16 images imported, 0 errors in 0:00:40.631jrs-macbookpro-25031:ome cblackburn$ 

12 of the reported images are part of a single Plate. Maybe adding X plates... in there might make sense if this is achievable?

@jburel
Copy link
Member

jburel commented Aug 19, 2014

Certainly a feature we could add to the insight import cc @gusferguson

@bpindelski
Copy link
Author

@ximenesuk I'll have to investigate the lack of newline. I didn't see it locally. Are you using Terminal.app or iTerm2?
I haven't tested the summary with a SPW import. I'd have to check how easy/hard it will be, as I presume it will follow a different code path. I also need to check what happens with the summary with advanced import (e.g. ln_s).

@ximenesuk
Copy link
Contributor

@bpindelski I'm using the terminal app.

If I import a Plate on its own I get no summary at all - expected if you've not looked at that yet. If I import a mix I get the above, ie files and Filesets are correct but Images are unexpected (rather than wrong). In this case the Plate was imported first, I'm not sure what would have happened if the Plate had been last - might that have suppressed the summary?

@sbesson sbesson added ON_HOLD and removed ON_HOLD labels Aug 19, 2014
@ximenesuk
Copy link
Contributor

Looks good:

==> Summary
60 files uploaded, 5 filesets, 1 plate created, 16 images imported, 0 errors in 0:00:28.625

@bpindelski
Copy link
Author

@jburel Does this need more testing? Or someone else to have a look?

@jburel
Copy link
Member

jburel commented Aug 25, 2014

Opened a card on UX board https://trello.com/c/8n944Jxx/278-import-summary
Merging

jburel added a commit that referenced this pull request Aug 25, 2014
Provide an import summary (see #12506).
@jburel jburel merged commit ccbba03 into ome:develop Aug 25, 2014
@bpindelski bpindelski deleted the import_details branch August 25, 2014 13:22
@joshmoore
Copy link
Member

Coming very late to this: since the report is so nice & compact, is there any reason to have it as optional?

@joshmoore
Copy link
Member

Moved this to a ticket: https://trac.openmicroscopy.org.uk/ome/ticket/12549

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.

None yet

5 participants