Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove DebugMode from all external APIs #3204

Merged
merged 4 commits into from
Jun 22, 2022

Conversation

eisenhauer
Copy link
Member

No description provided.

@eisenhauer
Copy link
Member Author

Not yet sorted, for example, ADIOS2-Examples or done anything to enable building with old and new APIs.

Copy link
Contributor

@chuckatkins chuckatkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

I approve in concept. Obviously the implementation has to work, but yeah. 👍

Copy link
Member

@JasonRuonanWang JasonRuonanWang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. This will make things much easier.

@eisenhauer
Copy link
Member Author

So, two further challenges assuming we go forward with this:

  1. To ease the transition we need a way for applications to compile with new and old versions of the API. I'd propose adding the following to ADIOSConfig.h
    #define ADIOS2_VERSION (ADIOS2_VERSION_MAJOR* 10000 + ADIOS2_VERSION_MINOR * 100 + ADIOS2_VERSION_PATCH)

And then code that want to compile with all versions can test as follows (for whatever version we make the transition on )
/* Test for ADIOS2 > 2.8.1 */
#if ADIOS2_VERSION > 20801
// use adios_init without debug flag
#else
// specify deprecated debug flag
#endif

  1. we have to sort, or something else, for contract testing. At this point I haven't dug into quite how the contract testing works. @chuckatkins, possible to weigh in on how to best approach this between now and when this is included in a specific release?

@eisenhauer
Copy link
Member Author

Tied to: ornladios/ADIOS2-Examples#55

@eisenhauer
Copy link
Member Author

The actual appropriate conditional here, assuming that this becomes official with release 2.9.0 is:
#if (defined(ADIOS2_VERSION) && (ADIOS2_VERSION >= 20900)) || defined(ADIOS2_DEBUGMODE_REMOVED)
The "defined(ADIOS2_DEBUGMODE_REMOVED)" piece will catch the situation where the application is compiled against master before 2.9.0 is released (and the version macros get updated).

I think the only question is when we want to merge this. Because once we do, it might be hard, for example, to spin release 2.8.2 without having this in it. That would mean that we have an API change on a non-major release. But if we think we'll do 2.9.0 before minor releases (or are willing to spin 2.8.2 off 2.8.1 (rather than master) with only the update to fix whatever bug we're trying to fix), then I'd say lets get this merged...

@chuckatkins
Copy link
Contributor

My attempt at keeping the debug mode api as an empty shim but explicitly marked as deprecated, at least for C and C++.
chuckatkins@9434a36

Note the use of enable_if in the C++ bindings to prevent the char * to bool conversion

@eisenhauer
Copy link
Member Author

So, I like things like the enable_if, but I assume that if this gets to the point where the ugly #ifdef's aren't necessary, then the contract tests will pass?

@chuckatkins chuckatkins force-pushed the DebugMode branch 5 times, most recently from 6d1fb9f to c7554b8 Compare June 21, 2022 05:07
@chuckatkins
Copy link
Contributor

@eisenhauer it took a few iterations to get it right, but the contract tests pass now and you can see the deprecation warnings when they build.

@chuckatkins
Copy link
Contributor

I'm working out how to effectively do the same for the fortran interface

@chuckatkins
Copy link
Contributor

Not many Fortran compilers support the deprecated attribute, at the moment all I can find is gfortran >= 11. That being said, it does work as expected:

call adios2_init(adios, MPI_COMM_WORLD, adios2_debug_mode_on, ierr)

produces:

   38 |     call adios2_init(adios, MPI_COMM_WORLD, adios2_debug_mode_on, ierr)
      |                                                                       1
Warning: Using subroutine ‘adios2_init_debug_mpi’ at (1) is deprecated [-Wdeprecated-declarations]

@eisenhauer
Copy link
Member Author

So, let me summarize my understanding to see if I've got it right from the application point of view. For C++, we introduce the new (no debug-mode) API, but leave the old API in place, simply marking it deprecated so that they get warnings. That seems straightforward and a good way to go. Applications as they stand can compile with old and new ADIOS, when they delete the Debugmode parameter in their init calls, that doesn't change (though with old ADIOS they might not get the init call that they counted on, as per prior problems). Eventually we'll kill the deprecated APIs and all will be well for apps who have transitioned.

I'm a little fuzzier on the C interface though. The init interfaces with a debugmode parameter (all of them?) have been marked deprecated. Do applications have a path for transitioning to an interface without a debugmode parameter? For creating code that would compile with both old and new versions of the API? I guess fortran is just an uglier version of the C question, with only gfortran supporting the deprecated warning. Anything to be done there?

@chuckatkins
Copy link
Contributor

So, let me summarize my understanding to see if I've got it right from the application point of view. For C++, we introduce the new (no debug-mode) API, but leave the old API in place, simply marking it deprecated so that they get warnings. That seems straightforward and a good way to go. Applications as they stand can compile with old and new ADIOS, when they delete the Debugmode parameter in their init calls, that doesn't change ... Eventually we'll kill the deprecated APIs and all will be well for apps who have transitioned.

Correct

(though with old ADIOS they might not get the init call that they counted on, as per prior problems)

The enable_if additions in the C++ api actually fix the long standing problem of unintended char* to bool conversion and would be a change we should make even if keeping the "Debug Mode" APIs. This also removes the need for the additional const char* constructors which were a not-so-great attempt to work around the issue. The correct std::string constructors will now get used as originally intended.

I'm a little fuzzier on the C interface though. The init interfaces with a debugmode parameter (all of them?) have been marked deprecated. Do applications have a path for transitioning to an interface without a debugmode parameter? For creating code that would compile with both old and new versions of the API?

The C interface maps the existing adios2_init(bool) and adios2_init(comm, bool) calls to the deprecated adios2_init_serial_debug(bool) and adios2_init_mpi_debug(comm, bool) respectively. The transition path is for applications to migrate to adios2_init_serial() and adios2_init_mpi(comm) which works in the existing implementation, with the changes in this pr, and in a future release with the debug apis removed entirely. The only caveat here is that there can't be a serial/mpi unspecified adios2_init that maintains compatibility with both the debug and non-debug apis.

I guess fortran is just an uglier version of the C question, with only gfortran supporting the deprecated warning. Anything to be done there?

The Fortran situation is quite a bit better than C since Fortran 90 allows for function overloading based on arguments and essentially works the same way as the C++ interface in that applications can continue to use adios2_init for old and new. We'll probably just want to make sure we communicate to the known Fortran users we have as much as we can that the debug mode API is on it's way out and they should adjust accordingly as it's less likely they'll get the appropriate compiler warning, unless of course using gcc11.

@eisenhauer
Copy link
Member Author

So, let me summarize my understanding to see if I've got it right from the application point of view. For C++, we introduce the new (no debug-mode) API, but leave the old API in place, simply marking it deprecated so that they get warnings. That seems straightforward and a good way to go. Applications as they stand can compile with old and new ADIOS, when they delete the Debugmode parameter in their init calls, that doesn't change ... Eventually we'll kill the deprecated APIs and all will be well for apps who have transitioned.

Correct

(though with old ADIOS they might not get the init call that they counted on, as per prior problems)

The enable_if additions in the C++ api actually fix the long standing problem of unintended char* to bool conversion and would be a change we should make even if keeping the "Debug Mode" APIs. This also removes the need for the additional const char* constructors which were a not-so-great attempt to work around the issue. The correct std::string constructors will now get used as originally intended.

I'm a little fuzzier on the C interface though. The init interfaces with a debugmode parameter (all of them?) have been marked deprecated. Do applications have a path for transitioning to an interface without a debugmode parameter? For creating code that would compile with both old and new versions of the API?

The C interface maps the existing adios2_init(bool) and adios2_init(comm, bool) calls to the deprecated adios2_init_serial_debug(bool) and adios2_init_mpi_debug(comm, bool) respectively. The transition path is for applications to migrate to adios2_init_serial() and adios2_init_mpi(comm) which works in the existing implementation, with the changes in this pr, and in a future release with the debug apis removed entirely. The only caveat here is that there can't be a serial/mpi unspecified adios2_init that maintains compatibility with both the debug and non-debug apis.

I guess fortran is just an uglier version of the C question, with only gfortran supporting the deprecated warning. Anything to be done there?

The Fortran situation is quite a bit better than C since Fortran 90 allows for function overloading based on arguments and essentially works the same way as the C++ interface in that applications can continue to use adios2_init for old and new. We'll probably just want to make sure we communicate to the known Fortran users we have as much as we can that the debug mode API is on it's way out and they should adjust accordingly as it's less likely they'll get the appropriate compiler warning, unless of course using gcc11.

Ah, no idea Fortran had function overloading. Then again, 32 years is about how long it's been since I wrote much in the way of fortran...

OK, so C++ is reasonable. C and some fortran users will get deprecated warnings until they change their code to call different routines, which they can start doing at any time and we should encourage that. I think that works for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants