EngineType consistency #1266

germasch · 2019-03-08T02:41:38Z

In PR #1264, I've added a couple of tests to test CXX11/C/Fortran binding behavior when it comes to creating (IO::Open) an engine. All these test pass, ie., they document current behavior.

C++

The code definitely looks nicest in C++:

TEST_F(ADIOS2_CXX11_API_IO, Engine)
{
    io.SetEngine("bpfile");
    EXPECT_EQ(io.EngineType(), "bpfile");

    adios2::Engine engine = io.Open("types.bp", adios2::Mode::Write);
    EXPECT_EQ(engine.Name(), "types.bp");
    EXPECT_EQ(engine.Type(), "BP3");

    EXPECT_EQ(io.EngineType(), "bp"); // FIXME? Is it expected that adios2_open
                                      // changes the engine_type string?
}

This works mostly as expected, though I think it's iffy to have the engine show up as three different strings ("bpfile", "BP3", "bp"). Having the result of of io.EngineType() change as a consequence of calling Open is also not something I'd expect.

C

In the C API, things happen about the same way, so that's good:

TEST_F(ADIOS2_C_API_IO, Engine)
{
    int ierr;

    ierr = adios2_set_engine(ioH, "bpfile");
    EXPECT_EQ(ierr, 0);

    std::string engine_type = testing::adios2_engine_type_as_string(ioH);
    EXPECT_EQ(engine_type, "bpfile");

    adios2_engine *engineH = adios2_open(ioH, "ctypes.bp", adios2_mode_write);

    // FIXME, I'd like to check that the engine type itself is correct, but
    // there's no API to get it
    std::string engine_name = testing::adios2_engine_name_as_string(engineH);
    EXPECT_EQ(engine_name, "ctypes.bp");

    engine_type = testing::adios2_engine_type_as_string(ioH);
    EXPECT_EQ(engine_type, "bp"); // FIXME? Is it expected that adios2_open
                                  // changes the engine_type string?
}

(Note that the actual C API for getting strings looks much worse -- I hid it in helper functions that return std::string for making testing simpler.) The C API is missing a function that allows one to find the type of a given adios2_engine, but if it existed, it'd return "BP3" like cxx11.

Fortran

Fortran also doesn't have an API function to get the type for a given engine (because that'd need a corresponding C API function). It does, however, have engine%type, though I'm not sure that's supposed to be part of the official API.

  call adios2_set_engine(io, "bpfile", ierr)

  call adios2_io_engine_type(engine_type, io, ierr)
  if (engine_type /= "bpfile") stop "FAIL adios2_io_engine_type: "//engine_type
  deallocate(engine_type)

  call adios2_open(engine, io, "ftypes.bp", adios2_mode_write, ierr)

  if (engine%type /= "bpfile") stop "FAIL engine%type: "//engine%type
  ! // FIXME, I'd like to check that the engine type itself is correct, but
  ! // there's no (function-style) API to get it
  ! // FIXME, I'd like to check the engine's name, but there's no API to get it

  call adios2_io_engine_type(engine_type, io, ierr)
  if (engine_type /= "bp") stop "FAIL adios2_io_engine_type: "//engine_type
  deallocate(engine_type)

The adios2_set_engine and adios2_io_engine_type functions behave the same as their C/CXX11 counterparts. The engine%type, however, is different, returning "bpfile" instead of "BP3".

So I can see that there may be the argument "bp", "bpfile", "BP3" all mean the same thing, so why should we even care about some sort of consistency. I don't disagree that it's okay to accept various aliases for a given engine. I do, however, think, it'd be good if ADIOS2 were consistent in always returning the official name ("BP3" in this case). I also really think that the various APIs should mirror each other as much as possible, ie., having functions missing from the C/Fortran API isn't really good, and having them behave subtly differently is worse, as that'll make long term maintenance unnecessarily hard.

I have a proposed solution for this, but that's only worthwhile if there's agreement that this should be improved.

The text was updated successfully, but these errors were encountered:

williamfgc · 2019-03-08T13:35:38Z

@germasch there is a reason for this, bpfile is a wildcard that points to the latest default (today BP3, BP4 in future releases), while bp3 is explicitly requesting the BP3 engine regardless of version. The same with SST (future SST-1 SST-2). Nevertheless, I see a bug in updating engine%type dynamically from the C++ library internals. engine%type is the supported way, introducing another public function doesn't add any value (nothing wrong with Fortran types, BTW). The returned type is the actual engine type that adios2 recognizes.

I also really think that the various APIs should mirror each other as much as possible, ie., having functions missing from the C/Fortran API isn't really good and having them behave subtly differently is worse, as that'll make long term maintenance unnecessarily hard.

This has been asked several times (C99 and Fortran 90 provide minimal low-level capabilities compared to 21st century language standards):
https://adios2.readthedocs.io/en/latest/faq/faq.html#why-are-c-and-fortran-apis-missing-functionality

germasch · 2019-03-08T15:19:23Z

So let's start with the point we agree on: engine%type needs to be updated from core. To support this even more, consider this:

  call adios2_set_engine(io, "", ierr) ! means default 
  call adios2_open(engine, io, "ftypes.bp", adios2_mode_write, ierr)
  if (engine%type /= "") stop "FAIL engine%type"

Right now, there is no function in the C API that would allow the Fortran API to set the engine%type to its actual value. There is adios2_engine_type, which however takes an adios2_io, not an adios2_engine, and hence doesn't do what's needed.

You rejected #1257, which would have been a basis to introduce an adios2_engine_type that would what's needed here. Yes, that's breaking an API. I'm quite sure the very reason that C API actually exists, that is, to enable the Fortran bindings to set engine%type, only that it turns out that wasn't actually interfacing the right function, and so I'm close to 100% positive that there isn't any actual user of that function out there that would be affected by breaking the API (and if there were, they'd get a compiler error after the change, so it's not hard-to-debug silent corruption).

This really comes down to a fundamental question, which is, do you want to get the APIs right at this point, or do you want to drag along broken or inconsistent legacy APIs for the next 5 years, which seemed to be one of the motivations to move from adios1 to adios2 in the first place?

There is no question that a well-established library with many users should never break an existing API in an incremental patch release. But realistically, this isn't where ADIOS2 is at. I know it's hard to take advice from someone you don't really know or trust. But I'll suggest that the most important thing to get right for ADIOS2 right now is to get its APIs right, including the various language bindings. You can always fix bugs, add features and rework internals later. Changing APIs now might be (a little) painful, but it only gets worse with time, and otherwise the only alternative is to drag along and support poor choices for many years.

So that's kind of my general rant. On the particular issue the choices I see are:

do nothing (ie, keep enginge%type broken)
introduce a new C API adios2_real_engine_type() and use that to fix the Fortran bindings.
rework the existing C API adios2_engine_type to do what's needed, which is an incompatible (but obvious) API change, with likely 0 users affected. Either introduce a adios2_io_engine_type or something like that to provide the original functionality, or just drop it.

(As far as I'm concerned, I would just drop the original IO::EngineType from all bindings, as it has very limited usefulness, and as can be seen above, potential for confusion. If anyone were to complain, it's easy to add it back. Fewer APIs -> less stuff to support)

williamfgc · 2019-03-08T15:45:48Z

Right now, there is no function in the C API that would allow the Fortran API to set the engine%type to its actual value.

Is not needed, engine types are set by IO and don't change along the scope of an engine. Updating the name with the internal name at Open is sufficient. Thanks for catching that bug.

germasch · 2019-03-08T16:04:32Z

Right now, there is no function in the C API that would allow the Fortran API to set the engine%type to its actual value.

Is not needed, engine types are set by IO and don't change along the scope of an engine. Updating the name with the internal name at Open is sufficient. Thanks for catching that bug.

How are you going to update the name with the internal name at Open if there is no (C) API function to get the internal name? (When you say "name", you're talking about the type string, right?)

williamfgc · 2019-03-08T16:09:37Z

@germasch There is a C function and it does what is needed. Yes, I meant "internal type", my bad.

germasch · 2019-03-08T16:26:13Z

Can you tell me which C function that is? If I had found one, I'd have used it to fix the bug we're talking about.

williamfgc · 2019-03-08T16:32:03Z

@germasch you mentioned above adios2_engine_type, it takes the io handler calling Open and the resulting type can be passed to engine%type. The question is: do you want to do it? I can always do it.

pnorbert · 2019-03-08T16:46:38Z

Please don’t remove the function from the IO object. The tutorial examples are using it to print out which engine we are using BEFORE attempting the Open() call. This helps us to see that we actually run the pipelines the way we intended. The frequent changes of engines during a live demo/handson leads to annoying hangs and we need to see immediately where we messed up. I don’t know other use case where we use this info before Open. If there was a function that sets the unique name right in SetEngine() so we get back the unique name, it would be even better.

…

On Fri, Mar 8, 2019 at 4:19 PM Kai Germaschewski ***@***.***> wrote: So let's start with the point we agree on: engine%type needs to be updated from core. To support this even more, consider this: call adios2_set_engine(io, "", ierr) ! means default call adios2_open(engine, io, "ftypes.bp", adios2_mode_write, ierr) if (engine%type /= "") stop "FAIL engine%type" Right now, there is no function in the C API that would allow the Fortran API to set the engine%type to its actual value. There is adios2_engine_type, which however takes an adios2_io, not an adios2_engine, and hence doesn't do what's needed. You rejected #1257 <#1257>, which would have been a basis to introduce an adios2_engine_type that would what's needed here. Yes, that's breaking an API. I'm quite sure the very reason that C API actually exists, that is, to enable the Fortran bindings to set engine%type, only that it turns out that wasn't actually interfacing the right function, and so I'm close to 100% positive that there isn't any actual user of that function out there that would be affected by breaking the API (and if there were, they'd get a compiler error after the change, so it's not hard-to-debug silent corruption). This really comes down to a fundamental question, which is, do you want to get the APIs right at this point, or do you want to drag along broken or inconsistent legacy APIs for the next 5 years, which seemed to be one of the motivations to move from adios1 to adios2 in the first place? There is no question that a well-established library with many users should never break an existing API in an incremental patch release. But realistically, this isn't where ADIOS2 is at. I know it's hard to take advice from someone you don't really know or trust. But I'll suggest that the most important thing to get right for ADIOS2 right now is to get its APIs right, including the various language bindings. You can always fix bugs, add features and rework internals later. Changing APIs now might be (a little) painful, but it only gets worse with time, and otherwise the only alternative is to drag along and support poor choices for many years. So that's kind of my general rant. On the particular issue the choices I see are: - do nothing (ie, keep enginge%type broken) - introduce a new C API adios2_real_engine_type() and use that to fix the Fortran bindings. - rework the existing C API adios2_engine_type to do what's needed, which is an incompatible (but obvious) API change, with likely 0 users affected. Either introduce a adios2_io_engine_type or something like that to provide the original functionality, or just drop it. (As far as I'm concerned, I would just drop the original IO::EngineType from all bindings, as it has very limited usefulness, and as can be seen above, potential for confusion. If anyone were to complain, it's easy to add it back. Fewer APIs -> less stuff to support) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1266 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADGMLUh-1j386FshwswmE0AVrHJIUf5xks5vUn97gaJpZM4bkhzl> .

germasch · 2019-03-08T16:49:06Z

@williamfgc, but that function doesn't give you the actual type of the engine, it gives you the IO::m_EngineType member, which is the string that you used in a previous IO::setEngine call. The actual engine type will be "BP3" (in this example), you'll never get that from the current adios2_engine_type.

Actually, I'm wrong: there is a way to get '"BP3"' from adios2_engine_type, ie, if you don't call adios2_set_engine because the default is "BP3". In that case (or if you explicitly set it to "BP3", os you'll get "BP3" if you call it before IO::Open. After you call IO::Open, you'll get "bp".

I mean, yes, it'll be possible to mess around with core/IO.cpp and the Fortran bindings to get "BP3" out of it in the end. But doesn't this discussion show how very intricate the current way is, which would all go away if this was done the "right" way, ie., have the Fortran and C interfaces provide the same Engine::Type() function that CXX11 has?

germasch · 2019-03-08T16:54:44Z

Please don’t remove the function from the IO object. The tutorial examples are using it to print out which engine we are using BEFORE attempting the Open() call. This helps us to see that we actually run the pipelines the way we intended. The frequent changes of engines during a live demo/handson leads to annoying hangs and we need to see immediately where we messed up. I don’t know other use case where we use this info before Open.

I have no issues with keeping the EngineType() function -- in fact, I forgot that it may be set through the .xml file, so being able to check it does can indeed be useful. So forget about that idea, I retract it.

If there was a function that sets the unique name right in SetEngine() so we get back the unique name, it would be even better.

Actually, that was going to be part of my proposed changes, but I didn't even write that yet in my initial post, since the default reaction I seem to be getting here is "it's good as-is".

williamfgc · 2019-03-08T16:54:46Z

@germasch let's close this. From your feedback (and others) the adios2 APIs are in pretty good stable shape. Are there missing functions in the C APIs, absolutely, but as you can see they are at the bottom of our list (majority of applications are Fortran and C++). The only API conflict reported so far is the adios2_engine_type in the C bindings. It has 2 very good reasons:

Requested from WDM work on code coupling, getting the engine_type from IO is important as it non-const, getting it from Engine not so much.
C APIs will always be limited by C not allowing function overloads. We pick the actually used case.

API translation between high-level and low-level languages will always be cumbersome and users tend to pick based on their needs (MPI C++ bindings were discontinued).

williamfgc · 2019-03-08T16:58:04Z

@germasch in any case, adding something like adios2_engine_get_type passing an engine in the C bindings will do the trick. Again, not having 1:1 APIs (in C only) is due to the function overload limitation of the good-old low-level C. Fortran is awesome is that sense as it allows functions overloads (but is a pain to dump all the boilerplate for it).

williamfgc · 2019-03-08T16:58:45Z

@pnorbert you have my word I won't break the APIs, having it in IO was actually a request from Eric doing WDM related work. See my proposed solution above.

germasch · 2019-03-08T18:22:29Z

@williamfgc @pnorbert So If I do this, do you consider that acceptable (I guess you can choose a subset)?

add a C API function called adios2_engine_get_type
use that function to set engine%type consistently in the Fortran API
Change core::SetEngine to do the translation from requested type (alias) to actual type earlier, so that you get the following semantics:

Mapping of alias happens when you call core::SetEngine, so a subsequent EngineType() will return the actual type.

The behavior would like like this

TEST_F(ADIOS2_CXX11_API_IO, Engine)
{
    io.SetEngine("bpfile");
    EXPECT_EQ(io.EngineType(), "BP3");

    adios2::Engine engine = io.Open("types.bp", adios2::Mode::Write);
    EXPECT_EQ(engine.Name(), "types.bp");
    EXPECT_EQ(engine.Type(), "BP3");

    EXPECT_EQ(io.EngineType(), "BP3");
}

(Note that the default is also "BP3" right now, so the if removing the SetEngine call, or replacing it with SetEngine("") (which means default) would give the same behavior.

Some readers/writers don't share a common name. For example, there is "InlineReader" and "InlineWriter" which will be returned from engine.Type(), respectively. It's not possible to know whether the Engine will be opened for writing/reading at SetEngine time, so the returned type from IO would just be "Inline".

williamfgc added the question label Mar 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EngineType consistency #1266

EngineType consistency #1266

germasch commented Mar 8, 2019

williamfgc commented Mar 8, 2019 •

edited

Loading

germasch commented Mar 8, 2019

williamfgc commented Mar 8, 2019

germasch commented Mar 8, 2019

williamfgc commented Mar 8, 2019

germasch commented Mar 8, 2019

williamfgc commented Mar 8, 2019 •

edited

Loading

pnorbert commented Mar 8, 2019 via email

germasch commented Mar 8, 2019

germasch commented Mar 8, 2019

williamfgc commented Mar 8, 2019

williamfgc commented Mar 8, 2019 •

edited

Loading

williamfgc commented Mar 8, 2019 •

edited

Loading

germasch commented Mar 8, 2019

EngineType consistency #1266

EngineType consistency #1266

Comments

germasch commented Mar 8, 2019

C++

C

Fortran

williamfgc commented Mar 8, 2019 • edited Loading

germasch commented Mar 8, 2019

williamfgc commented Mar 8, 2019

germasch commented Mar 8, 2019

williamfgc commented Mar 8, 2019

germasch commented Mar 8, 2019

williamfgc commented Mar 8, 2019 • edited Loading

pnorbert commented Mar 8, 2019 via email

germasch commented Mar 8, 2019

germasch commented Mar 8, 2019

williamfgc commented Mar 8, 2019

williamfgc commented Mar 8, 2019 • edited Loading

williamfgc commented Mar 8, 2019 • edited Loading

germasch commented Mar 8, 2019

williamfgc commented Mar 8, 2019 •

edited

Loading

williamfgc commented Mar 8, 2019 •

edited

Loading

williamfgc commented Mar 8, 2019 •

edited

Loading

williamfgc commented Mar 8, 2019 •

edited

Loading