Deprecated DebugMode param in ADIOS2 init

[eisenhauer]

DebugMode has been deprecated (because it was not found to have an effect on performance) and has been removed from ADIOS2 internals, leaving all sanity checks active. However, this now-inoperable parameter still exists in the various language bindings, either in ADIOS object constructor or init() functions. For each binding we need to choose a strategy and timeline for transitioning them out (or decide to leave them if it's viewed as too disruptive).

[williamfgc]

May I suggest keeping it for future use at the savings of not breaking API for the ecosystem maintainers and users e.g. @ax3l and myself? This is a common performance feature introduced in databases and I found it very useful in the scientific workflows I worked in my past experience. While it might not be the bottleneck in current HPC write cases, it is common to avoid multiple checks in metadata extraction of complex workflows. That being said, I'd like to use adios2 and keep contributing for different I/O problems, but removing features and making the API unstable (as of v2.5.0) in every version makes it very difficult to justify its use for the long-term as a production ready technology, CC other stakeholders: @chuckatkins @pnorbert @sklasky @dpugmire @tkurc

[williamfgc]

Hope this helps the discussion: I ran some benchmarks (kudos to @NAThompson for introducing me to google-benchmark) for a proxy app that resembles data I/O patterns we are trying to tackle (code is below). Bottomline: Debug mode can squeeze 2-3% for write by just switching a bool flag, I still need to look at read. Squeezing every second is important for the app I'm working with, and this option in adios2 could help.

Running these on my local Mac. These are far from stress tests (1-5M vars-1-5M attributes). adios2 v2.5.0 debug mode = true:

Benchmark                       Time             CPU   Iterations
-----------------------------------------------------------------
BM_adios2_write/100000 31966208858 ns   31921618000 ns            1
BM_adios2_write/200000 66933369580 ns   66847440000 ns            1
BM_adios2_write/300000 99180746067 ns   98890918000 ns            1
BM_adios2_write/400000 144998962734 ns   143423604000 ns            1
BM_adios2_write/500000 192458612287 ns   187584078000 ns            1

adios2 v2.5.0 debug mode = false

Benchmark                       Time             CPU   Iterations
-----------------------------------------------------------------
BM_adios2_write/100000 30867953721 ns   30791826000 ns            1
BM_adios2_write/200000 64943908377 ns   64832113000 ns            1
BM_adios2_write/300000 96357937914 ns   96085171000 ns            1
BM_adios2_write/400000 142792746695 ns   140711116000 ns            1
BM_adios2_write/500000 187649799884 ns   183377241000 ns            1

code:

#include <adios2.h>
#include <array>
#include <iostream>
#include <stdexcept>
#include <string>

#include <benchmark/benchmark.h>

static void write(const std::size_t ndatasets) {
  try {
    constexpr std::size_t nfields = 10;
    adios2::ADIOS adios(false);

    adios2::IO ioWriter = adios.DeclareIO("writer");
    for (std::size_t d = 0; d < ndatasets; ++d) {
      const std::string dataset = "/dataset_" + std::to_string(d);

      for (std::size_t f = 0; f < nfields; ++f) {
        const std::string field =
            dataset + "/g1/g2/g3/field_" + std::to_string(f);
        auto var = ioWriter.DefineVariable<double>(
            field, adios2::Dims{1}, adios2::Dims{0}, adios2::Dims{1});
        ioWriter.DefineAttribute<std::string>(field + "/type", "field");
      }
    }

    const std::array<double, 3> values = {1, 2, 3};
    adios2::Engine engineWriter = ioWriter.Open("vars.bp", adios2::Mode::Write);
    for (std::size_t d = 0; d < ndatasets; ++d) {
      const std::string dataset = "/dataset_" + std::to_string(d);

      for (std::size_t f = 0; f < nfields; ++f) {
        const std::string field =
            dataset + "/g1/g2/g3/field_" + std::to_string(f);

        engineWriter.Put<double>(field, values.data());
      }
    }
    engineWriter.Close();

  } catch (std::exception& e) {
    std::cout << "Exception caught " << e.what() << "\n";
  }
}

static void BM_adios2_write(benchmark::State& state) {
  // Perform setup here
  for (auto _ : state) {
    // This code gets timed
    const std::size_t ndatasets = static_cast<std::size_t>(state.range(0));
    write(ndatasets);
  }
}
// Register the function as a benchmark
BENCHMARK(BM_adios2_write)->DenseRange(100000, 500000, 100000);
// Run the benchmark
BENCHMARK_MAIN();

Thanks.

[chuckatkins]

Bottomline: Debug mode can squeeze 2-3% for write by just switching a bool flag

It's somewhat of a false equivalency since you're measuring it as implemented which, when disabled has 1 comparison and a branch, and when enabled has 2 comparisons and 2 branches (for most checks). Eliminating debug mode would change the implementation such that there would always be only 1 comparison and 1 branch. Given that most of the debug mode checks are simple comparisons and not "expensive" content checks the result of eliminating debug mode entirely will likely be on par with the current implementation of debug mode disabled.

[williamfgc]

It's somewhat of a false equivalency since you're measuring it as implemented

@chuckatkins I'd assume any test that drove the decision would have gone that route. In any case, it's a fair concern so I ran more tests against master and got similar results (I doubt checking a cached bool is the bottleneck, but the source code is provided so anyone can try on different systems).

with current master:

Benchmark                       Time             CPU   Iterations
-----------------------------------------------------------------
BM_adios2_write/100000 31803454027 ns   31646887000 ns            1
BM_adios2_write/200000 65643598592 ns   65399678000 ns            1
BM_adios2_write/300000 98292367450 ns   97870724000 ns            1
BM_adios2_write/400000 144331387030 ns   144150890000 ns            1
BM_adios2_write/500000 191002015846 ns   186121562000 ns            1

My 2 cents is to combine the current master (the current clean up makes sense), but keep the debug mode option open for avoiding checks we might identify as bottlenecks after profiling for particular apps (I can provide those PRs for my particular app) + not break APIs by completely removing something that could be useful. This is part of my evaluation of adios2 as an I/O backend option for performance.

[eisenhauer]

Internal use of debugmode is off-topic here, so lets keep things on topic. This issue is on the question of how to adapt the init calls to a deprecated parameter. WRT internal use of debugmode, all conditionals were examined on a case-by-case, line-by-line basis over a period of weeks, with microbenchmark analysis of those that protected non-trivial code. You can get in touch with the dev team if you're curious about the details.

[germasch]

My 2 cents on this topic is that I don't think it's worth breaking the API over. It may be worth adding additional calls that don't take debugMode parameter where feasible. E.g., in the C++ API, debugMode already has a default value, so the constructor can already be used without a debug parameter.

In C, obviously that's not possible. I guess the situation there right now is a bit iffy anyway, since adios_init has a different signature depending on whether MPI is in use or not. I'm not sure if the adios_init_{serial,mpi} has been exposed for long, it may have just been added by @bradking. If it's only been available in the development version, maybe this is a chance to remove the debug mode paramter from it and declare it to be the API that one should switch to in the future, but compatibility for the. old adios_init could still be maintained. (Same for the versions that take a config file).

I haven't looked at the Fortran MPI in detail, but since F90 supports overloading, I suppose it should also be possible to declare a new interface without debug mode.

Basically, I'd say introducing a new, simpler signature would be good where it can be done, but I wouldn't break existing code for it. Most compilers support some kind of deprecated attribute that could be used to warn users that they should switch to a new API if you decide that the old ones should go away at some point. I'm not sure I'd do that, though, maintaining support for the existing signature should be pretty trivial. If there's some major rework of the API in the future for some other reason, that'd of course be an opportunity to include this cleanup, too.

[bradking]

I'm not sure if the adios2_init_{serial,mpi} has been exposed for long

Those were added in Jan 2020 by #1946 and AFAIK have not been in a release. I think we can remove the debug_mode argument from them.

EDIT: Note however that plain adios2_init and adios2_init_config are currently provided by #defines that map them to adios2_init{,_config}_{serial,mpi} based on whether MPI interfaces are enabled.

[NAThompson]

Now that DebugMode is deprecated, what should become of the empty constructor?

The documentation (which appears slightly broken, btw) still says

// Do not use () for empty constructor.

Is this advice still relevant? It appears that () is now the only constructor if you don't provide a config file.

[bradking]

#2132 removes the debug_mode argument from the never-released adios2_init{,_config}_{serial,mpi} constructor functions.

[ax3l]

Hm, we currently provide the argument (always as false for users) but I personally did not play around with it. Due to that I cannot weight in on that, sorry. @franzpoeschel @guj did you explore this?

Either way, is there a user-facing ADIOS2_VERSION_GREATER-kind of c-style macro we can use to anticipate breaking API changes? (Not that I enjoy API breakage ;-) but if you do some breakage in 2.6.0 I am fine with it, because we will upgrade for complex and extended floating point support anyway. )

[sklasky]

Hi, I am very concerned that people think we are changing the user-facing APIs, but in all of my conversations with Norbert, this is not happening. I think that after several years we might tweak the APIs, but I haven’t heard we are changing them…

There are internal changes which IMHO is different. These will occur to help make ADIOS a more maintainable code.

There are potential performance challenges as we keep moving forward, and one of our top priorities is more testing for reading, writing, and staging. The better we test, the more we know. This is critical for us… OK… sounds like COVID-19 testing, but I feel that this is the most critical thing for us.

Scott

From: Axel Huebl [email protected] Sent: Monday, April 13, 2020 2:39 PM To: ornladios/ADIOS2 [email protected] Cc: Klasky, Scott A. [email protected]; Mention [email protected] Subject: [EXTERNAL] Re: [ornladios/ADIOS2] Deprecated DebugMode param in ADIOS2 init (#2127)

Hm, we currently provide the argumenthttps://github.com/openPMD/openPMD-api/blob/0.11.1-alpha/src/IO/ADIOS/ADIOS2IOHandler.cpp#L68 (always as false for users) but I personally did not play around with it. Due to that I cannot weight in on that, sorry. @franzpoeschelhttps://github.com/franzpoeschel did you explore this?

Either way, is there a user-facing ADIOS2_VERSION_GREATER-kind of c-style macro we can use to anticipate breaking API changes? (Not that I enjoy API breakage ;-) but if you do some breakage in 2.6.0 I am fine with it, because we will upgrade for complex and extended floating point support anyway. )

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/ornladios/ADIOS2/issues/2127#issuecomment-613033644, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADGPYBZKEIJAV44KXALLU73RMNL35ANCNFSM4MFYPM3Q.

[pnorbert]

We are not removing the ADIOS() signatures with the debugMode flag, not even the adios2::DebugON constant, so apps should compile without any changes.

[NAThompson]

@pnorbert : What's your opinion on the C++14 deprecated tag. Is there any hope for compiler support on the systems we need to run on?

[franzpoeschel]

Hm, we currently provide the argument (always as false for users) but I personally did not play around with it. Due to that I cannot weight in on that, sorry. @franzpoeschel @guj did you explore this?

Either way, is there a user-facing ADIOS2_VERSION_GREATER-kind of c-style macro we can use to anticipate breaking API changes? (Not that I enjoy API breakage ;-) but if you do some breakage in 2.6.0 I am fine with it, because we will upgrade for complex and extended floating point support anyway. )

I think the question has resolved by now, given that API-breaking changes are not planned? @ax3l FWIW, setting ADIOS2_DEBUG_MODE = true was very helpful during development of the ADIOS2 backend while it still had an effect and I only set it to false afterwards for release. Think we can leave it at that for now?

[ax3l]

Thanks for your feedback.

FWIW, setting ADIOS2_DEBUG_MODE = true was very helpful during development of the ADIOS2 backend

That actually sounds to me like some version of this functionality should not be deprecated but actually be encouraged for everyone developing against the ADIOS2 API.

Think we can leave it at that for now?

Yes, in openPMD-api we leave it false in production. But good to know this helps when debugging one's own calls against ADIOS2 (to find downstream usage issues faster).

[germasch]

@ax3l, what has happened here is that the API still takes the debug mode argument to keep things compatible, but it doesn't have any effect anymore -- ADIOS2 does the sanity checks always now, which were previously only done when debug mode was on. As I understand it, the performance impact of always having those checks was found to be negligible by careful benchmarking by @eisenhauer.

[eisenhauer]

That's pretty much the size of it... Several PRs involved, but #2055 and #2065 are illustrative.

[ax3l]

Oh cool, thanks for the clarification!

[eisenhauer]

I'm going to re-open this issue because the presence of the unused debugMode flag in the C++ bindings does still cause problems. For example, we have the following constructors for ADIOS (among others):

1. ADIOS(const std::string &configFile, const bool debugMode = true);
   

2. ADIOS(const char *configFile);
   

3. ADIOS(const bool debugMode = true);
   

If you do an ADIOS constructor with just a single string parameter to specify an XML configFile, which one gets called? That's right, it's number 3. That is, the compiler converts the string parameter to a boolean, uses it as the debugMode parameter and the XML init file you specified gets ignored. (And yes, I worked with someone experienced for a couple of hours before we figured out that this was one of the problems we were having. If you want independent confirmation, look at this https://www.cppstories.com/2019/07/surprising-conversions-char-bool/.) Arguably this is more a C++ stupidity than an ADIOS problem, but it's a C++ stupidity that our users are more exposed to than they have to be because we still have a deprecated parameter in our API... I personally think that this is worth changing the API for in some new release, because this can be very confusing at the point of new code development, when we are acquiring new users. Yes, old code will have to delete the unused parameter when they transition to a new version, but I don't think that's going to make anyone stop using ADIOS. I also like @ax3l 's idea for an ADIOS version macro so that code can easily adapt to the slightly different APIs.

[JasonRuonanWang]

@eisenhauer Then let's do it now and this will be solved in 2.9.0. I don't like the idea of keeping stupidities either just for the sake of not breaking users' code. It will cause more problems to users' code. If I remember, when this stupidity was introduced, it broke a lot of our users' code. So our job should be preventing such stupidity from being introduced again, rather than preventing them from being fixed.

[ax3l]

x-ref: https://github.com/openPMD/openPMD-api/pull/1269

[ax3l]

@JasonRuonanWang yes, but please consider your wording. APIs are introduced by humans in a stringent time budget. Yes, let's fix it now. Thanks.

[eisenhauer]

ADIOS2 debugmode has been long deprecated, closing this issue.