Skip to content

Conversation

@pgrete
Copy link
Collaborator

@pgrete pgrete commented Apr 12, 2024

PR Summary

Leftover todos (potentially for future PRs):

  • cleaner integration in build system
  • add logic for selection of var (rather than all)
  • double check additional logic for restart outputs (like UserWorkBeforeRestart)
  • Add doc
  • Add callback to provide/forward standard compatible downstream code info
  • Add support for Metadata::None
  • Add support for slices

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • Change is breaking (API, behavior, ...)
    • Change is additionally added to CHANGELOG.md in the breaking section
    • PR is marked as breaking
    • Short summary API changes at the top of the PR (plus optionally with an automated update/fix script)
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

@BenWibking
Copy link
Collaborator

I asked GPT-5 to review this PR. Is this an accurate comment?

> Review comment:

  - [P1] Implement VariableExists for OpenPMD restart — src/outputs/restart_opmd.hpp:158-162
    For the OpenPMD restart path RestartReaderOPMD::VariableExists currently just returns true. In ParthenonManager::RestartPackages we
  rely on this check to skip variables that are absent from the restart file (e.g., when someone adds a new Metadata::Restart variable
  after an older restart was written). Because this stub always reports the dataset exists, the code will go on to call ReadBlocks,
  which then throws from the PARTHENON_REQUIRE_THROWS guards when the OpenPMD record/component is missing. In practice this makes
  OpenPMD restarts fail in exactly the scenarios that already work with the HDF5 reader. Please implement the actual existence check
  (distinguishing between field vs swarm data) so the reader can safely skip missing variables.

@BenWibking
Copy link
Collaborator

BenWibking commented Oct 1, 2025

On macOS with the Homebrew OpenMPI build and more than 1 MPI rank, ADIOS2 crashes due to a bug in OpenMPI (MPI shared memory windows are borked) and brings down Parthenon with it.

I managed to work around this with backend_config = inputs/adios_no_shm.json:

{
  "adios2": {
    "engine": {
      "type": "bp4",
      "parameters": {
        "AggregatorChain": "everyonewrites"
      }
    }
  }
}

@BenWibking
Copy link
Collaborator

I've added AMR support to my VisIt openPMD reader plugin: https://github.com/BenWibking/openpmd-visit-reader.

Testing is welcome.

@pgrete
Copy link
Collaborator Author

pgrete commented Oct 7, 2025

On macOS with the Homebrew OpenMPI build and more than 1 MPI rank, ADIOS2 crashes due to a bug in OpenMPI (MPI shared memory windows are borked) and brings down Parthenon with it.

I managed to work around this with backend_config = inputs/adios_no_shm.json:

{
  "adios2": {
    "engine": {
      "type": "bp4",
      "parameters": {
        "AggregatorChain": "everyonewrites"
      }
    }
  }
}

Is the runtime config correct this way?
I'm asking at it says bp4 and the plan is to only support bp5 (as bp4 is deprecated).

@pgrete
Copy link
Collaborator Author

pgrete commented Oct 7, 2025

I asked GPT-5 to review this PR. Is this an accurate comment?

> Review comment:

  - [P1] Implement VariableExists for OpenPMD restart — src/outputs/restart_opmd.hpp:158-162
    For the OpenPMD restart path RestartReaderOPMD::VariableExists currently just returns true. In ParthenonManager::RestartPackages we
  rely on this check to skip variables that are absent from the restart file (e.g., when someone adds a new Metadata::Restart variable
  after an older restart was written). Because this stub always reports the dataset exists, the code will go on to call ReadBlocks,
  which then throws from the PARTHENON_REQUIRE_THROWS guards when the OpenPMD record/component is missing. In practice this makes
  OpenPMD restarts fail in exactly the scenarios that already work with the HDF5 reader. Please implement the actual existence check
  (distinguishing between field vs swarm data) so the reader can safely skip missing variables.

yes, this is why the function body currently carries a TODO(pgrete) need impl comment.
Should be straightforward (way more than for hdf5) to implement as it's just a simple check for a single key (rather than a hierarchy).
Will fix now.

@BenWibking
Copy link
Collaborator

On macOS with the Homebrew OpenMPI build and more than 1 MPI rank, ADIOS2 crashes due to a bug in OpenMPI (MPI shared memory windows are borked) and brings down Parthenon with it.
I managed to work around this with backend_config = inputs/adios_no_shm.json:

{
  "adios2": {
    "engine": {
      "type": "bp4",
      "parameters": {
        "AggregatorChain": "everyonewrites"
      }
    }
  }
}

Is the runtime config correct this way? I'm asking at it says bp4 and the plan is to only support bp5 (as bp4 is deprecated).

Yes. I thought bp4 was still supported?

Although I rebuilt OpenMPI with Spack on my Mac, and it works. So it's only an issue for the Homebrew-installed OpenMPI.

@pgrete
Copy link
Collaborator Author

pgrete commented Oct 7, 2025

On macOS with the Homebrew OpenMPI build and more than 1 MPI rank, ADIOS2 crashes due to a bug in OpenMPI (MPI shared memory windows are borked) and brings down Parthenon with it.
I managed to work around this with backend_config = inputs/adios_no_shm.json:

{
  "adios2": {
    "engine": {
      "type": "bp4",
      "parameters": {
        "AggregatorChain": "everyonewrites"
      }
    }
  }
}

Is the runtime config correct this way? I'm asking at it says bp4 and the plan is to only support bp5 (as bp4 is deprecated).

Yes. I thought bp4 was still supported?

Yes, correct. I just read up on this and bp4 might even have some advantages at large scale (though at the cost of a significant host memory footprint), see https://openpmd-api.readthedocs.io/en/latest/backends/adios2.html#heavy-i-o
Personally, I imagine sticking to bp5 as it's been doing pretty well so far.

@pgrete pgrete changed the title WIP: Add OpenPMD support Add OpenPMD support Oct 9, 2025
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.

5 participants