MPI_Info update (2nd / corrected PR)

[dsolt]

This is my first pull request, please give careful review. Not sure if this should be on a branch first? This change is intended for 2.1 and is a new model of handling MPI_Info changes to communicators/windows/files. It moves much of the info handling to the opal layer, but retains a wrapper for info objects that are exposed to the user. This allows us to use the info keyvalue concept internally and also implement MPI_Info using this internal concept.

The main driver for this change was to allow different components to see changes to MPI_Info (via MPI_Comm_set_info, etc) and have an opportunity to respond to those changes by either accepting the change and modifying their future behavior or modifying the proposed info change if they do not support it.

There are some scenarios which are difficult for this model. Particularly if multiple components (called subscribers in the code) use the same key but conflict on the values they support. Currently the key is presented to each component in turn and each has a chance to modify the value. The ordering therefore matters. The key (no pun intended) is to not do this. Each module should either use its own keys or they should agree on what values they can accept.

This code has received very minimal testing. I verified that no_locks values could be updated and the new value rejected when the underlying component does not support it. I can work on further testing, but would like some eyes on this as I do that to make sure the overall direction makes sense and that my approach to moving code around makes sense for the Open MPI architecture.

I would especially like @hjelmn to make sure that this will work for him.

@jsquyres @hjelmn @nysal @jjhursey @gpaulsen

[lanl-ompi]

Test FAILed.

[jsquyres]

@dsolt will fix the "make check" problems and update this PR.

[ggouaillardet]

it seems you used a tab instead of whitespaces

[dsolt]

Ok, I'm surprised there are not lots of these (maybe there are). I wasn't super careful because I thought that the commit hook script checked for this. I don't think my commit hook script is setup correctly though because the copyrights did not get updated either (as you noted in another comment). I'm looking into that.

[ggouaillardet]

@dsolt generally speaking, you have the option to update the copyright at the beginning of each file you update (e.g. 2016 IBM)
I do not know about IBM, and I know my employer enjoys being credited

[jsquyres]

@dsolt Have you been able to run all these through the IBM info tests, and any other info tests that you have? Have you been able to write a test or three showing that the new infrastructure inside OMPI works, beyond just using INFO_SET/GET?

[dsolt]

Not yet, but soon... hopefully this week.

From: Jeff Squyres [email protected]
To: open-mpi/ompi [email protected]
Cc: David Solt/Dallas/IBM@IBMUS
Date: 01/29/2016 03:05 PM
Subject: Re: [ompi] MPI_Info update (2nd / corrected PR) (#1308)

@dsolt Have you been able to run all these through the IBM info tests, and
any other info tests that you have? Have you been able to write a test or
three showing that the new infrastructure inside OMPI works, beyond just
using INFO_SET/GET?
—
Reply to this email directly or view it on GitHub.

[gpaulsen]

Mark Allen has agreed to test this new functionality (next week), and possibly write a few new tests. If he does write new tests, should he push them to MTT?

[jsquyres]

Yes, please put them in the ibm/info directory in the ompi-tests repo. We'll need to add a little configury to make sure they don't fire for the v1.10 series, but that's easy enough.

[jsquyres]

@dsolt @gpaulsen Any movement on this PR?

[dsolt]

Ok, so some code walk through and testing discussion led to the realization that the previous MPI_Comm_dup and MPI_Comm_dup_with_info both ignore the whole info concept entirely. So, there is some work to do yet to fix those (and MPI_Win_dup, MPI_File_dup). So, questions on what the MPI Standard intends for MPI_Comm_dup to do:

MPI_Info_set(myinfo, "use_shared_memory", "yes");
MPI_Comm_set_info(comm, myinfo);
MPI_Comm_get_info(comm, myinfo_returned); 
/// Assume here that there was not enough shared memory available to use shared-memory for 
// communication on this comm, so MPI_Info_get() tells us that use_shared_memory is "no".
MPI_Comm_free(&anothercomm); 
/// now there is more shared-memory available
MPI_Comm_dup(comm, &newcom);
MPI_comm_get_info(newcom, myinfo_returned);
//  WHAT SHOULD THIS DO?   WHAT SHOULD BE "PRESENTED" TO NEWCOM

Should the info on the newcomm be "no" since it was "no" on its parent or should it be "yes" since MPI_Comm_set_info() was used to set "yes" for shared-memory on its parent?

Consider the opposite, the original comm was asked to use shared-memory and it did, but new does not have enough shared-memory available to use shared-memory, should it return "yes" or "no"? It seems crazy that internally it isn't using shared-memory (because it wasn't available), but it is going to return "yes" just because MPI_Comm_get_info on its parent would return "yes" for "use_shared_memory".

So, the boils down to two questions:

Should MPI_Comm_dup blindly duplicate the key/value pairs of the previous communicator or should they be presented to the new communicator and the subscribers will determine what the get should return. I'm pretty sure for this we want to present values to the new communicator, not just blindly replicate them.

Assuming they are presented, should the key/values that were set on the original communicator be presented to the new communicator or should the key/values that get would return on the original communicator be presented to the new communicator? I really hope we can go with the get option so we don't have to store separate set and get values.

[dsolt]

I guess I should say that I actually think that during a Dup, the MPI_Comm_set_info values of the parent should be presented to the new communicator, not the MPI_Comm_get_info values of the parent comm. I don't like that answer since it means we have to store the set values for the purpose of dup, but it seems like the right thing to do.

[markalle]

When the MPI 3.1 standard describes MPI_Comm_dup() and MPI_Comm_dup_with_info() it twice implies that MPI_Comm_dup() will just blindly copy the info of its parent without subscriber callbacks modifying the values. But then in 6.4.4 "Communicator Info" it finally uses language that supports the more logical interpretation, where it says "when the communicator is duplicated using MPI_COMM_DUP or MPI_COMM_IDUP. In this case, all hints associated with the original communicator are also applied to the duplicated communicator". That key phrase "applied to" is what I want to see in the other two places, making it clear that subscriber-callbacks process the parent's info at MPI_Comm_dup() time rather than just duplicating them.

I think it's logical that in the case of MPI_Comm_dup(), the paren'ts current info is inherited to be used as the starting point, which then goes through the callbacks to become the new comm's potentially different info. And in the case of MPI_Comm_dup_with_info() the input info argument is the starting point.

That's an interesting idea, about saving all original info argument values rather than just using the current info values of the parent communicator, but I don't think that's what the standard is saying to do. Besides, I think MPI_Comm_dup_with_info() is the call that would be used by people who care about the info settings.

[markalle]

As far as making tests, we haven't done much yet.

If we're trying to specifically test the concept of components subscribing to info keys and potentially modifying them, we have the problem that MPI is allowed to keep a value, change it, or throw it away. So the testcase has no basis for deciding if MPI is behaving correctly or not.

I'm tempted to make the testcase directly use opal_infosubscribe_subscribe() to add its own callback to be triggered at MPI_Comm_set_info() time, but that definitely woudn't be a standards compliant test.

Or we could just write general tests using the MPI calls that were touched, and see if there are any catastrophic failures in the general areas of those functions.

[bosilca]

MPI_Comm_set_info is defined as a collective call. My understanding is that if we allow the new communicator to reevaluate the inherited hints, we basically force another collective upon communicator creation (one based on data with unknown size).

[dsolt]

MPI_Comm_set is collective, but does not necessarily mean it has to communicate. Our current implementation does not barrier and this PR doesn't introduce a barrier. But it is true that when presenting the hints to the underlying subscribers, the individual subscriber components may choose to do a collective operation which could be costly.

[jsquyres]

@dsolt Pro github tip: prefix blocks of C code with a line of exactly three single-ticks and the letter "c" (the three single ticks tell Github Markdown to make it be a verbatim / fixed-width section; the letter "c" means to syntax highlight the block with C-style syntax) and suffix the block with a line of exactly three single ticks (to close the verbatim block). I edited your comment from a few days ago to add this to make it a bit more readable.

[jsquyres]

This sounds like a thorny topic -- perhaps we should discuss this in Dallas next week? I'll add it to the agenda.

One whacky idea while I'm thinking about it (perhaps to be discussed next week?): @dsolt correctly identifies a dichotomy between the user-specified values and the OMPI-returned values -- it creates an ambiguity in MPI_COMM_DUP_WITH_INFO. Perhaps we should have two values (as Dave implies): one for the value that the user requested, and then another one for what OMPI actually did on this communicator. We can further have a naming convention for all such hints. E.g.:

• ompi_KEYNAME: prefix the key with ompi_ to make it clear that it is Open MPI-specific. The user/application sets the value associated with this key with MPI_INFO_SET this value, and then MPI_COMM_INFO_SETs it on the communicator. If the user later MPI_COMM_INFO_GETs the info and then MPI_INFO_GETs this key, it will guarantee to be the same as the value that they set via MPI_INFO_SET+MPI_COMM_INFO_SET.
• ompi_KEYNAME_provided: The same key name, but with the _provided suffix. The value will be the value that Open MPI actually decided to use. E.g., in Dave's example, it could be "yes" if there was shared memory available, or "no" if there was not.

In this way, when we MPI_COMM_DUP_WITH_INFO, the new communicator can always use the ompi_KEYNAME values to know what the user / application wants (vs. what Open MPI decided on the parent communicator).

Admittedly, this is a workaround for an unclear MPI standard issue. Perhaps the real fix will need to be debated up in the Forum...?

[jsquyres]

@markalle Your approach to testing this sounds correct (i.e., it may be more of a unit test of this infrastructure vs. an actual MPI application).

[markalle]

I like the idea of using prefix/suffixes. It seems like the easiest method for maintaining a copy of the incoming value set by the user, separate from the potentially modified value processed by callbacks.

I'm tempted to do the reverse of your example though, eg keep a "ompi_saved_[keyname]" key with the unmodified user-input and let the original key/value be modified. This seems more intuitive for the user who requests some setting and then queries to see what they received. Nothing in the standard currently would clue them in to look for "[keyname]_provided".

And to avoid a bunch of nuisance-code related to string-length, maybe only save the above extra key/val pair for recognized ompi keys which by convention we'd keep far enough below MPI_MAX_INFO_KEY to ensure room for a prefix/suffix.

[jsquyres]

@markalle Fair enough -- you're probably right that a user would expect to SET a key and then GET the same key back to see what happened. I also agree that we only need to keep values for keys that OMPI recognizes -- I think there's standard language that supports this, about how MPI_COMM_INFO_GET will only return hints that are "being used," or somesuch (i.e., if OMPI doesn't recognize it, it won't use it, so there's no point in storing it).

[bosilca]

The standard mandates the length of the keys. By adding a prefix or suffix we might, depending on the length of the user keys, need more than this predefined length. How do we deal with the worst case where we will need to truncate.

[markalle]

I guess we can wait then. I took the mpi-standard statement
"The info object returned in info_used will contain all hints currently active for this communicator. This set of hints may be greater or smaller than the set of hints specified when the communicator was created, as the system may not recognize some hints set by the user, and may recognize other hints that the user has not set."
as giving us free reign to do pretty much anything. Although it admittedly doesn't specifically say we can modify values.

[markalle]

Back to the testing topic: where should unit tests go for Open MPI? If the API ends up simple enough I can just extern a couple functions and call them from an independent program that's just linked against libmpi.so I might try that, but if it involves essentially making a test that lives inside the Open MPI library, would I make it something that can be #ifdefed or maybe have the test activiated by environment var?

Is there a mechanism / precedent to follow?

[jsquyres]

@markalle The Forum is arguing over that exact statement. You can follow the thread: http://lists.mpi-forum.org/mpi-forum/2016/02/3173.php

@markalle Regarding testing: we have 2 general areas for tests. Small unit tests live inside the test/ tree in the OMPI tree itself, and are activated by running "make check" (and they're run automatically when we make official distribution trarballs). Outside of that, we put stuff in the ompi-tests repo. We originally gathered a bunch of test suites in there, and then started supplementing those test suites with our own additions and whatnot. Probably the one that has grown the most with our own custom tests is the IBM test suite (which is MPI API-level tests, not unit tests of OPAL or ORTE).

[gpaulsen]

As a reminder, @jsquyres please discuss with MPI Forum and try to drive to some conclusion. Oh, and herd some cats into a bag while you're at it.

[jsquyres]

Due to an extraordinarily-long agenda at the Forum this week, we did not get to fully address this issue. 😦

Here's what we got:

1. It was decided that @jdinan will make an errata proposal to have any info attached to the communicator (and file and window) not propagate when the communicator is dup'ed (etc.). The text changes for this is largely done (and are very easy). The Forum seemed very receptive to this idea; it's likely to pass. So we can probably go ahead and modify OMPI code to effect this behavior.
2. It was noticed that the text for MPI_COMM_SET_INFO does not specify whether setting an info on a comm (and file and win) completely replaces all hints that were set before, or whether it merges them in with any hints that were previously set.
   • Jeff's $0.02: the function is called "SET" -- it should wholly replace any hints that were set before. If the function were called "ADD," then it could add hints to hints that were already set on the communicator (or file or win). But it's not. It's called "SET."
3. MPI_COMM_GET_INFO is still a train wreck; there isn't any consensus on what exactly it should do (we literally ran out of time to talk this through further).
   • Jeff's $0.02: it should return exactly the same hints that were set by the application. There should be some new/different kind of operation that provides information on how the implementation reacted to those hints.
   • There was some talk of using MPI_T to find out how the implementation to reacted to the hints, but the downside of that is that there (currently) are no Fortran bindings for MPI_T.

So it's now up to us (and possibly @jdinan, if he cares) to propose what MPI_COMM_SET_INFO and MPI_COMM_GET_INFO should do. We should probably all carefully read what MPI-3.1 says and then make a proposal. Bonus points will be awarded if the proposal can be an interpretation of the current text, but it's quite possible (likely?) that the current text is simply totally borked, and we'll just have to propose new text that isn't an interpretation of the current text.

[markalle]

I'm okay with MPI_Comm_dup() inheriting nothing, as that would be in line with all the other comm creation routines. The code is pretty easy to swap between various interpretations if we need to later as well.  Right now there is quite a bit of extra code that allows values to change while a backup of the original remains behind, and that's unnecessary baggage under the current interpretation. But I kind of hate to rip it out. Right now you get what you set, but the code is taking a round-about method of making that happen. Internally it has both the original and the modified settings lying around.
 
I do notice though that every time there's discussion on the behavior upon inheritance, people mention windows/files as well. As far as I know there is no symmetrical situation for windows/files.  Ie there is an MPI_Comm_dup() but there's no MPI_File_dup() or MPI_Win_dup() call in the standard (unless it's new and I'm behind the times).
 
As a side note the MPI_Win_create_keyval() seems to think there's such a thing as MPI_Win_dup() since that call takes an MPI_Win_copy_attr_function callback as an argument.  But I guess it's either doing that for the sake of symmetry or to be compatible with a hypothetical future MPI_Win_dup(). But right now I think there's no circumstance in which that callback should be triggered.

[jsquyres]

I'm okay with MPI_Comm_dup() inheriting nothing, as that would be in line with all the other comm creation routines. The code is pretty easy to swap between various interpretations if we need to later as well. Right now there is quite a bit of extra code that allows values to change while a backup of the original remains behind, and that's unnecessary baggage under the current interpretation. But I kind of hate to rip it out. Right now you get what you set, but the code is taking a round-about method of making that happen. Internally it has both the original and the modified settings lying around.

I wouldn't rip it out yet -- I think there's still a question of what to do with MPI_COMM_GET_INFO.

I voiced my opinion up above (see #1308 (comment)) -- do you agree?

I do notice though that every time there's discussion on the behavior upon inheritance, people mention windows/files as well. As far as I know there is no symmetrical situation for windows/files. Ie there is an MPI_Comm_dup() but there's no MPI_File_dup() or MPI_Win_dup() call in the standard (unless it's new and I'm behind the times).

True (no, you're not behind the times).

As a side note the MPI_Win_create_keyval() seems to think there's such a thing as MPI_Win_dup() since that call takes an MPI_Win_copy_attr_function callback as an argument. But I guess it's either doing that for the sake of symmetry or to be compatible with a hypothetical future MPI_Win_dup(). But right now I think there's no circumstance in which that callback should be triggered.

Good point. I'll pass this on to the RMA working group.

[jdinan]

@jsquyres, sure, I'm happy to help with the spec work.

[dsolt]

I'm not sure if this discussion should continue in this PR or be moved
elsewhere? For now... a few comments:

It was decided that @jdinan will make an errata proposal to have any info attached to the communicator (and file and window) not propagate when the communicator is dup'ed (etc.).

Would this mean that MPI_Comm_dup_with_info is identical to MPI_Comm_dup +
MPI_Comm_set_info? In that case that makes MPI_Comm_dup_with_info a type
of convenience routine that we don't generally do in MPI. I could see it
having some value since MPI_Comm_dup_with_info may (doesn't have to!)
barrier and it might be possible to have a faster implementation when the
two calls are combined. Still, pretty sure that was not the original
intent. There were no forum members who where around when the Info stuff
was put in who spoke up and said, "This is what we had in mind"?

I agree with your interpretation of what "SET" should do (replace, not
add). There are still strangeness about what exactly an implementation
should do in some cases. FOO has a default of OFF, MPI_Comm_set_info
sets it to "ON" and then MPI_Comm_set_info doesn't specify it all. Should
it be on or off?

Dave

EDIT: Lotus notes really does horrid things when replying to github issues...

[jdinan]

For some info hints, it may only be possible to apply them when the communicator is created (i.e. there's no pending communication since the communicator hasn't been created yet). MPI_Comm_set_info (and also any of the constructors that take an info argument) may not succeed in setting the hints the user supplied. MPI_Comm_get_info was intended to be paired with MPI_Comm_set_info so users could check if the call to set info had the desired effect.

[markalle]

My least favorite aspect of the current code (which I have a testcase for and which finally has no memory leaks, but for which I still need to create a pull request) is the location from which the
    opal_infosubscribe_change_info()
function is called.
 
Once inside that function I think what we do is okay, and that's what most of the code in this checkin is about. But where the change_info is triggered is kind of sketchy:

1. At MPI_{Comm,Win,File}_set_info() time
2. At MPI_{Comm,Win,File}_get_info() time if no info object has yet been attached
3. At MPI_Comm_{dup,idup} time (somewhat arbitrarily located after ompi_comm_activate()/ompi_comm_activate_nb().
    
   Specifically item 2 is what I don't like.  I'd rather it be a more fundamental part of object creation that happens early with a NULL info argument where defaults would be applied, then still have BTL checkpoint friendly #1 and Provide warnings if unknown MCA params used #3.
    
   Mark
    
    
   ----- Original message -----From: James Dinan [email protected]: open-mpi/ompi [email protected]: Mark Allen/Dallas/IBM@IBMUSSubject: Re: [ompi] MPI_Info update (2nd / corrected PR) (MPI_Info update (2nd / corrected PR) #1308)Date: Mon, Mar 7, 2016 8:54 AM 
   For some info hints, it may only be possible to apply them when the communicator is created (i.e. there's no pending communication since the communicator hasn't been created yet). MPI_Comm_set_info (and also any of the constructors that take an info argument) may not succeed in setting the hints the user supplied. MPI_Comm_get_info was intended to be paired with MPI_Comm_set_info so users could check if the call to set info had the desired effect.
   —Reply to this email directly or view it on GitHub.

 

[bosilca]

@markalle, for MPI_COMM_IDUP the info should be copied as early as possible in the duplication process, preferably before we return from MPI_COMM_IDUP (or we will need some versioning mechanism). Doing it during the call to activate might be too late, I would rather move it up in ompi_comm_idup_internal (right after ompi_comm_set_nb?).

[markalle]

Okay, I changed it again. Previously the infosubscriber design wouldn't have worked well with duping the info as early as possible, because all activation of the callbacks including the initial activation was tied to the setting of the info. So that design would have resulted in setting the comm's info early, walking a non-existent list of callbacks, and then later having modules register callbacks they want to have triggered.
 
Now the callbacks are triggered

1. when a module registers them on an object
2. at MPI_Comm_set_info() time
   and specifically not on the initial copy of info into a new object being created. And that copy is expected to happen early so callback registrations will see the info.
    
   This did unfortunately lower the quality of my test somewhat. Previously my testcase was following mostly the same path as the real product would. Now my testcase is basically manually triggering callbacks then checking to see if those callbacks were triggered, which of course they were.  But I think it's a more logical design overall since triggering a module's callback at the time the module registers it removes ambiguity about triggering them too early or too late.
    
   Mark
    
    
   ----- Original message -----From: bosilca [email protected]: open-mpi/ompi [email protected]: Mark Allen/Dallas/IBM@IBMUSSubject: Re: [ompi] MPI_Info update (2nd / corrected PR) (MPI_Info update (2nd / corrected PR) #1308)Date: Mon, Mar 7, 2016 6:42 PM 
   @markalle, for MPI_COMM_IDUP the info should be copied as early as possible in the duplication process, preferably before we return from MPI_COMM_IDUP (or we will need some versioning mechanism). Doing it during the call to activate might be too late, I would rather move it up in ompi_comm_idup_internal (right after ompi_comm_set_nb?).
   —Reply to this email directly or view it on GitHub.

 

[hjelmn]

Can you rebase on master now that the mpool/rcache rewrite is in?

[ibm-ompi]

Build Failed with XL compiler! Please review the log, and get in touch if you have questions.

[jsquyres]

This PR was discussed today in the OMPI face-to-face meeting in Dallas.

Conclusions:

1. The MPI Forum is leaning towards not propagating info when comms are dup'ed.
2. The MPI Forum has not resolved the "what info keys / values do you get when you call MPI_*_INFO_GET?" question.
3. Open MPI at least sorta half decided on the "what info do you get?" question: in the WIN functions, you'll get all the possible info keys back. This may or may not be what the Forum ultimately decides.
4. This PR is super stale and needs to be re-based
   • Additionally, @markalle indicated that he has bug fixes to contribute to this PR.
   • @jjhursey and/or @gpaulsen will work with @markalle to get his fixes included in this PR.
5. In order to actually make progress on this PR -- i.e., so we can get opal_info_t around in OPAL, it seems like we should split this PR into (at least?) two PRs:
   1. One that adds the opal_info_t plumbing
   2. Another that adds the infrastructure for MPI_*_INFO_SET/GET, etc.

This allows other parts of OMPI to start using opal_info_t in OPAL, but we can still wait for the Forum to make a decision w.r.t. MPI_*_INFO_GET.

[jdinan]

1. The no propagation in dup ticket is ready for voting at the next meeting.
2. On my to-do list, unless someone else gets to it first. Seems likely that the semantic will be "whatever the implementation wants to tell you when you ask."

[gpaulsen]

:bot:retest

[ibm-ompi]

Build Failed with XL compiler! Please review the log, and get in touch if you have questions.

Gist: https://gist.github.com/dbb1e3d9ac621d7f658b22f51a29c5f3

[jsquyres]

A proposal was just filed with the MPI Forum on the "What happens when you MPI_*_GET_INFO?" question: mpi-forum/mpi-issues#63

[jsquyres]

Please add a Signed-off-by line to this PR's commit.

[jjhursey]

Replaced by PR #2941