search

JuliaCon / proceedings-review

6 stars 1 forks source link

[REVIEW]: MPI.jl: Julia bindings for the Message Passing Interface #68

Closed whedon closed 2 years ago

whedon commented 3 years ago

Submitting author: !--author-handle-->@simonbyrne<!--end-author-handle-- (Simon Byrne) Repository: https://github.com/JuliaParallel/MPI.jl Branch with paper.md (empty if default branch): Version: Editor: !--editor-->@crstnbr<!--end-editor-- Reviewers: @dalcinl, @stevengj Archive:

Status

status

Status badge code:

HTML: <a href="https://proceedings.juliacon.org/papers/6c9e2d73ce0bcb9c3771a9b730fef28d"><img src="https://proceedings.juliacon.org/papers/6c9e2d73ce0bcb9c3771a9b730fef28d/status.svg"></a>
Markdown: [![status](https://proceedings.juliacon.org/papers/6c9e2d73ce0bcb9c3771a9b730fef28d/status.svg)](https://proceedings.juliacon.org/papers/6c9e2d73ce0bcb9c3771a9b730fef28d)

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) by leaving comments 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

@dalcinl & @stevengj, 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.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @crstnbr know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Review checklist for @dalcinl

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Paper format

Content

Code of Conduct

General checks

Functionality

Documentation

Paper format

Content

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @dalcinl, @stevengj it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/JuliaCon/proceedings-review) 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/JuliaCon/proceedings-review:

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

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
whedon commented 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.15 s (627.5 files/s, 56726.9 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Julia                           68           1380            578           5510
Markdown                        16            144              0            443
YAML                             8             73              7            366
SVG                              1              0              0             80
Bourne Shell                     1              5             20             39
TOML                             2              4              0             30
-------------------------------------------------------------------------------
SUM:                            96           1606            605           6468
-------------------------------------------------------------------------------

Statistical information for the repository 'fd5280ce7d51695c8975f415' was
gathered on 2021/02/07.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Bart Janssens                    5            39              2            6.39
Erik Schnetter                   5           101             90           29.75
Filippo Vicentini                1             4              0            0.62
Jake Bolewski                    1            41             41           12.77
Jared Crean                      2             8              0            1.25
Joel Mason                       1             2              0            0.31
Katie Hyatt                      1             8              0            1.25
Lucas C Wilcox                   1            54              0            8.41
Sam                              1             4              0            0.62
Simon Byrne                      2             0            138           21.50
Steven G. Johnson                2            51             49           15.58
Thomas Bolemann                  3             9              1            1.56

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
whedon commented 3 years ago

PDF failed to compile for issue #68 with the following error:

Can't find any papers to compile :-(

carstenbauer commented 3 years ago

@whedon generate pdf from branch paper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch paper. Reticulating splines etc...
whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

whedon commented 3 years ago

:wave: @dalcinl, please update us on how your review is going (this is an automated reminder).

whedon commented 3 years ago

:wave: @stevengj, please update us on how your review is going (this is an automated reminder).

dalcinl commented 3 years ago

A few minor comments/requests for the article:

simonbyrne commented 3 years ago 
* Some of the references do not have a DOI.

@crstnbr I've included the DOIs in the bib file: any idea why they don't appear?

stevengj commented 3 years ago

Some comments:

• why is mpi4py faster for large generic objects? Is it Python pickle vs Julia Serialization module?

• An important part of MPI is non-blocking communications, but asynchonous I/O is usually handled in julia with tasks or "green threads". Are there any prospects to combine these? (recently asked on discourse)

• MPI persistent communications are not supported? any plans?

• In section 3.2 what about non-Array containers supporting stride and pointer? Should there be discussion of the MPI.Buffer and MPI.UBuffer types and the design decisions here?

• dynamic process management in MPI-2 is not supported? any plans?

• one-sided communications seem to be supported in MPI.jl but not are mentioned in the paper?

• parallel I/O support? is it possible to use MPI.jl with parallel HDF5 and other libraries?

• The MPI.jl repo was started in 2012, and is one of the oldest Julia packages. What have been the major milestones and changes over the last 8 years? Any design mistakes or lessons learned?

typo: "arbirary" in sec. 3.1

dalcinl commented 3 years ago

@stevengj Out of curiosity, could you provide me with a link to that discussion related to combining non-blocking MPI and green threads?

simonbyrne commented 3 years ago

I believe he is referring to this one: https://discourse.julialang.org/t/how-to-combine-mpi-non-blocking-isend-irecv-and-julia-tasks/52524

simonbyrne commented 3 years ago

@whedon generate pdf from branch paper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch paper. Reticulating splines etc...
whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

simonbyrne commented 3 years ago

@whedon generate pdf from branch paper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch paper. Reticulating splines etc...
whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

simonbyrne commented 3 years ago

Thank you @dalcinl and @stevengj for the feedback. We have addressed the comments as follows:

  • Section 3.2, about MPI.jl automatically building and commiting datatypes. What about user-defined datatypes? Are those supported? Can those be used in communication calls?. Any plan to support that? I think this should be clarified in the text, even if just with a short sentence. MPI.jl's datatype automatism looks even better than mpi4py's, but support for a more low level, MPI-native way could also be of use for experienced users and library developers.

Yes, this available, we have added a sentence to this effect. (note it is now section 3.3)

  • Page 2, section 3.4, last sentence in right column before Sec. 4 starts: how to deal with binaries which depend on MPI. I'm not sure what this really means. Are you talking about interoperating with shared libraries containing MPI-based parallel algorithms? I would suggest a slight reword to make meaning of this sentence clearer.

Yes, that is exactly the problem. We have expanded the text here, and given examples (HDF5 and PETSc).

  • Page 3, left column, second paragraph, there is a missing blank space between MPI.send and the word "and".

Fixed.

  • Page 3, left column, second paragraph. You blame Julia's GC for the slowdown with medium message sizes. Did you try to bracket the communication+timing with gc_disable()/gc_enable() to confirm, if that is ever possible? BTW, could you clarify what Python and NumPy versions were used in these tests? FYI, mpi4py branch master, to be released soon, now uses the highest available pickle protocol to communicate generic objects. Under Python 3.8, that would be pickle protocol version 5, which introduces important optimizations for buffer-like objects like NumPy arrays.

We did try disabling the GC, and it didn't have much effect. Instead we benchmarked the serialization protocols directly, which do appear to explain the main differences. We have clarified this and added the version numbers as well.

  • Some of the references do not have a DOI.

This appears to be a problem with the style sheet.

  • Perhaps you should add a disclaimer to the paper (similar to the one you have in your docs), that the project does not support the full feature set of the MPI standard, together with a promise that they this will be addressed in the future?

We have tried to address this in the new section 3.1.

• why is mpi4py faster for large generic objects? Is it Python pickle vs Julia Serialization module?

This does appear to be the case. We have added comments to this effect in section 4.1.

• An important part of MPI is non-blocking communications, but asynchonous I/O is usually handled in julia with tasks or "green threads". Are there any prospects to combine these? (recently asked on discourse)

We have added section 3.6 to address this.

• MPI persistent communications are not supported? any plans?

They are not currently supported, we have mentioned it in section 3.1.

• In section 3.2 what about non-Array containers supporting stride and pointer? Should there be discussion of the MPI.Buffer and MPI.UBuffer types and the design decisions here?

UBuffer was added since this paper was originally written. We have added it to (now) section 3.3.

• dynamic process management in MPI-2 is not supported? any plans?

There is some support (mentioned in new section 3.1), but unfortunately their utility is hampered by the requirement to use within the MPI launcher (discussed at end of the new section 3.6).

• one-sided communications seem to be supported in MPI.jl but not are mentioned in the paper?

It is mentioned in the new section 3.1.

• parallel I/O support? is it possible to use MPI.jl with parallel HDF5 and other libraries?

I/O support is mentioned in new section 3.1, added discussion of integration with HDF5 in section 3.5.

• The MPI.jl repo was started in 2012, and is one of the oldest Julia packages. What have been the major milestones and changes over the last 8 years? Any design mistakes or lessons learned?

We added a brief history in new section 1.1

In addition to the above changes, we added some context to the pooled variance example (section 4.3).

simonbyrne commented 3 years ago

@whedon generate pdf from branch paper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch paper. Reticulating splines etc...
whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

simonbyrne commented 3 years ago

I also added the doi links to the bibliography

dalcinl commented 3 years ago

@simonbyrne Great job, your clarifications improved the text considerably. A few minor issues remains:

@crstnbr After Simon fixes these minor typos above, I'm done with this review, I consider the paper ready to go. Is this confirmation enough, or should I somehow tell whedon about it?

carstenbauer commented 3 years ago

@dalcinl That should be confirmation enough. Thank you for your review!

@stevengj, can you take a look at the updated article proof?

simonbyrne commented 3 years ago

@whedon generate pdf from branch paper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch paper. Reticulating splines etc...
simonbyrne commented 3 years ago

Thanks @dalcinl, I have made the changes, and fixed a few other typos I found.

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

carstenbauer commented 3 years ago

@stevengj, to get this over the finish line it'd be great if you could take a (final) look at the updated article proof and finish your review / give your recommendation. Thanks in advance!

carstenbauer commented 3 years ago

@whedon remind @stevengj in 2 weeks

whedon commented 3 years ago

Reminder set for @stevengj in 2 weeks

whedon commented 3 years ago

:wave: @stevengj, please update us on how your review is going (this is an automated reminder).

carstenbauer commented 2 years ago

@stevengj, can I ask you once more to take a final look at this and give your recommendation? Would be great to finish this before JuliaCon 2021.

stevengj commented 2 years ago

LGTM.

carstenbauer commented 2 years ago

@whedon commands

whedon commented 2 years ago

Here are some things you can ask me to do:

# List all of Whedon's capabilities
@whedon commands

# Assign a GitHub user as the sole reviewer of this submission
@whedon assign @username as reviewer

# Add a GitHub user to the reviewers of this submission
@whedon add @username as reviewer

# Re-invite a reviewer (if they can't update checklists)
@whedon re-invite @username as reviewer

# Remove a GitHub user from the reviewers of this submission
@whedon remove @username as reviewer

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

# Change editorial assignment
@whedon assign @username as editor

# Set the software archive DOI at the top of the issue e.g.
@whedon set 10.0000/zenodo.00000 as archive

# Set the software version at the top of the issue e.g.
@whedon set v1.0.1 as version

# Open the review issue
@whedon start review

EDITORIAL TASKS

# All commands can be run on a non-default branch, to do this pass a custom 
# branch name by following the command with `from branch custom-branch-name`.
# For example:

# Compile the paper
@whedon generate pdf

# Compile the paper from alternative branch
@whedon generate pdf from branch custom-branch-name

# Remind an author or reviewer to return to a review after a
# certain period of time (supported units days and weeks)
@whedon remind @reviewer in 2 weeks

# Ask Whedon to do a dry run of accepting the paper and depositing with Crossref
@whedon recommend-accept

# Ask Whedon to check the references for missing DOIs
@whedon check references

# Ask Whedon to check repository statistics for the submitted software
@whedon check repository

EiC TASKS

# Invite an editor to edit a submission (sending them an email)
@whedon invite @editor as editor

# Reject a paper
@whedon reject

# Withdraw a paper
@whedon withdraw

# Ask Whedon to actually accept the paper and deposit with Crossref
@whedon accept deposit=true
carstenbauer commented 2 years ago

@whedon check references

carstenbauer commented 2 years ago

@simonbyrne, can you please archive the current state of MPI.jl using Zenodo and post the associated DOI here (see https://joss.readthedocs.io/en/latest/editing.html#after-reviewers-recommend-acceptance).

simonbyrne commented 2 years ago

@whedon set 10.5281/zenodo.5063032 as archive

whedon commented 2 years ago

I'm sorry @simonbyrne, I'm afraid I can't do that. That's something only editors are allowed to do.

simonbyrne commented 2 years ago

@crstnbr zenodo doi is here: https://doi.org/10.5281/zenodo.5063032

carstenbauer commented 2 years ago

@whedon set 10.5281/zenodo.5063032 as archive

whedon commented 2 years ago

OK. 10.5281/zenodo.5063032 is the archive.

carstenbauer commented 2 years ago

@whedon accept

whedon commented 2 years ago

To recommend a paper to be accepted use @whedon recommend-accept

carstenbauer commented 2 years ago

@whedon recommend-accept

whedon commented 2 years ago 
Attempting dry run of processing paper acceptance...
whedon commented 2 years ago

PDF failed to compile for issue #68 with the following error: 

 Can't find any papers to compile :-(