Skip to content

hdf5 use subfiling if available#1177

Open
PhilipDeegan wants to merge 2 commits intoPHAREHUB:masterfrom
PhilipDeegan:hdf5_subfiling
Open

hdf5 use subfiling if available#1177
PhilipDeegan wants to merge 2 commits intoPHAREHUB:masterfrom
PhilipDeegan:hdf5_subfiling

Conversation

@PhilipDeegan
Copy link
Copy Markdown
Member

No description provided.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 23, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • ✅ Review completed - (🔄 Check again to review again)
📝 Walkthrough

Walkthrough

Adds compile-time and runtime gating for HDF5 subfiling (env-driven subfile count and SubFiler helper), refactors HighFiveFile to construct and apply local FAPL with conditional subfiling, and introduces a new CMake option plus conditional MPI init/finish changes tied to subfiling availability.

Changes

Cohort / File(s) Summary
HDF5 subfiling & file creation
src/hdf5/detail/h5/h5_file.hpp
Adds HDF5 subfiling detection/includes, determine_subfile_count() and SUB_FILES, introduces SubFiler that queries/updates subfiling/IOC FAPLs, refactors HighFiveFile::createHighFiveFile to build a local FAPL and conditionally apply SubFiler, removes fapl_ member and the templated overload accepting external FileAccessProps&.
SAMRAI CMake options
res/cmake/dep/samrai.cmake
Minor CMake cleanup of option calls and adds new ENABLE_TIMERS option (default OFF).
MPI init/finalize path
src/amr/samrai.cpp
Conditionally uses MPI_Init_thread(..., MPI_THREAD_MULTIPLE, ...) and calls MPI_Finalize() when subfiling VFD macro is enabled; otherwise keeps previous SAMRAI MPI init path. Adds fallback definition for H5_HAVE_SUBFILING_VFD when missing.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Caller
participant HF as HighFiveFile::createHighFiveFile
participant SubFiler
participant HDF5 as HDF5 API (FAPL)
participant File as HighFiveFile
Caller->>HF: createHighFiveFile(path, flags, para)
HF->>HDF5: construct local FileAccessProps (fapl)
alt SUB_FILES > 0
HF->>SubFiler: SubFiler(fapl.getId())
SubFiler->>HDF5: H5Pget_fapl_subfiling / H5Pget_fapl_ioc
SubFiler->>SubFiler: set stripe_count = SUB_FILES
SubFiler->>HDF5: H5Pset_fapl_ioc(fapl_id, ioc_cfg)
SubFiler->>HDF5: H5Pset_fapl_subfiling(fapl_id, subfiling_cfg)
end
HF->>File: open/create file with fapl
File-->>Caller: file handle

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • nicolasaunai
🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive No description was provided by the author; the pull request description is empty. Add a description explaining the purpose, motivation, and impact of adding conditional HDF5 subfiling support.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and clearly describes the main purpose of the changeset: adding HDF5 subfiling support when available.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/hdf5/detail/h5/h5_file.hpp`:
- Around line 101-104: Subfiling is being added unconditionally via
fapl.add(SubFiler{}) when H5_HAVE_SUBFILING_VFD is set; restrict this so
SubFiler is only applied for parallel I/O by checking the parallel flag (para)
before adding it. Modify the block that currently references
fapl.add(SubFiler{}) (guarded by H5_HAVE_SUBFILING_VFD) to also test that para
== true (or equivalent parallel-mode indicator) and only call
fapl.add(SubFiler{}) when both the compile-time macro and the runtime para flag
indicate parallel I/O.
- Around line 59-82: The SubFiler constructor must fully initialize the
H5FD_subfiling_config_t before calling H5Pset_fapl_subfiling: zero-initialize
vfd_config (e.g., memset or value-initialize), then explicitly set its required
fields such as magic (use the HDF5 subfiling magic constant), version (use the
correct subfiling config version constant), stripe_count (keep
detail::SUB_FILES), and any other mandatory fields like
stripe_unit/target_bytes/alignment/flags per the H5FD_subfiling_config_t
definition; leave the apply method calling H5Pset_fapl_subfiling unchanged but
ensure vfd_config is valid when constructed in SubFiler::SubFiler.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7cb5f083-6eaf-4881-b8c8-8c5ff152f368

📥 Commits

Reviewing files that changed from the base of the PR and between 4b1b9ed and dc5c914.

📒 Files selected for processing (1)
  • src/hdf5/detail/h5/h5_file.hpp

@PhilipDeegan PhilipDeegan force-pushed the hdf5_subfiling branch 2 times, most recently from e79ca9f to 35d229f Compare March 23, 2026 13:31
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/hdf5/detail/h5/h5_file.hpp (1)

117-117: Remove the unconditional std::cout from this file-open path.

Every participating rank will print here on each parallel file open, which is noisy and adds avoidable overhead for what looks like a temporary debug aid.

♻️ Proposed cleanup
-            std::cout << "detail::SUB_FILES! " << detail::SUB_FILES << std::endl;
             if (detail::SUB_FILES > 0) // run time disabled
                 fapl.add(SubFiler{fapl.getId()});

Based on learnings, PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/hdf5/detail/h5/h5_file.hpp` at line 117, Remove the unconditional debug
print in the file-open path: delete the line that writes std::cout <<
"detail::SUB_FILES! " << detail::SUB_FILES << std::endl; (which references
detail::SUB_FILES) and, if needed, replace it with a proper logging call or
conditional debug macro that only emits output when verbose/debug mode is
enabled; ensure no other side-effects remain in the file-open codepath.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@res/cmake/dep/samrai.cmake`:
- Around line 33-34: The two CMake cache entries set(ENABLE_TIMERS OFF CACHE
BOOL "NO!" FORCE) and set(ENABLE_CHECK_ASSERTIONS OFF CACHE BOOL "NO!" FORCE)
should not forcibly overwrite user cache values; remove the FORCE keyword so
they become set(ENABLE_TIMERS OFF CACHE BOOL "NO!") and
set(ENABLE_CHECK_ASSERTIONS OFF CACHE BOOL "NO!"). Keep the default OFF but
allow cache overrides (note: do not change any handling of
DEBUG_CHECK_ASSERTIONS — that stays driven by CMAKE_BUILD_TYPE).

In `@src/hdf5/detail/h5/h5_file.hpp`:
- Around line 116-119: Replace the startup MPI initialization call in
tools/config/main.cpp from MPI_Init to MPI_Init_thread(..., MPI_THREAD_MULTIPLE,
...) and handle/check its return value so the process ensures the requested
thread support before enabling HDF5 subfiling; then remove the debug std::cout
line in src/hdf5/detail/h5/h5_file.hpp that prints detail::SUB_FILES and keep
the existing conditional that calls fapl.add(SubFiler{fapl.getId()}) when
detail::SUB_FILES > 0 (ensuring subfiling is only used when MPI_Init_thread
succeeded).

---

Nitpick comments:
In `@src/hdf5/detail/h5/h5_file.hpp`:
- Line 117: Remove the unconditional debug print in the file-open path: delete
the line that writes std::cout << "detail::SUB_FILES! " << detail::SUB_FILES <<
std::endl; (which references detail::SUB_FILES) and, if needed, replace it with
a proper logging call or conditional debug macro that only emits output when
verbose/debug mode is enabled; ensure no other side-effects remain in the
file-open codepath.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ee78c92c-6abf-4541-8fe0-0d7922a3516d

📥 Commits

Reviewing files that changed from the base of the PR and between dc5c914 and e79ca9f.

📒 Files selected for processing (2)
  • res/cmake/dep/samrai.cmake
  • src/hdf5/detail/h5/h5_file.hpp

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
src/hdf5/detail/h5/h5_file.hpp (3)

64-65: Type mismatch between detail::SUB_FILES and subfiles parameter.

detail::SUB_FILES is std::size_t but the parameter is declared as int, causing implicit narrowing. While unlikely to cause issues in practice (subfile counts won't exceed INT_MAX), using a consistent type improves clarity.

♻️ Suggested fix
-    SubFiler([[maybe_unused]] hid_t const plist_id,
-             [[maybe_unused]] int const subfiles = detail::SUB_FILES)
+    SubFiler([[maybe_unused]] hid_t const plist_id,
+             [[maybe_unused]] std::size_t const subfiles = detail::SUB_FILES)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/hdf5/detail/h5/h5_file.hpp` around lines 64 - 65, The SubFiler
constructor currently declares the parameter "int const subfiles" but uses
detail::SUB_FILES (type std::size_t) as the default, causing a type mismatch;
change the parameter type to std::size_t (e.g., "std::size_t const subfiles") so
the default and parameter types match and avoid implicit narrowing—update the
SubFiler([[maybe_unused]] hid_t const plist_id, [[maybe_unused]] int const
subfiles = detail::SUB_FILES) declaration to use std::size_t for subfiles and
adjust any callers if they relied on signed int.

74-74: Typo: "defauls" → "defaults".

📝 Suggested fix
-        // expected defauls
+        // expected defaults
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/hdf5/detail/h5/h5_file.hpp` at line 74, Fix the typo in the inline
comment in h5_file.hpp: change the word "defauls" to "defaults" in the comment
near the expected defaults section (the comment line containing "// expected
defauls") so the comment reads "// expected defaults".

71-71: Remove commented-out dead code.

This duplicates line 68 and adds no value.

🧹 Suggested fix
-        // H5Pget_fapl_subfiling(plist_id, &vfd_config);
         vfd_config.shared_cfg.stripe_count = subfiles;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/hdf5/detail/h5/h5_file.hpp` at line 71, Remove the commented-out
duplicate call "H5Pget_fapl_subfiling(plist_id, &vfd_config);" from
src/hdf5/detail/h5/h5_file.hpp—it's dead/commented code duplicating the earlier
occurrence (line with the same call) and should be deleted to avoid clutter;
ensure no other references rely on that commented line and keep the single
canonical invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/hdf5/detail/h5/h5_file.hpp`:
- Around line 68-69: The calls to H5Pget_fapl_subfiling and H5Pget_fapl_ioc
ignore their return values, leaving vfd_config and ioc_config potentially
uninitialized on failure; update the code around plist_id to check each call's
return value, detect negative (error) returns from
H5Pget_fapl_subfiling(plist_id, &vfd_config) and H5Pget_fapl_ioc(plist_id,
&ioc_config), and handle errors consistently (e.g., log with context and
propagate/throw or return an error code) so that failures are not silently
ignored and downstream code using vfd_config/ioc_config is protected.

---

Nitpick comments:
In `@src/hdf5/detail/h5/h5_file.hpp`:
- Around line 64-65: The SubFiler constructor currently declares the parameter
"int const subfiles" but uses detail::SUB_FILES (type std::size_t) as the
default, causing a type mismatch; change the parameter type to std::size_t
(e.g., "std::size_t const subfiles") so the default and parameter types match
and avoid implicit narrowing—update the SubFiler([[maybe_unused]] hid_t const
plist_id, [[maybe_unused]] int const subfiles = detail::SUB_FILES) declaration
to use std::size_t for subfiles and adjust any callers if they relied on signed
int.
- Line 74: Fix the typo in the inline comment in h5_file.hpp: change the word
"defauls" to "defaults" in the comment near the expected defaults section (the
comment line containing "// expected defauls") so the comment reads "// expected
defaults".
- Line 71: Remove the commented-out duplicate call
"H5Pget_fapl_subfiling(plist_id, &vfd_config);" from
src/hdf5/detail/h5/h5_file.hpp—it's dead/commented code duplicating the earlier
occurrence (line with the same call) and should be deleted to avoid clutter;
ensure no other references rely on that commented line and keep the single
canonical invocation.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 234bc6ce-f0a7-4069-81eb-85c7c7c081f5

📥 Commits

Reviewing files that changed from the base of the PR and between e79ca9f and 35d229f.

📒 Files selected for processing (2)
  • res/cmake/dep/samrai.cmake
  • src/hdf5/detail/h5/h5_file.hpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • res/cmake/dep/samrai.cmake

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/hdf5/detail/h5/h5_file.hpp`:
- Around line 64-95: The SubFiler constructor currently acquires
vfd_config.ioc_fapl_id via H5Pget_fapl_subfiling but never closes it, causing
leaks and unsafe behavior if the constructor throws; implement RAII for that
handle by adding a dedicated owning member (e.g., an ioc_fapl_id wrapper or
std::unique_ptr-like holder with a custom deleter that calls H5Pclose) and
ensure SubFiler's destructor closes the ioc_fapl_id, make the constructor
transfer ownership into that RAII member only after successful acquisition, and
update any code that uses vfd_config.ioc_fapl_id (including the H5Pset_fapl_ioc
call and apply) to access the RAII-owned handle so ownership is exception-safe
and properly released.
- Around line 26-33: determine_subfile_count currently returns 20 by default
which auto-enables HDF5 subfiling; change the default to 0 so subfiling is
opt-in via PHARE_H5_SUB_FILES. Edit the function determine_subfile_count (and
any related constant usage like detail::SUB_FILES) to return
core::get_env_as("PHARE_H5_SUB_FILES", int{0}) when H5_HAVE_SUBFILING_VFD is
defined, leaving behavior unchanged otherwise.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: cdd15cb2-1f28-43fc-83fd-2ed4270cd8bd

📥 Commits

Reviewing files that changed from the base of the PR and between 35d229f and 124873b.

📒 Files selected for processing (2)
  • res/cmake/dep/samrai.cmake
  • src/hdf5/detail/h5/h5_file.hpp
✅ Files skipped from review due to trivial changes (1)
  • res/cmake/dep/samrai.cmake

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/hdf5/detail/h5/h5_file.hpp (1)

26-35: Consider defaulting SUB_FILES to 0 for opt-in behavior.

The current default of 20 means any parallel build with subfiling VFD present will automatically enable subfiling when detail::SUB_FILES > 0 (line 116). Since HDF5 subfiling requires MPI_THREAD_MULTIPLE, this automatic enablement may cause issues for deployments that initialize MPI without that thread level. Making it opt-in by defaulting to 0 would be safer.

🛡️ Suggested change
 int inline determine_subfile_count()
 {
 `#if` H5_HAVE_SUBFILING_VFD
-    return core::get_env_as("PHARE_H5_SUB_FILES", int{20});
+    return core::get_env_as("PHARE_H5_SUB_FILES", int{0});
 `#endif`
     return 0;
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/hdf5/detail/h5/h5_file.hpp` around lines 26 - 35, The SUB_FILES default
currently enables subfiling by returning 20 from determine_subfile_count(),
causing automatic activation when H5_HAVE_SUBFILING_VFD is present; change the
default to 0 so subfiling is opt-in by environment only. Update the
determine_subfile_count() implementation to call
core::get_env_as("PHARE_H5_SUB_FILES", int{0}) (refer to the
determine_subfile_count function and the SUB_FILES static inline constant) so
SUB_FILES becomes 0 unless the environment overrides it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/amr/samrai.cpp`:
- Around line 14-20: The MPI_Init_thread call captures 'provided' but never
verifies MPI_THREAD_MULTIPLE support; update the block where
MPI_Init_thread(&argc, &argv, MPI_THREAD_MULTIPLE, &provided) is called to check
the 'provided' value and handle the failure (e.g., log an error via process
logger or SAMRAI logging and abort/throw) if provided < MPI_THREAD_MULTIPLE;
locate this logic around MPI_Init_thread, the 'provided' variable, and the
subsequent SAMRAI::tbox::SAMRAI_MPI::init(MPI_COMM_WORLD) call and ensure the
check runs before calling SAMRAI::tbox::SAMRAI_MPI::init.

In `@src/hdf5/detail/h5/h5_file.hpp`:
- Around line 62-86: The SubFiler constructor can leak vfd_config.ioc_fapl_id if
an exception is thrown after H5Pget_fapl_subfiling/H5Pget_fapl_ioc, so wrap the
acquired ioc_fapl_id in an RAII holder or a scope guard and only assign it back
to vfd_config (or release the guard) after all subsequent calls (like
H5Pset_fapl_ioc) succeed; alternatively, change the sequence in SubFiler to
store the ioc handle into a local hid_t temp (from H5Pget_fapl_ioc), perform
H5Pset_fapl_ioc using that temp, and on success assign temp to
vfd_config.ioc_fapl_id (or ensure the RAII wrapper transfers ownership), and
ensure ~SubFiler still closes vfd_config.ioc_fapl_id with H5Pclose.

---

Nitpick comments:
In `@src/hdf5/detail/h5/h5_file.hpp`:
- Around line 26-35: The SUB_FILES default currently enables subfiling by
returning 20 from determine_subfile_count(), causing automatic activation when
H5_HAVE_SUBFILING_VFD is present; change the default to 0 so subfiling is opt-in
by environment only. Update the determine_subfile_count() implementation to call
core::get_env_as("PHARE_H5_SUB_FILES", int{0}) (refer to the
determine_subfile_count function and the SUB_FILES static inline constant) so
SUB_FILES becomes 0 unless the environment overrides it.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: bbadd05c-ab5d-4e15-92d0-2e9c1bd2180b

📥 Commits

Reviewing files that changed from the base of the PR and between 124873b and ad44180.

📒 Files selected for processing (3)
  • res/cmake/dep/samrai.cmake
  • src/amr/samrai.cpp
  • src/hdf5/detail/h5/h5_file.hpp

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
src/amr/samrai.cpp (1)

14-24: Move the MPI_THREAD_MULTIPLE validation before SAMRAI_MPI::init.

The check for provided < MPI_THREAD_MULTIPLE occurs after SAMRAI::tbox::SAMRAI_MPI::init(MPI_COMM_WORLD). If the MPI implementation doesn't provide the required thread level, SAMRAI will already be partially initialized before the exception is thrown.

♻️ Proposed fix to validate before SAMRAI initialization
 `#if` H5_HAVE_SUBFILING_VFD
     int provided;
     MPI_Init_thread(&argc, &argv, MPI_THREAD_MULTIPLE, &provided);
+    if (provided < MPI_THREAD_MULTIPLE)
+        throw std::runtime_error(
+            "MPI_THREAD_MULTIPLE required for HDF5 subfiling but not provided");
     SAMRAI::tbox::SAMRAI_MPI::init(MPI_COMM_WORLD);
-    if (provided < MPI_THREAD_MULTIPLE)
-        throw std::runtime_error(
-            "MPI_THREAD_MULTIPLE required for HDF5 subfiling but not provided");
 
 `#else`  // normal way
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/amr/samrai.cpp` around lines 14 - 24, Call MPI_Init_thread(&argc, &argv,
MPI_THREAD_MULTIPLE, &provided) first, then immediately check if provided <
MPI_THREAD_MULTIPLE and throw the std::runtime_error if so, and only after that
call SAMRAI::tbox::SAMRAI_MPI::init(...); in other words move the provided
validation to occur before invoking SAMRAI::tbox::SAMRAI_MPI::init so SAMRAI is
not initialized when the required MPI thread support is absent (references:
MPI_Init_thread, provided, MPI_THREAD_MULTIPLE, SAMRAI::tbox::SAMRAI_MPI::init).
src/hdf5/detail/h5/h5_file.hpp (1)

81-86: Guard against closing an invalid handle in destructor.

If H5Pget_fapl_subfiling fails and throws before vfd_config.ioc_fapl_id is populated with a valid handle, or if the object is moved-from, calling H5Pclose on an uninitialized/invalid handle causes undefined behavior.

🛡️ Proposed fix to validate handle before closing
     ~SubFiler()
     {
 `#if` H5_HAVE_SUBFILING_VFD
-        H5Pclose(vfd_config.ioc_fapl_id);
+        if (vfd_config.ioc_fapl_id > 0)
+            H5Pclose(vfd_config.ioc_fapl_id);
 `#endif`
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/hdf5/detail/h5/h5_file.hpp` around lines 81 - 86, The SubFiler destructor
currently calls H5Pclose(vfd_config.ioc_fapl_id) unconditionally when
H5_HAVE_SUBFILING_VFD is set; guard against closing an invalid handle by
checking the handle against H5I_INVALID_HID before calling H5Pclose (i.e., only
call H5Pclose if vfd_config.ioc_fapl_id != H5I_INVALID_HID) and ensure
vfd_config.ioc_fapl_id is initialized to H5I_INVALID_HID where vfd_config is
constructed/declared to cover move-from or failed H5Pget_fapl_subfiling cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tools/config/main.cpp`:
- Around line 19-24: The hdf5_subfiling flag is set unconditionally true when
HDF5 is present; change it to reflect the actual subfiling VFD availability by
checking the H5_HAVE_SUBFILING_VFD macro: set constexpr static bool
hdf5_subfiling = true only when H5_HAVE_SUBFILING_VFD is defined (otherwise
false), keeping the existing H5_HAVE_PARALLEL handling and the hdf5_version
behavior intact so the symbols hdf5_subfiling and H5_HAVE_SUBFILING_VFD are used
to determine the correct boolean value.
- Around line 52-55: The function write_hdf5_has_subfiling() currently writes
the result of hdf5_is_parallel() instead of the subfiling flag; change its call
so it writes the actual subfiling status (use the H5_HAVE_SUBFILING_VFD macro or
the appropriate helper like hdf5_has_subfiling() if available) to the
"PHARE_HDF5_has_subfiling.txt" file via write_string_to_file, ensuring the value
is converted to a string representation.

---

Nitpick comments:
In `@src/amr/samrai.cpp`:
- Around line 14-24: Call MPI_Init_thread(&argc, &argv, MPI_THREAD_MULTIPLE,
&provided) first, then immediately check if provided < MPI_THREAD_MULTIPLE and
throw the std::runtime_error if so, and only after that call
SAMRAI::tbox::SAMRAI_MPI::init(...); in other words move the provided validation
to occur before invoking SAMRAI::tbox::SAMRAI_MPI::init so SAMRAI is not
initialized when the required MPI thread support is absent (references:
MPI_Init_thread, provided, MPI_THREAD_MULTIPLE, SAMRAI::tbox::SAMRAI_MPI::init).

In `@src/hdf5/detail/h5/h5_file.hpp`:
- Around line 81-86: The SubFiler destructor currently calls
H5Pclose(vfd_config.ioc_fapl_id) unconditionally when H5_HAVE_SUBFILING_VFD is
set; guard against closing an invalid handle by checking the handle against
H5I_INVALID_HID before calling H5Pclose (i.e., only call H5Pclose if
vfd_config.ioc_fapl_id != H5I_INVALID_HID) and ensure vfd_config.ioc_fapl_id is
initialized to H5I_INVALID_HID where vfd_config is constructed/declared to cover
move-from or failed H5Pget_fapl_subfiling cases.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b5eadf42-dbc1-4016-9c88-b76950af6f07

📥 Commits

Reviewing files that changed from the base of the PR and between 124873b and a0b9de7.

📒 Files selected for processing (5)
  • res/cmake/dep/samrai.cmake
  • src/amr/samrai.cpp
  • src/hdf5/detail/h5/h5_file.hpp
  • tools/config/config.py
  • tools/config/main.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • res/cmake/dep/samrai.cmake

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.

2 participants