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

[REVIEW]: MaDaTS: Managing Data on Tiered Storage for Scientific Workflows #830

Closed
36 tasks done
whedon opened this issue Jul 23, 2018 · 72 comments
Closed
36 tasks done
Assignees
Labels
accepted published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. review

Comments

@whedon
Copy link

whedon commented Jul 23, 2018

Submitting author: @dghoshal-lbl (Devarshi Ghoshal)
Repository: https://github.com/dghoshal-lbl/madats
Version: v1.1.2
Editor: @danielskatz
Reviewer: @ian-taylor, @gflofst
Archive: 10.5281/zenodo.1439288

Status

status

Status badge code:

HTML: <a href="http://joss.theoj.org/papers/0ec885baba24844be3c89eb532ecc9b0"><img src="http://joss.theoj.org/papers/0ec885baba24844be3c89eb532ecc9b0/status.svg"></a>
Markdown: [![status](http://joss.theoj.org/papers/0ec885baba24844be3c89eb532ecc9b0/status.svg)](http://joss.theoj.org/papers/0ec885baba24844be3c89eb532ecc9b0)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@ian-taylor & @gflofst, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @danielskatz know.

Please try and complete your review in the next two weeks

Review checklist for @ian-taylor

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: Does the release version given match the GitHub release (v1.1.2)?
  • Authorship: Has the submitting author (dghoshal-lbl) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

Review checklist for @gflofst

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: Does the release version given match the GitHub release (v1.1.2)?
  • Authorship: Has the submitting author (dghoshal-lbl) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?
@whedon
Copy link
Author

whedon commented Jul 23, 2018

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks. @ian-taylor, it looks like you're currently assigned as the reviewer for this paper 🎉.

⭐ Important ⭐

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands

@whedon
Copy link
Author

whedon commented Jul 23, 2018

Attempting PDF compilation. Reticulating splines etc...

@whedon
Copy link
Author

whedon commented Jul 23, 2018

@danielskatz
Copy link

👋 @ian-taylor & @gflofst - please go ahead and start your reviews

See the first comment in the issue for details on how to do the review, and https://joss.readthedocs.io/en/latest/reviewer_guidelines.html and https://joss.readthedocs.io/en/latest/review_criteria.html for more details. In brief, what I'm looking for is you both to check off the 18 items on your checklists.

If you have problems, feel free to comment here or email me.

Finally, a note that @ian-taylor will not be able to do his review until the 2nd week of August.

@danielskatz
Copy link

👋 @ian-taylor & @gflofst - any update on your reviews? I don't see any checkmarks yet...

@gflofst
Copy link

gflofst commented Jul 30, 2018 via email

@gflofst
Copy link

gflofst commented Jul 30, 2018

The license is not a standard license, but a University of California/DOE license. It is very straightforward reminiscent of the MIT license.

@danielskatz
Copy link

Thanks @gflofst

👋 @dghoshal-lbl - Note that JOSS requires using an OSI-approved license - which is this?

@gflofst
Copy link

gflofst commented Jul 30, 2018

It has no text citing it as a particular license. Instead, it lists that it is approved by Uni. Cal. and LBL (and DOE). The license text is basically MIT (do what you want, cite this code, we are not responsible for what you do or what happens because of use).

@dghoshal-lbl
Copy link

@danielskatz @gflofst It is a modified BSD license.

@gflofst
Copy link

gflofst commented Jul 30, 2018

In the setup.py, it says BSD. How exacting should that match be to be acceptable?

@danielskatz
Copy link

Modified in any way is not acceptable.

To be published by JOSS, the code needs to have an OSI-approved license.

@dghoshal-lbl
Copy link

There is an additional paragraph at the end to the original BSD license (due to lab policies).

@dghoshal-lbl
Copy link

Is it mandatory to have the exact same license? Previously one of the papers was accepted because the license is still BSD, with an additional paragraph.

@danielskatz
Copy link

Yes, it is mandatory. Can I check the process for the other paper? Which was it?

@gflofst
Copy link

gflofst commented Jul 30, 2018

Community guidelines are either missing or not obvious

@gflofst
Copy link

gflofst commented Jul 30, 2018

Build instructions lack information about creating the required storage.yaml file needed to run setup.py

@dghoshal-lbl
Copy link

@danielskatz
i) This is the paper: https://joss.theoj.org/papers/f68f92a740c6e42365025be9a9efed1b
ii) Software package: https://zenodo.org/record/290299#.W1-isdhKjOQ (Please note license on the right side, which says -- BSD 3-Clause "New" or "Revised" License (BSD-3-Clause))
iii) And the review comments: https://bitbucket.org/dghoshal/frieda/issues/1/joss-review-comments

@dghoshal-lbl
Copy link

@gflofst A sample storage.yaml file is created when setup.py is run. Did the build process fail?

@danielskatz
Copy link

👋 @arfon - please weigh in on the license discussion for this submission, as well as for https://doi.org/10.21105/joss.00164

@arfon
Copy link
Member

arfon commented Jul 31, 2018

Is it mandatory to have the exact same license? Previously one of the papers was accepted because the license is still BSD, with an additional paragraph.

I think this statement is contradictory, i.e. we don't know what it is now and whether it's 'still BSD' in legal terms. The whole point of having a standard set of approved and vetted open source licenses is that they are used verbatim without modification.

This submission should be considered blocked by this issue and I think the JOSS editorial board needs to discuss the status of https://doi.org/10.21105/joss.00164 as the non-standard license is problematic and something we should have caught (and likely would going forward with this openjournals/whedon-api#35).

@gflofst
Copy link

gflofst commented Jul 31, 2018

@dghoshal-lbl Here is the output when I try to run setup. It looks like I should have created a directory, but this isn't specified in the instructions. Please review and adjust the instructions accordingly.
setup.txt

@dghoshal-lbl
Copy link

@gflofst There is no need to create any additional directory. The config/ directory is already present in the top-level directory of the source tree.
Unfortunately I am unable to reproduce the error on my side. I need some more information.

  • Is this where you have checked out the source (including setup.py): /Users/gflofst/build/joss/run?
  • Have you manually configured MADATS_HOME environment variable?

@gflofst
Copy link

gflofst commented Jul 31, 2018

I did the clone into joss so the top level directory is
/Users/gflofst/build/joss/madats

I do not have MADATS_HOME set in this terminal window. While the instructions after the two steps say to set it up, it should be a step in the instructions in order too. When I do set it to the above directory, it works. Having to set it to generate the file that will set it doesn't make sense.

@dghoshal-lbl
Copy link

You do not need to set MADATS_HOME manually. I will fix the instructions in the readme if that is not clear enough. However, if you run setup from the top level directory "/Users/gflofst/build/joss/madats" (where setup.py is), it should create the required files.
I was wondering why the setup log shows "/Users/gflofst/build/joss/run" as MADATS_HOME. What is this joss/run/ directory, if code is in joss/madats/?

@gflofst
Copy link

gflofst commented Jul 31, 2018

code is in joss/madats and that is where I run setup.py from. The only way I can get it to work properly is if I set MADATS_HOME manually before running setup.py.

@dghoshal-lbl
Copy link

That shouldn't happen. Can you please open up a new terminal and let me know if "echo $MADATS_HOME" prints anything (without you manually setting it up)? If it doesn't print anything, can you please rerun "python setup.py install" and send me the output of the build process?

I tried in a couple of different environments and unable to reproduce the error. Can you please check if there is a "storage.yaml" file present in config/ directory at the end of the build process?

@dghoshal-lbl
Copy link

@ian-taylor The example section in Readme lists the steps to create a workflow of your own. But I think you are asking for a script or a sample workflow that you can tweak to build your own workflow. Is that correct?

@ian-taylor
Copy link

@dghoshal-lbl yes, that's correct. I just think that if there were a few example YAML files and scripts that could be configured for a particular scheduler etc, then this would make it a lot easier for a programmer to get going and use the system, without having to invest time in learning how to construct workflows for themselves. The review instructions require this also. Do you think you could add a couple of such examples?

@dghoshal-lbl
Copy link

@ian-taylor I think I have a few examples in the source tree. I will check those and see if I can add a few lines in the readme/user-guide on how to use those and build your own workflows using the examples as a reference. Hopefully that will help.

@danielskatz
Copy link

@ian-taylor - how are things looking to you?

@ian-taylor
Copy link

I was awaiting confirmation from @dghoshal-lbl that the examples were added.

@dghoshal-lbl
Copy link

@danielskatz @ian-taylor There are a few examples in the examples/ directory in the source tree. I am adding more information about these examples in the user guide so that users can use these as reference.

@ian-taylor
Copy link

Can you let me know when the documentation is complete so I can try and verify ?

@dghoshal-lbl
Copy link

@ian-taylor Sure, I will let you know once I have updated the documentation.

@dghoshal-lbl
Copy link

@ian-taylor I have added Chapter-3 in UserGuide.pdf to explain some examples of using MaDaTS. Let me know if it addresses what you asked for.

@danielskatz
Copy link

@ian-taylor - this is waiting for you as far as I can tell

@dghoshal-lbl
Copy link

@ian-taylor Any update on the review?

@ian-taylor
Copy link

@dghoshal-lbl I thought checked all of these boxes a few days ago but two got unchecked. Anyway, I rechecked now so all should be done.

@ian-taylor
Copy link

over to you @danielskatz

@danielskatz
Copy link

danielskatz commented Sep 28, 2018

👋 @gflofst - what about the community guidelines checklist item? Can you check this off, or let us know what else you think is needed?

@gflofst
Copy link

gflofst commented Sep 28, 2018

@danielskatz Those are really basic, but hit the checkbox for compliance. I hit the checkbox.

@danielskatz
Copy link

@dghoshal-lbl - please archive the current version somewhere (e.g. Zenodo) and let me know the DOI of the archive.

@dghoshal-lbl
Copy link

@danielskatz - here's the DOI: 10.5281/zenodo.1439288

@danielskatz
Copy link

@whedon set 10.5281/zenodo.1439288 as archive

@whedon
Copy link
Author

whedon commented Sep 30, 2018

OK. 10.5281/zenodo.1439288 is the archive.

@danielskatz
Copy link

👋 @arfon - this one's ready to accept

@arfon arfon added the accepted label Oct 1, 2018
@arfon
Copy link
Member

arfon commented Oct 1, 2018

@ian-taylor, @gflofst - many thanks for your reviews here and to @danielskatz for editing this submission ✨

@dghoshal-lbl - your paper is now accepted into JOSS and your DOI is https://doi.org/10.21105/joss.00830 ⚡ 🚀 💥

@arfon arfon closed this as completed Oct 1, 2018
@whedon
Copy link
Author

whedon commented Oct 1, 2018

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](http://joss.theoj.org/papers/10.21105/joss.00830/status.svg)](https://doi.org/10.21105/joss.00830)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.00830">
  <img src="http://joss.theoj.org/papers/10.21105/joss.00830/status.svg" alt="DOI badge" >
</a>

reStructuredText:
.. image:: http://joss.theoj.org/papers/10.21105/joss.00830/status.svg
   :target: https://doi.org/10.21105/joss.00830

This is how it will look in your documentation:

DOI

We need your help!

Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:

@dghoshal-lbl
Copy link

@arfon @danielskatz @gflofst @ian-taylor Thank you all for reviewing and getting the paper published!

@whedon whedon added published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. labels Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. review
Projects
None yet
Development

No branches or pull requests

6 participants