-
Notifications
You must be signed in to change notification settings - Fork 51
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
Fix ADIOS Warnings #267
Fix ADIOS Warnings #267
Conversation
Fix warnings and uninitialized vars in implementation.
{ | ||
ADIOS_FILE *f; | ||
ADIOS_FILE *f = nullptr; | ||
#if !openPMD_HAVE_MPI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@C0nsultant can you tell me why we snip out these region? If MPI is enabled, where does the serial implementation come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of the ADIOS MPI debacle, I got rid of all mock traces if we build without _NOMPI
. As ParallelADIOSIOHandlerImpl
inherits from ADIOSIOHandlerImpl
, we need to compile both. To make it easier not to run into MPI problems, I hid them when the serial ADIOS would not work as intended anyway. In that case, it can't be used as a backend, but merely provides functionality for the parallel backend.
The destructor has to be snipped out regardless because the logic is copied in the parallel version. Calling it twice with already finalized ADIOS will make something explode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in the current version, ADIOS will include a proper MPI as soon as we compile parallel which will work, besides the need to be started in an MPI context, also serial.
Does that mean you NOP-ed the serial backend if the parallel backend can be build? It's a bit tricky xD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in the current version, ADIOS will include a proper MPI
Which is part of my reasoning behind this. Having that compiled into the serial backend seems like the wrong thing to do. #258 seperated serial and parallel backends to the point where we do not asumme a working MPI mock employed in the parallel code (for the serial backend).
Depending on where ADIOS goes with respect to ornladios/ADIOS#183, we can change this.
Does that mean you NOP-ed the serial backend if the parallel backend can be build?
When it will be built, not just if it could.
But yes, it neutures a big part of the serial backend in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#258 seperated serial and parallel backends to the point where we do not asumme a working MPI mock employed in the parallel code (for the serial backend).
Makes absolute sense, that was the idea.
When it will be built, not just if it could.
Ok, I see.
I don't think that is fully necessary to clip it since it will still work as long as an MPI context is around. Maybe I get #254 to work, otherwise I will un-neuture the serial part again and document the RT MPI context detail with enabled parallel ADIOS backend using serial mode.
Fix warnings and uninitialized vars in implementation.