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

Engine type changed in repeated output #3720

Closed
liangwang0734 opened this issue Jul 27, 2023 · 13 comments · Fixed by #3955
Closed

Engine type changed in repeated output #3720

liangwang0734 opened this issue Jul 27, 2023 · 13 comments · Fixed by #3955
Assignees

Comments

@liangwang0734
Copy link

liangwang0734 commented Jul 27, 2023

Main code, repeat_io.cxx:

#include <cstdio>
#include <string>
#include <mpi.h>
#include <adios2.h>

int main(int argc, char **argv)
{
  MPI_Init(&argc, &argv);

  adios2::ADIOS ad_("adios2cfg.xml", MPI_COMM_WORLD, adios2::DebugON);
  adios2::IO io_;
  adios2::Engine engine_;
  
  std::string io_name_ = "checkpoint";
  for (int c=0; c<3; c++) {
    io_ = ad_.DeclareIO(io_name_);
    std::string filename = "test.bp";
    engine_ = io_.Open(filename, adios2::Mode::Write);
    printf("+++ c=%d, type=%s\n", c, engine_.Type().c_str());
    engine_.Close();
    ad_.RemoveIO(io_name_);
  }

  MPI_Finalize();

  return 0;
}

adios2cfg.xml:

<?xml version="1.0"?>
<adios-config>
    <io name="checkpoint">
        <engine type="BP5">
            <parameter key="SubStreams" value="16"/>
        </engine>
    </io>
</adios-config>

Results (only the first iteration uses BP5; using adios2 release_28):

+++ c=0, type=BP5Writer
+++ c=1, type=BP4Writer
+++ c=2, type=BP4Writer

Question: How to make the subsequent IO use BP5?
One option is to move the declaration and removal of io_ out of the loop, but that requires some refactoring of our production code, so I prefer to avoid that.

@liangwang0734 liangwang0734 changed the title Engine type change in repeated output Engine type changed in repeated output Jul 27, 2023
@pnorbert
Copy link
Contributor

pnorbert commented Jul 28, 2023 via email

@liangwang0734
Copy link
Author

@pnorbert Yes, moving the IO creation out of the loop would solve the problem in this example. But for now, I prefer not to do this in our user code. Can we set the engine type manually at or after IO::Open?

@liangwang0734
Copy link
Author

I now use IO::SetEngine to manually change the engine type.

If there is no plan/interest to fix this, I will close this issue.

@pnorbert
Copy link
Contributor

pnorbert commented Jul 28, 2023 via email

@pnorbert
Copy link
Contributor

pnorbert commented Jul 28, 2023 via email

@germasch
Copy link
Contributor

FWIW, this problem happens in master, too, though slightly differently in that if one sets "bp4" in the config file, it'll be bp4 the first time around, and then switch to bp5 in subsequent uses.

The reason is that the IO is created when the config file is read (when the ADIOS2 object is created), and the engine type is set correctly from the config file. But if one removes the IO and makes a new one, that one is created from scratch, unaware of the existing config for it, and so it defaults to a m_EngineType of "File", which turns into into BP5. I would consider it a bug, too, but I don't think there's an easy fix.

@spyridon97 spyridon97 self-assigned this Nov 8, 2023
@spyridon97
Copy link
Contributor

spyridon97 commented Dec 5, 2023

State of this issue:

In master if you run the code initially attached you get the following result:

~/Programming/Cpp/Test/cmake-build-relwithdebinfo > mpirun -np 2 ./test                                                      4s
+++ c=0, type=BP5Writer
+++ c=0, type=BP5Writer
+++ c=1, type=BP5Writer
+++ c=1, type=BP5Writer
+++ c=2, type=BP5Writer
+++ c=2, type=BP5Writer

What @germasch states is also true currently in master when using BP4 instead of BP5.

mpirun -np 2 ./test
+++ c=0, type=BP4Writer
+++ c=0, type=BP4Writer
+++ c=1, type=BP5Writer
+++ c=1, type=BP5Writer
+++ c=2, type=BP5Writer
+++ c=2, type=BP5Writer

I am looking into why this is happening.

@spyridon97
Copy link
Contributor

spyridon97 commented Dec 5, 2023

After looking into the code my understanding is the following:

When using a config file, an ADIOS object is initialized and it defines a set of IO/Operator objects.
This is basically a convenience mechanism, instead of writing code to initialize the IO objects (and operators).

  1. Beginning of 1st iteration:
    If you decide to declare an IO object with the same name as one defined in the config file, nothing is actually going to happen internally (1st iteration).

  2. End of 1st iteration:
    If you remove the phenomenally newly created IO, you actually delete what the config file originally defined.

  3. Beginning of 2nd iteration: If in the next iteration, you decide to define an IO object with the same name as one in the config file, the engine is not gonna be set to the same value as the one in the config file. because as we said the config file is only used for initialization and nothing else.

If the intended behavior of a config file is to initialize an ADIOS object with IO/Operators, then this is an expected behavior and not a bug.

If the intended behavior of a config file is to initialize an ADIOS object AND set the defaults (WHICH NEVER CHANGE) for initializing an ADIOS object, then this is a bug.

@williamfgc
Copy link
Contributor

It's a bug and a very corner case. Config files are a convenience to move things to runtime, and Remove functions are part of the danger zone for very unique cases. We can find tons of cases like these if we combine the APIs in a way outside our targeted use-cases, which are always prioritized over corner cases with no real use-case.

@anagainaru
Copy link
Contributor

Maybe we should re-discuss our policy to silently ignore invalid parameters / user input? This is all caused by the fact that we allow (and ignore) re-creating an IO object with the same name. A topic for next Tuesday.

@williamfgc
Copy link
Contributor

The question is if the new IO with the same name as the removed one is a completely new entity (which is the current policy). Remove functions are a very special case we never paid much attention as it's a one off.

@spyridon97
Copy link
Contributor

I know that ParaView is not related to ADIOS, but in my mind, the config file was the equivalent of a state file in ParaView. The state file is used to initialize something, but when you do any change (addition or removal of filters) to the paraview state, the previously loaded state, should not affect what happens next.

@spyridon97
Copy link
Contributor

spyridon97 commented Dec 7, 2023

I know that ParaView is not related to ADIOS, but in my mind, the config file was the equivalent of a state file in ParaView. The state file is used to initialize something, but when you do any change (addition or removal of filters) to the ParaView state, the previously loaded state φιλε, should not affect what happens next, e.g. at the creation of a new filter.

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 a pull request may close this issue.

6 participants