-
Notifications
You must be signed in to change notification settings - Fork 59
update examples, add libfabric #1449
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.
OK this looks very nice, a real advance for Charliecloud. Lots of suggestions in-line. General comments:
- My brain hurts following the logic. Do we need to have it reviewed by someone who knows this MPI stuff?
- Let's make an issue to add libfabric injection (or whatever a good description of the new MPI functionality is).
- Please write up a (short!) paragraph to share internally and with the team summarizing this improvement and why it's cool.
- The documentation is very well written.
bin/ch-fromhost
Outdated
-d, --dest DST place following files in IMGDIR/DST, overriding inference | ||
-d, --dest DST place preceding files in IMGDIR/DST, overriding inference |
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.
Is this really resolved? Doesn't in fact affect things that follow it on the command line?
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.
Looks great! One typo I wouldn't bother with normally, but also the internal tests fail. I'll send you details.
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.
Looks good, thank you!
Addresses: #1443, #1549, and possibly others.
Makes #256 moot.