ompi icon indicating copy to clipboard operation
ompi copied to clipboard

ompi_info_t does not follow OMPI object semantics

Open devreal opened this issue 3 years ago • 4 comments

According to the discussion on https://github.com/open-mpi/ompi/pull/10349, the ompi_info_t is a) an opal_object_t and can thus be allocated using OBJ_NEW, but b) must (in the general case) be allocated using ompi_info_allocate to make sure the ompi instance is properly retained and released. We cannot move the instance retain/release into the constructor because that would create a cyclic dependency on the release, causing the instance to never shut down. This dichotomy is somewhat annoying.

It is not clear why the info object must interact with the ompi instance in the first place. Some information would be appreciated to make a decision on how to move forward. We might want to merge https://github.com/open-mpi/ompi/pull/10349 to fix an obvious bug and then deliberate on whether to change the info semantics.

I marked this issue as a blocker because it's a bug in master and v5.0.x. A fix is at https://github.com/open-mpi/ompi/pull/10349.

devreal avatar May 17 '22 15:05 devreal

Removing the blocker label since https://github.com/open-mpi/ompi/pull/10349 was merged and the user-facing issue was fixed. The underlying discrepancy between info allocation and object semantics remains though and should be addressed.

devreal avatar May 25 '22 13:05 devreal

5.0.x: https://github.com/open-mpi/ompi/pull/10420

awlauria avatar Jun 13 '22 19:06 awlauria

Sorry - @devreal is this ready to be closed or should it remain open? I am not sure if there are other pr's that addressed the issue.

awlauria avatar Jun 13 '22 19:06 awlauria

We should leave it open for now. The fixes in https://github.com/open-mpi/ompi/pull/10420 and https://github.com/open-mpi/ompi/pull/10349 didn't fix the underlying issue of info objects not being pure ompi objects. We'll have to discuss the path forward on this.

devreal avatar Jun 13 '22 19:06 devreal