han bugfix: coll_han_allreduce_low_module does not set the correct module #11823

wenduwan · 2023-07-14T01:23:35Z

Currently coll_han_allreduce_low_module does not set the correct intra-node collectives module, i.e. tuned vs sm.

The root cause is the ompi_comm_coll_preference hint is not correctly passed and stored in the new communicator's s_info(I'm not sure about the naming - does it mean subscriber info?). This patch addresses that issue.

github-actions · 2023-07-14T01:24:15Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

0d10025: han: explicitly exclude sm/tuned when splitting co...

check_signed_off: does not contain a valid Signed-off-by line

5403341: opal info_subscriber: update subscriber object s_i...

check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

wenduwan · 2023-07-14T01:24:49Z

opal/util/info_subscriber.c

-                && 0 != strncmp(updated_value, value_str->string, value_str->length)) {
-                err = opal_info_set(object->s_info, iterator->ie_key->string, updated_value);
-            }
+            opal_infosubscribe_inform_subscribers(object, iterator->ie_key->string,


Reading opal_infosubscribe_inform_subscribers I do not think the return value has any significance. Hence we should always update object->s_info

wenduwan · 2023-07-14T02:12:13Z

CI failure

 ++ /GIT/ompi-refs_pull_11823_merge/ompi/ompi_install/bin/mpicc -o /GIT/ompi-refs_pull_11823_merge/jenkins_scripts/jenkins/ompi/c11_test /GIT/ompi-refs_pull_11823_merge/jenkins_scripts/jenkins/ompi/c11_test.c /GIT/ompi/ompi_install/lib/liboshmem.so
  gcc: error: /GIT/ompi/ompi_install/lib/liboshmem.so: No such file or directory

I'm not sure if this is related - @jsquyres could you help me understand the problem? Thanks!

B-a-S · 2023-07-14T05:48:30Z

CI failure

 ++ /GIT/ompi-refs_pull_11823_merge/ompi/ompi_install/bin/mpicc -o /GIT/ompi-refs_pull_11823_merge/jenkins_scripts/jenkins/ompi/c11_test /GIT/ompi-refs_pull_11823_merge/jenkins_scripts/jenkins/ompi/c11_test.c /GIT/ompi/ompi_install/lib/liboshmem.so
  gcc: error: /GIT/ompi/ompi_install/lib/liboshmem.so: No such file or directory

I'm not sure if this is related - @jsquyres could you help me understand the problem? Thanks!

The test is fixed and passed

gkatev · 2023-07-14T07:53:52Z

ompi/mca/coll/han/coll_han_subcomms.c

     * This sub-communicator contains the ranks that share my node.
     */
-    opal_info_set(&comm_info, "ompi_comm_coll_preference", "tuned,^han");
+    opal_info_set(&comm_info, "ompi_comm_coll_preference", "tuned,^han,^sm");


Not sure that this is necessary, but anyway if memory serves right, it has to be tuned,^han,sm (only one ^); needs checking

@gkatev @wenduwan That is correct -- there should only be a single ^

gkatev · 2023-07-14T07:57:08Z

related: #10335

Edit: FYI the patch that I have at the end of that issue (with opal_info_dup) works, but I really don't know if it's suitable/elegant

wenduwan · 2023-07-14T14:51:07Z

Thanks all. I excluded the problematic commit.
@gkatev thank you for pointing me to the issue - I wasn't aware of the conversation. I think your patch makes sense, so I included it in this PR(not trying to steal your thunder 😅).

TBH I don't have a perfect solution to #10335, and I agree with @bosilca that this is more of a bandaid. But I think this patch does make things marginally better.

I'm open to suggestions.

gkatev · 2023-07-14T14:58:47Z

Thank you for the work on this. I don't know if the opal_info_dup patch is perfect, but it's been working without hiccups (but perhaps it's not robust and may spuriously fail??)

The root problem that this PR is trying to address is the same as #10335 right? I mean, here the problem is ompi_comm_coll_preference, but from what I understand there is nothing special about ompi_comm_coll_preference, and the issue should apply to all info keys (?).

wenduwan · 2023-07-14T15:01:55Z

The root problem that this PR is trying to address is the same as #10335 right? I mean, here the problem is ompi_comm_coll_preference, but from what I understand there is nothing special about ompi_comm_coll_preference, and the issue should apply to all info keys (?).

I agree. The issue I experienced was merely a symptom. This patch should apply to all info in general.

wenduwan · 2023-07-19T20:26:34Z

👋 @bosilca Could you kindly review this change? Thank you!

bosilca

I understand what the problem is, but I don't understand the proposed solution. Why is there a need to duplicate the info before updating them ?

opal/util/info_subscriber.c

wenduwan · 2023-07-19T23:01:44Z

opal/util/info_subscriber.c

+            if (0 < update_cnt) {
+                err = opal_info_set(object->s_info, iterator->ie_key->string,
+                                    iterator->ie_value->string);


I understand what the problem is, but I don't understand the proposed solution. Why is there a need to duplicate the info before updating them ?

@bosilca To me the confusion is actually here - is opal_infosubscribe_change_info supposed to also update object->s_info or not. Correct me if I'm wrong, but the code reads to me that opal_infosubscribe_change_info is only responsible for notifying the 'subscribers' of new_info. L260-L262 somewhat looks out of place.

This leads me to think that setting object->s_info(via opal_info_dup(info, &newcomp->super.s_info)) should be separate from opal_infosubscribe_change_info.

Please let me know what I missed.

For what the extra information might be worth, there were opal_info_dup calls present before 6bd36a7:

ompi/ompi/communicator/comm.c

Lines 613 to 617 in 3b4d64f

/* Copy info if there is one */

if (info) {

newcomp->super.s_info = OBJ_NEW(opal_info_t);

opal_info_dup(info, &(newcomp->super.s_info));

}

After digging into the code it seems the root cause was a lack of validation and a lack of understanding of the impact of 6bd36a7 into the code base. This issue with the info is just one of the many other issues that got into the main, in exchange of being able to create communicators with a tag given by an external software stack (aka to create sessions).

As a result, I retract my original complain about duplicating the info. I still have an issue with the changes in opal/util/info_subscriber.c.

wenduwan · 2023-08-30T15:54:41Z

@bosilca Do you have more thoughts on this?

wenduwan · 2023-11-15T15:28:11Z

@bosilca Heartbeat in case it's buried.

wenduwan · 2024-01-12T03:13:57Z

@bosilca @gkatev Do you have more comments on this PR? We are currently blocked on this - the sm coll cannot be selected for HAN allreduce.

gkatev · 2024-01-30T20:12:57Z

I did test this again and can again confirm that (as far as I can tell) it does solve the info-key/HAN problem.

I'm also trying again to understand the info-subscriber code. I guess the big question here is what opal_infosubscribe_change_info() is supposed to do? I can see two possibilities:

It goes through the keys in new_info, and if a subscriber decides to update the key, it updates its value in object->s_info.
It goes through the keys in new_info, and adds everything to object->s_info, whether as-is, or modified as dictated by a subscriber.
- (If it's supposed to do this, the current implementation's "as-is" part is bugged from what I understand)

Am I on point here? This PR here looks like it operates on the first assumption? (as my patch in #10335 did)

Does the implementation of e.g. MPI_Comm_set_info(), which uses opal_infosubscribe_change_info(), help us arrive to either of the possibilities above?

ompi/ompi/mpi/c/comm_set_info.c

Line 49 in 3b6fe68

opal_infosubscribe_change_info(&(comm->super), &(info->super));

gkatev · 2024-01-30T20:29:09Z

opal/util/info_subscriber.c

-                updated_value = item->callback(object, key, updated_value);
-                if (found_callback) {
-                    *found_callback = 1;
+                if (item->callback(object, key, new_value)) {


Here we are throwing away the value returned from the callback. Because we are assuming that the potentially updated key will be saved in new_value? But seeing some instances where a subscriber is utilized (mca_pml_ob1_set_allow_overtake, ompi_osc_ucx_set_no_lock_info), the opposite appears to happen. (this looks to be a result of somewhat confusing variable naming in the original implementation?)

@wenduwan any thoughts on this? I'm looking at e.g. mca_pml_ob1_set_allow_overtake(), which appears to be an info-subscriber callback (registered in mca_pml_ob1_add_comm()):

ompi/ompi/mca/pml/ob1/pml_ob1.c

Lines 268 to 270 in bb7ecde

/* Register the subscriber alert for the mpi_assert_allow_overtaking info. */

opal_infosubscribe_subscribe (&comm->super, "mpi_assert_allow_overtaking",

"false", mca_pml_ob1_set_allow_overtake);

ompi/ompi/mca/pml/ob1/pml_ob1.c

Lines 208 to 243 in bb7ecde

static const char*

mca_pml_ob1_set_allow_overtake(opal_infosubscriber_t* obj,

const char* key,

const char* value)

{

ompi_communicator_t *ompi_comm = (ompi_communicator_t *) obj;

bool allow_overtake_was_set = OMPI_COMM_CHECK_ASSERT_ALLOW_OVERTAKE(ompi_comm);

/* As we keep the out-of-sequence messages ordered by their sequence, as a receiver we

* can just move the previously considered out-of-order messages into the unexpected queue,

* and we maintain some form of logical consistency with the message order.

*/

if (opal_str_to_bool(value)) {

if (!allow_overtake_was_set) {

ompi_comm->c_flags |= OMPI_COMM_ASSERT_ALLOW_OVERTAKE;

mca_pml_ob1_merge_cant_match(ompi_comm);

}

return "true";

}

if (allow_overtake_was_set) {

/* However, in the case we are trying to turn off allow_overtake, it is not clear what

* should be done with the previous messages that are pending on our peers, nor with

* the messages currently in the network. Similarly, if one process turns off allow

* overtake, before any potential sender start sending valid sequence numbers there

* is no way to order the messages in a sensible order.

* The possible solution is cumbersome, it would force a network quiescence followed by

* a synchronization of all processes in the communicator, and then all peers will

* start sending messages starting with sequence number 0.

* A lot of code for minimal benefit, especially taking in account that the MPI standard

* does not define this. Instead, refuse to disable allow overtake, and at least the

* user has the opportunity to check if we accepted to change it.

*/

return "true";

}

return "false";

}

The modified info value is returned from the function. From what I understand we need to keep this returned value and pass it to opal_info_set() in opal_infosubscribe_change_info().

@gkatev Many apologies for missing your comment earlier.

I took another look at this change. I don't quite understand how the callback value can be useful in this case(and now I doubt if this change is even correct).

Consider this snippet:

OPAL_LIST_FOREACH (item, list, opal_callback_list_item_t) { updated_value = item->callback(object, key, updated_value); ... }

This code iterates over a list of subscribers, and invokes the callback with a mutable variable updated_value, and after the loop updated_value will be set to the last callback's return value. I have 2 problems/questions about this approach:

Why would it invoke the callback with a changing update_value - this means the order of subscribers matters. I do not understand this.

Why would it only keep the last updated_value and ignore the others? Again this means the order of subscribers must be critical.

@bosilca @devreal Could you please give some background about the original purpose of opal_infosubscribe_inform_subscribers?

The MPI info values are complicated to handle, because they need to reflect not only what the user intent but also what the library accept. Thus, in some cases the user should be able to investigate which of the info provided will be taken into consideration by the implementation. This mechanism is set in place in OMPI via the subscribers, when the user sets a value we inform the subscribers about it and then we set it with a flag to indicate if the it was accepted or not.

Let's add this into the next week's agenda, as it deserves a more in-depth discussion.

Added to meeting agenda

This code iterates over a list of subscribers, and invokes the callback with a mutable variable updated_value, and after the loop updated_value will be set to the last callback's return value. I have 2 problems/questions about this approach:

1. Why would it invoke the callback with a changing `update_value` - this means the order of subscribers matters. I do not understand this. 2. Why would it only keep the last `updated_value` and ignore the others? Again this means the order of subscribers must be critical.

Hmm I see yes. With the current state of the callbacks it generally seems like we have to keep and use the return value, but your concerns above do indeed raise deeper questions about how all this is supposed to work (and I personally don't really know 🙂),

Because it was never meant to support multiple subscribers on the same info key. Basically the idea is that an info object belongs, or could impact, a single subscriber. Which means that is a single subscriber that will modify the value, while everyone else might just look at it. The order might matter, but clearly we never had an issue with the ordering so far.

gkatev · 2024-04-20T09:45:30Z

Let me try a pre-meeting recap of my understanding of the root of this specific issue, just to make sure that we are on the same page (or at least that I'm on the correct one).

Why is there a need to duplicate the info before updating them ?

It's not duplication per se, it's a copy into the new communicator.

In the past, at 81917f2, ompi_comm_split_type() did contain an opal_info_dup() call:

ompi/ompi/communicator/comm.c

Lines 935 to 939 in 81917f2

    
           // Copy info if there is one. 
        
           newcomp->super.s_info = OBJ_NEW(opal_info_t); 
        
           if (info) { 
        
               opal_info_dup(info, &(newcomp->super.s_info)); 
        
           }

The way I understand it is that it's simply copying the passed info into the newly created comm. (the coll components then read this info during the ompi_comm_activate() call)

(Similar situation in ompi_comm_dup_with_info(), L1039-1043)

This changes in 6bd36a7, where opal_info_dup() is replaced with opal_infosubscribe_change_info():

ompi/ompi/communicator/comm.c

Lines 963 to 967 in 6bd36a7

    
           ompi_comm_assert_subscribe (newcomp, OMPI_COMM_ASSERT_LAZY_BARRIER); 
        
           ompi_comm_assert_subscribe (newcomp, OMPI_COMM_ASSERT_ACTIVE_POLL); 
        
           if (info) { 
        
               opal_infosubscribe_change_info(&newcomp->super, info); 
        
           }

(Similarly in ompi_comm_dup_with_info(), L1068-1072)

This is where the bug is introduced. The problem is that opal_infosubscribe_change_info() only sets values that get returned from subscribers, so keys like ompi_comm_coll_preference are never added to the new comm, and never get passed to coll base/components.

Now, perhaps opal_infosubscribe_change_info() is supposed to be doing something different than it currently is?? Like adding all info keys, whether as-is or as-modified by subscribers? Or maybe the opal_info_dup() calls were simply forgotten? Or perhaps the use case of collective components passing around info keys was not fully taken into account.

I'll leave it up to you to discuss how it's all supposed to be in the grand scheme of things!

This patch adds missing info duplication steps. Unused/unreferenced info will be removed in later calls to opal_info_remove_unreferenced Signed-off-by: Wenduo Wang <wenduwan@amazon.com>

wenduwan · 2024-04-26T15:19:18Z

After face-to-face discussion with @bosilca and @devreal , we recognize that the info subscriber subsystem is bust and needs major rework. For this PR we only need to duplicate the info.

bosilca · 2024-04-26T15:28:02Z

Indeed, @devreal and myself reviewed the info/subscriber code and come to the conclusion that is does not do what it was required to, neither by the MPI standard nor by the users. Forcefully adding the keys, as proposed in this patch will only solve the HAN issue, while in same time breaking the MPI requirements (because the subscribers infrastructure lost the ability to remove keys that will be ignored). We figured out what needs to be done (mostly undoing an old patch), and @devreal volunteered to do so.

Meanwhile, I'm skeptical about merging this PR as it breaks as many things as it fixes.

wenduwan · 2024-04-26T15:30:35Z

@bosilca Ah thanks for the elaboration. I will cancel this PR then and follow up with @devreal .

wenduwan requested a review from bosilca July 14, 2023 01:23

github-actions bot added the Target: main label Jul 14, 2023

wenduwan commented Jul 14, 2023

View reviewed changes

wenduwan force-pushed the info_subscriber branch from 0d10025 to 3b5a528 Compare July 14, 2023 01:50

wenduwan added the bug label Jul 14, 2023

gkatev reviewed Jul 14, 2023

View reviewed changes

wenduwan force-pushed the info_subscriber branch 2 times, most recently from 93d3a67 to 996713d Compare July 14, 2023 14:48

bosilca reviewed Jul 19, 2023

View reviewed changes

opal/util/info_subscriber.c Outdated Show resolved Hide resolved

wenduwan force-pushed the info_subscriber branch from 996713d to 040ad7c Compare July 19, 2023 22:51

wenduwan commented Jul 19, 2023

View reviewed changes

wenduwan self-assigned this Nov 15, 2023

wenduwan requested review from bosilca and gkatev January 12, 2024 03:12

gkatev reviewed Jan 30, 2024

View reviewed changes

wenduwan mentioned this pull request Apr 18, 2024

Info key not propagated during communicator creation #10335

Closed

ompi/communicator: duplicate provided opal info in the new communicator

64db513

This patch adds missing info duplication steps. Unused/unreferenced info will be removed in later calls to opal_info_remove_unreferenced Signed-off-by: Wenduo Wang <wenduwan@amazon.com>

wenduwan force-pushed the info_subscriber branch from 040ad7c to 64db513 Compare April 26, 2024 15:17

wenduwan closed this Apr 26, 2024

devreal mentioned this pull request Apr 26, 2024

Fix the info subscriber mechanism and hidden info keys #12498

Merged

devreal mentioned this pull request May 7, 2024

[5.0.x] Fix the info subscriber mechanism and hidden info keys #12529

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

han bugfix: coll_han_allreduce_low_module does not set the correct module #11823

han bugfix: coll_han_allreduce_low_module does not set the correct module #11823

wenduwan commented Jul 14, 2023

github-actions bot commented Jul 14, 2023

wenduwan Jul 14, 2023

wenduwan commented Jul 14, 2023

B-a-S commented Jul 14, 2023

gkatev Jul 14, 2023 •

edited

jsquyres Jul 14, 2023

gkatev commented Jul 14, 2023 •

edited

wenduwan commented Jul 14, 2023

gkatev commented Jul 14, 2023

wenduwan commented Jul 14, 2023

wenduwan commented Jul 19, 2023

bosilca left a comment

wenduwan Jul 19, 2023

gkatev Jul 20, 2023

bosilca Apr 20, 2024

wenduwan commented Aug 30, 2023

wenduwan commented Nov 15, 2023

wenduwan commented Jan 12, 2024

gkatev commented Jan 30, 2024 •

edited

gkatev Jan 30, 2024 •

edited

gkatev Apr 19, 2024 •

edited

wenduwan Apr 19, 2024

bosilca Apr 19, 2024

wenduwan Apr 19, 2024

gkatev Apr 20, 2024

bosilca Apr 20, 2024

gkatev commented Apr 20, 2024 •

edited

wenduwan commented Apr 26, 2024

bosilca commented Apr 26, 2024

wenduwan commented Apr 26, 2024

	/* Copy info if there is one */
	if (info) {
	newcomp->super.s_info = OBJ_NEW(opal_info_t);
	opal_info_dup(info, &(newcomp->super.s_info));
	}

	/* Register the subscriber alert for the mpi_assert_allow_overtaking info. */
	opal_infosubscribe_subscribe (&comm->super, "mpi_assert_allow_overtaking",
	"false", mca_pml_ob1_set_allow_overtake);

	static const char*
	mca_pml_ob1_set_allow_overtake(opal_infosubscriber_t* obj,
	const char* key,
	const char* value)
	{
	ompi_communicator_t ompi_comm = (ompi_communicator_t ) obj;
	bool allow_overtake_was_set = OMPI_COMM_CHECK_ASSERT_ALLOW_OVERTAKE(ompi_comm);

	/* As we keep the out-of-sequence messages ordered by their sequence, as a receiver we
	* can just move the previously considered out-of-order messages into the unexpected queue,
	* and we maintain some form of logical consistency with the message order.
	*/
	if (opal_str_to_bool(value)) {
	if (!allow_overtake_was_set) {
	ompi_comm->c_flags \|= OMPI_COMM_ASSERT_ALLOW_OVERTAKE;
	mca_pml_ob1_merge_cant_match(ompi_comm);
	}
	return "true";
	}
	if (allow_overtake_was_set) {
	/* However, in the case we are trying to turn off allow_overtake, it is not clear what
	* should be done with the previous messages that are pending on our peers, nor with
	* the messages currently in the network. Similarly, if one process turns off allow
	* overtake, before any potential sender start sending valid sequence numbers there
	* is no way to order the messages in a sensible order.
	* The possible solution is cumbersome, it would force a network quiescence followed by
	* a synchronization of all processes in the communicator, and then all peers will
	* start sending messages starting with sequence number 0.
	* A lot of code for minimal benefit, especially taking in account that the MPI standard
	* does not define this. Instead, refuse to disable allow overtake, and at least the
	* user has the opportunity to check if we accepted to change it.
	*/
	return "true";
	}
	return "false";
	}

han bugfix: coll_han_allreduce_low_module does not set the correct module #11823

han bugfix: coll_han_allreduce_low_module does not set the correct module #11823

Conversation

wenduwan commented Jul 14, 2023

github-actions bot commented Jul 14, 2023

Choose a reason for hiding this comment

wenduwan commented Jul 14, 2023

B-a-S commented Jul 14, 2023

gkatev Jul 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatev commented Jul 14, 2023 • edited

wenduwan commented Jul 14, 2023

gkatev commented Jul 14, 2023

wenduwan commented Jul 14, 2023

wenduwan commented Jul 19, 2023

bosilca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenduwan commented Aug 30, 2023

wenduwan commented Nov 15, 2023

wenduwan commented Jan 12, 2024

gkatev commented Jan 30, 2024 • edited

gkatev Jan 30, 2024 • edited

Choose a reason for hiding this comment

gkatev Apr 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatev commented Apr 20, 2024 • edited

wenduwan commented Apr 26, 2024

bosilca commented Apr 26, 2024

wenduwan commented Apr 26, 2024

gkatev Jul 14, 2023 •

edited

gkatev commented Jul 14, 2023 •

edited

gkatev commented Jan 30, 2024 •

edited

gkatev Jan 30, 2024 •

edited

gkatev Apr 19, 2024 •

edited

gkatev commented Apr 20, 2024 •

edited