Remove some more symbols removed in MPI 3.0 #6625

I always worry about changing the values of enums, especially in something as core as the dataytpe enums.
Would it be better to just NOT define the removed enums and constants, and leave the other values the same?

If (as in this PR) almost all of the constants/enums changed values, it might affect any hard coded values elsewhere in Open MPI (hopefully none) or in user's code. (That would be an unportable/non-standard usage, but we could be a bit kinder if we don't change ALL of these values).

This change might also affect other non-open sourced PMLs that might use Open MPI's datatype engine.

Thoughts?

jsquyres · 2019-04-30T19:59:12Z

@ggouaillardet This patch looks pretty good -- thanks!

We talked about this on the Webex today. We tend to agree with @gpaulsen's question: can we not do the integer handle re-ordering? It would probably be ok to leave "holes" in the integer handle ranges -- just put comments in the relevant places in the code explaining why there are holes there.

Thanks!

ggouaillardet · 2019-05-01T01:45:33Z

I am fine to implement the suggested change(s).

Just to be clear, the renumbering thing occurs in two places

when MPI_COMBINER_{HINDEXED,HVECTOR,STRUCT}_INTEGER are removed from a public (but anonymous) enum
when MPI_LB and MPI_UB were removed from the internal datatypes.

In the first case, I can avoid renumbering by replacing MPI_COMBINER_HINDEXED_INTEGER and friends with OMPI_REMOVED_MPI_COMBINER_HVECTOR_INTEGER and friends (I did that for the v4.0.x branch).

In the second case, that will require a small trick, since the MOOG() macro is expected to be called in sequence

#define MOOG(name, index)                                               \
    do {                                                                \
        ompi_mpi_##name.dt.d_f_to_c_index =                             \
            opal_pointer_array_add(&ompi_datatype_f_to_c_table, &ompi_mpi_##name); \
        if( ompi_datatype_number_of_predefined_data < (ompi_mpi_##name).dt.d_f_to_c_index ) \
            ompi_datatype_number_of_predefined_data = (ompi_mpi_##name).dt.d_f_to_c_index; \
        assert( (index) == ompi_mpi_##name.dt.d_f_to_c_index );         \
    } while(0)

We already broke the ABI compatibility when we removed the MPI1 symbols, so I naively finished the job even if I had to break the ABI compatibility even more.
And as I wrote earlier, I undertand your concern and will implement the requested changes.
Please tell me if I should avoid all renumbering, or only avoid the second renumbering.

gpaulsen · 2019-05-01T21:48:06Z

Thanks. I don't know the details well enough to comment. Hopefully @jsquyres or others can comment. Or we'll discuss next week.

jsquyres

I see what you're saying about the renumbering -- you made a sane choice, but the amount of secondary renumbering it caused made us all nervous. I think your rationale is sound -- one way is probably no better than the other (i.e., deliberately leave a bogus name in an enum which just reminds us forever that something was deleted, or renumber). It might be possible to slightly pretty up leaving holes by having a secondary macro like MOOG_SKIP() or something, but still - kinda ugly.

Unless anyone else has objections, perhaps your first version was the right one: just rip the band aid off and remove everything (which triggers renumbering). If we want them gone, make them really be gone gone gone (since you're right: we already broke API and ABI).

Could you submit the ROMIO changes upstream? (or at least some of them -- e.g., they will still have to look for UB/LB, but some of the other straightforward changes could be accepted) I also noticed you didn't change the indenting of ROMIO code, presumably in order to make it potentially easier to merge down future versions of ROMIO. 👍

ompi/mpi/fortran/use-mpi-ignore-tkr/mpi-ignore-tkr-removed-interfaces.h.in

ompi/mpi/man/man3/MPI_Comm_create_errhandler.3in

ompi/mpi/fortran/use-mpi-tkr/mpi-f90-removed-interfaces.h

ompi/mpi/man/man3/MPI_Type_indexed.3in

jsquyres · 2019-05-01T22:21:42Z

ompi/mpiext/shortfloat/mpif-h/mpiext_shortfloat_mpifh.h.in

-        parameter (MPIX_CXX_SHORT_FLOAT_COMPLEX=76)
+        parameter (MPIX_SHORT_FLOAT=72)
+        parameter (MPIX_C_SHORT_FLOAT_COMPLEX=73)
+        parameter (MPIX_CXX_SHORT_FLOAT_COMPLEX=74)


It would be nice to avoid having to do these. These are scary -- i.e., are there any others hiding around the code base that we're unaware of?

Indeed, this kinda raises the question of how new datatypes should be added in MPI extensions (especially if multiple extensions can add datatypes -- they could conflict!). Not necessarily a problem for this PR to solve, but it's one that bears some thought...

ompi/datatype/ompi_datatype_internal.h

bosilca · 2019-05-01T22:49:38Z

ompi/include/mpi.h.in

@@ -630,13 +630,10 @@ enum {
  MPI_COMBINER_DUP,
  MPI_COMBINER_CONTIGUOUS,
  MPI_COMBINER_VECTOR,


If we associate values we can cope with the holes in the enum.

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

…PI 3.0) Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

…3.0) Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

- MPI_COMBINER_HVECTOR_INTEGER - MPI_COMBINER_HINDEXED_INTEGER - MPI_COMBINER_STRUCT_INTEGERo note this commit breaks ABI compatibility since a public enum is modified. Thanks Lisandro Dalcin for reporting this. Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

ggouaillardet · 2019-05-02T14:51:03Z

@jsquyres I updated the PR according to the reviews, and I am still doing the same renumbering.

jsquyres · 2019-05-03T14:06:15Z

I approved this PR because it is correct.

However, I think the "WIP" label is still good for now.

Specifically: I still think it's an open question as to whether we should actually do this for v5.0 or not. Per https://github.com/open-mpi/ompi/wiki/5.0.x-FeatureList#features-deferred-to-v50x-from-40x, we decided not do delete all the MPI-1/2 removed symbols for v5.0 -- see https://github.com/open-mpi/ompi/wiki/5.0.x-FeatureList#features-deferred-to-v50x-from-40x (yes, I know we already removed some from master, but I don't know if we want to compound the issue if we want to put them back for v5.0 and not actually delete them until v6.0).

AboorvaDevarajan · 2019-05-31T01:42:03Z

Can one of the admins verify this patch?

gpaulsen · 2019-07-08T19:34:54Z

We may not want to merge this into master now (for v5.0) since we're working to put this functionality back (see #6660).
Eventually we WILL want this functionality, so I'm not sure the best way to preserve this work.

ibm-ompi · 2020-02-06T08:55:26Z

The IBM CI (GNU/Scale) build failed! Please review the log, linked below.

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

ibm-ompi · 2020-03-23T14:54:50Z

The IBM CI (PGI) build failed! Please review the log, linked below.

Gist: https://gist.github.com/0c6f9d4dca582b72e9b8295997910cea

gpaulsen · 2020-05-04T19:49:25Z

@ggouaillardet @bosilca @jsquyres - This PR is quite old, and needs to be rebased at a minimum.

v5.0 is a major release in which to get a Backward Compatible break like this in. Is this PR a change we still want for v5.0?

jsquyres · 2020-05-05T12:32:56Z

@gpaulsen Please see my comment above where I said that we do not want this for v5.0, and should be targeted at v6.0: #6625 (comment)

hppritcha · 2025-02-06T17:53:26Z

what about this PR? do we want to revive it for 6.0.0?

jsquyres · 2025-02-06T18:18:57Z

I would imagine that this PR would need to change a bunch with respect to #12226, right?

ggouaillardet added Target: main 😳 Backward compat break labels Apr 30, 2019

ggouaillardet force-pushed the topic/remove_mpi30_removed_symbols branch from f104442 to 7c8f011 Compare April 30, 2019 04:15

ggouaillardet force-pushed the topic/remove_mpi30_removed_symbols branch from f39a3cf to e420edf Compare April 30, 2019 05:40

ggouaillardet added the ⚠️ WIP-DNM! label Apr 30, 2019

ggouaillardet force-pushed the topic/remove_mpi30_removed_symbols branch from e420edf to d1d8c4a Compare April 30, 2019 10:56

ggouaillardet removed the ⚠️ WIP-DNM! label Apr 30, 2019

ggouaillardet requested review from gpaulsen and jsquyres April 30, 2019 13:33

gpaulsen reviewed Apr 30, 2019

View reviewed changes

gpaulsen added the ⚠️ WIP-DNM! label May 1, 2019

jsquyres requested changes May 1, 2019

View reviewed changes

bosilca reviewed May 1, 2019

View reviewed changes

ggouaillardet added 9 commits May 2, 2019 23:21

fortran: remove unused files related to removed interfaces

6cf32ca

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

fortran: remove all references to MPI_Address (removed in MPI 3.0)

6d823b3

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

fortran: remove all references to MPI_Errhandler_create (removed in M…

06bf36d

…PI 3.0) Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

fortran: remove all references to MPI_Errhandler_get (removed in MPI …

88ebb37

…3.0) Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

fortran: remove all references to MPI_Errhandler_set (removed in MPI …

5eed846

…3.0) Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

fortran: remove all references to MPI_Type_extent (removed in MPI 3.0)

f1c8e16

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

fortran: remove all references to MPI_Type_hindexed (removed in MPI 3.0)

f0b7523

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

fortran: remove all references to MPI_Type_hvector (removed in MPI 3.0)

e0190b3

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

fortran: remove all references to MPI_Type_lb (removed in MPI 3.0)

a01054a

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

ggouaillardet added 4 commits May 2, 2019 23:37

fortran: remove all references to MPI_Type_struct (removed in MPI 3.0)

2a16bfa

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

fortran: remove all references to MPI_Type_ub (removed in MPI 3.0)

24b71ee

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

mpi: remove references to MPI_LB and MPI_UB (removed in MPI 3.0)

522ad12

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

ggouaillardet force-pushed the topic/remove_mpi30_removed_symbols branch from d1d8c4a to 522ad12 Compare May 2, 2019 14:44

jsquyres approved these changes May 3, 2019

View reviewed changes

gpaulsen added the Target: v6.0.x label May 5, 2020

gpaulsen added this to the v6.0.0 milestone May 5, 2020

Remove some more symbols removed in MPI 3.0 #6625

Are you sure you want to change the base?

Remove some more symbols removed in MPI 3.0 #6625

Uh oh!

Conversation

ggouaillardet commented Apr 30, 2019

Uh oh!

ibm-ompi commented Apr 30, 2019

Uh oh!

ibm-ompi commented Apr 30, 2019

Uh oh!

ibm-ompi commented Apr 30, 2019

Uh oh!

ibm-ompi commented Apr 30, 2019

Uh oh!

ibm-ompi commented Apr 30, 2019

Uh oh!

gpaulsen left a comment

Choose a reason for hiding this comment

Uh oh!

jsquyres commented Apr 30, 2019

Uh oh!

ggouaillardet commented May 1, 2019

Uh oh!

gpaulsen commented May 1, 2019

Uh oh!

jsquyres left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsquyres May 1, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bosilca May 1, 2019

Choose a reason for hiding this comment

Uh oh!

ggouaillardet commented May 2, 2019

Uh oh!

jsquyres commented May 3, 2019

Uh oh!

AboorvaDevarajan commented May 31, 2019

Uh oh!

gpaulsen commented Jul 8, 2019

Uh oh!

ibm-ompi commented Feb 6, 2020

Uh oh!

ibm-ompi commented Mar 23, 2020

Uh oh!

gpaulsen commented May 4, 2020

Uh oh!

jsquyres commented May 5, 2020

Uh oh!

hppritcha commented Feb 6, 2025

Uh oh!

jsquyres commented Feb 6, 2025

Uh oh!

Uh oh!