[Possibly breaking] Require all containers with the same base name in a `DataCollection` be made from the same field set by lroberts36 · Pull Request #1449 · parthenon-hpc-lab/parthenon

lroberts36 · 2026-09-02T17:56:37Z

PR Summary

[Made with assistance from generative AI]

This PR updates DataCollection to check that every container with a given base name is created from the same field list. Previously, code like

pmesh->mesh_data.Add("my_container", partition1, {fielda, fieldb, fieldc});
pmesh->mesh_data.Add("my_container", partition2, {fielda, fieldc});

would run fine if there was no overlap between partition1 and partition2, now it will throw an error.

I think that we should be enforcing this because boundary communication on meshdata that is built similarly to my_container will cause deadlocks in the task list. We ran into this issue in multigrid, where there MeshData with the same base name are built by both downstream code and internal to the solver on different partitions.

To make it easier to build containers in multiple places without keeping the field lists synchronized by hand, we also add the ability to query the DataCollection for the list of fields used to create a partition on a given base name, allowing

auto field_list = pmesh->mesh_data.GetCreationFields("my_container"); 
auto md = pmesh->mesh_data.Add("my_container", partition, field_list);

GetCreationFields returns an empty vector if no container has been previously created with the requested base name, so the example above would include all fields.

It is still valid to do something like

auto md1 = pmesh->mesh_data.Add("my_container", partition, {fielda, fieldb});
auto md2 = pmesh->mesh_data.Add("my_container", partition);

where the second call without a field list will still return the pre-existing container with the more limited field set.

PR Checklist

…les and introspect containers in solvers

lroberts36 · 2026-09-02T17:57:14Z

@par-hermes format

Yurlungur

Some nitpicks below, but otherwise LGTM.

Aside: I find myself nitpicking AI comments a lot these days because I find them so frequently overly verbose that they confuse rather than clarify.

Yurlungur · 2026-09-02T20:07:16Z

+      PARTHENON_THROW(
+          "Container \"" + name +
+          "\" is being created from different field lists on different sources. All "
+          "instances sharing a name must be created from the same field list.");
+    }


Might be useful for debugging to print the source label (if we know it) and both sets of fields.

Yurlungur · 2026-09-02T20:07:24Z

+      // containers built by hand or through a different DataCollection); an empty list
+      // means "all fields"/"don't check" and always passes.
+      if (fields.size() && !(it->second)->CreatedFrom(fields))
+        PARTHENON_THROW(key + " already exists in collection but fields do not match.");


Might be useful for debugging to print the source label (if we know it) and both sets of fields.

Yurlungur · 2026-09-02T20:10:09Z

Also I agree we should enforce this behavior. I think it was probably a mistake that it didn't in the first place.

Co-authored-by: Jonah Miller <DrLoco3000@gmail.com> Co-authored-by: Luke Roberts <lfroberts@lanl.gov>

lroberts36 · 2026-09-08T17:43:19Z

@par-hermes format

lroberts36 · 2026-09-08T17:45:21Z

@c-prather and @pgrete: Can you give a review on this? I think it is reasonable that we enforce all containers with the same base name contain the same set of fields, but I want to make sure that this doesn't conflict with anything in Kharma or AthenaPK.

c-prather

Tried this & it doesn't break anything in KHARMA. I agree with enforcing it generally.

pgrete

Given that uid are now used internally, does this mean that if I construct sth from a string vector, the resulting pack is not guaranteed to be in the same order of the passed vector any more?
Thinking in terms of a migration path for AthenaPK here and a potential hiccup for "simple" downstream implementations.

Also Codex identified a separate issue due to different defaults for empty lists{} in Initialization paths between MeshBlockData and ``MeshBlock`:

An empty fields list is source-dependent: copying MeshData preserves its current subset, while creating from a BlockListPartition selects all package fields. Recording {} in both cases
lets disjoint sources with different subsets pass the same-name check. It also makes GetCreationFields() return {} for a subset, allowing solver-created partitions to expand it to all
fields. Please resolve empty lists to the effective field set before recording and comparing them.

A reproducer unit test is here:

diff --git a/tst/unit/test_data_collection.cpp b/tst/unit/test_data_collection.cpp
index 013807d55..988b505cb 100644
--- a/tst/unit/test_data_collection.cpp
+++ b/tst/unit/test_data_collection.cpp
@@ -10,7 +10,10 @@
 // license in this material to reproduce, prepare derivative works, distribute copies to
 // the public, perform publicly and display publicly, and to permit others to do so.
 //========================================================================================
+// This file was made in part with generative AI.
+
 #include <memory>
+#include <set>
 #include <string>
 #include <vector>
 
@@ -18,6 +21,7 @@
 
 #include "basic_types.hpp"
 #include "interface/data_collection.hpp"
+#include "interface/mesh_data.hpp"
 #include "interface/meshblock_data.hpp"
 #include "interface/metadata.hpp"
 #include "kokkos_abstraction.hpp"
@@ -149,3 +153,78 @@ TEST_CASE("Adding MeshBlockData objects to a DataCollection", "[DataCollection]"
     }
   }
 }
+
+TEST_CASE("MeshData with the same base name must have the same effective field set",
+          "[DataCollection]") {
+  DataCollection<MeshData<Real>> d;
+
+  std::vector<int> size(6, 1);
+  Metadata m({Metadata::Independent}, size);
+
+  auto pkg = std::make_shared<StateDescriptor>("DataCollection MeshData field set test");
+  pkg->AddField("var1", m);
+  pkg->AddField("var2", m);
+  pkg->AddField("var3", m);
+
+  // Use different gids so that the two MeshData sources have distinct DataCollection
+  // cache keys despite using the same destination base name.
+  auto pmb0 = std::make_shared<MeshBlock>();
+  pmb0->gid = 0;
+  pmb0->resolved_packages = pkg;
+
+  auto pmb1 = std::make_shared<MeshBlock>();
+  pmb1->gid = 1;
+  pmb1->resolved_packages = pkg;
+
+  auto part0 = std::make_shared<parthenon::BlockListPartition>(
+      0, parthenon::GridIdentifier::leaf(), parthenon::BlockList_t{pmb0}, nullptr);
+  auto part1 = std::make_shared<parthenon::BlockListPartition>(
+      1, parthenon::GridIdentifier::leaf(), parthenon::BlockList_t{pmb1}, nullptr);
+
+  const std::vector<std::string> fields0{"var1", "var2"};
+  const std::vector<std::string> fields1{"var1", "var3"};
+
+  // Different base names may use different field subsets.
+  auto &src0 = d.Add("src0", part0, fields0);
+  auto &src1 = d.Add("src1", part1, fields1);
+
+  REQUIRE(src0->NumBlocks() == 1);
+  REQUIRE(src1->NumBlocks() == 1);
+  REQUIRE(src0->ContainsExactly(fields0));
+  REQUIRE(src1->ContainsExactly(fields1));
+
+  // Omitting the field list copies the source subset.
+  auto &work0 = d.Add("work", src0);
+  REQUIRE(work0->ContainsExactly(fields0));
+
+  SECTION("Implicit copies of different subsets are rejected across partitions") {
+    REQUIRE_THROWS_WITH(
+        d.Add("work", src1),
+        Catch::Matchers::Contains("different field lists on different sources"));
+  }
+
+  SECTION("Implicit copies of matching subsets are accepted across partitions") {
+    auto &matching = d.Add("matching", part1, fields0);
+    auto &work1 = d.Add("work", matching);
+    REQUIRE(work1->NumBlocks() == 1);
+    REQUIRE(work1->ContainsExactly(fields0));
+  }
+
+  SECTION("Creation fields describe the copied subset") {
+    const std::set<parthenon::Uid_t> expected{src0->GetBlockData(0)->UniqueID("var1"),
+                                              src0->GetBlockData(0)->UniqueID("var2")};
+    REQUIRE(d.GetCreationFields("work") == expected);
+  }
+
+  SECTION("Creation fields preserve the subset on a new block partition") {
+    auto &work1 = d.AddFromSet("work", part1, d.GetCreationFields("work"));
+    REQUIRE(work1->NumBlocks() == 1);
+    REQUIRE(work1->ContainsExactly(fields0));
+  }
+
+  SECTION("An omitted field list still reuses an existing subset container") {
+    auto &subset = d.Add("subset", part0, fields0);
+    REQUIRE(d.Add("subset", part0) == subset);
+    REQUIRE(subset->ContainsExactly(fields0));
+  }
+}

I verified this locally but am wondering how relevant this path is in practice for us.

pgrete · 2026-09-09T06:43:12Z

+    auto &md_base =
+        pmesh->mesh_data.AddFromSet(container_base, partitions[partition],
+                                    pmesh->mesh_data.GetCreationFields(container_base));
    // Container in which the solution is stored and with which the downstream user can


Does this imply that this is the recommend downstream pattern now, too?
If so, it might be worth to also mention this in the doc (and in general add a two-liner in the existing doc to reflect this change).

No, I don't think this is the generally recommended pattern unless you need to do something specifically like what is done in multigrid where someone creates containers in one place from a set of partitions/meshdata and then somewhere else you need to create them from a different set of partitions/meshdata.

pgrete · 2026-09-09T06:51:31Z

@@ -122,6 +135,54 @@ class DataCollection {
  void clear() { containers_.clear(); }


Should the clear also clear name_creation_fields_?

yes, good catch.

lroberts36 added 2 commits August 31, 2026 20:28

fixes to DataCollection::Add to be more careful about lists of variab…

98fbd36

…les and introspect containers in solvers

Merge branch 'develop' into lroberts36/fix-multiple-solvers

b60c3dd

par-hermes and others added 2 commits September 2, 2026 17:57

cpp-py-formatter

e2a76be

changelog

0160f61

lroberts36 changed the title ~~Require all containers with the same base name in a DataCollection be made from the same field set~~ [Possibly breaking] Require all containers with the same base name in a DataCollection be made from the same field set Sep 2, 2026

lroberts36 added the breaks-downstream label Sep 2, 2026

lroberts36 requested review from Yurlungur, c-prather and pgrete September 2, 2026 18:01

Yurlungur approved these changes Sep 2, 2026

View reviewed changes

Apply batched suggestions from code review

aa3dc54

Co-authored-by: Jonah Miller <DrLoco3000@gmail.com> Co-authored-by: Luke Roberts <lfroberts@lanl.gov>

cpp-py-formatter

0ebf11d

c-prather approved these changes Sep 8, 2026

View reviewed changes

pgrete reviewed Sep 9, 2026

View reviewed changes

		@@ -122,6 +135,54 @@ class DataCollection {
		void clear() { containers_.clear(); }

Uh oh!

Conversation

lroberts36 commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

lroberts36 commented Sep 2, 2026

Uh oh!

Yurlungur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Yurlungur Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

Yurlungur Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Yurlungur commented Sep 2, 2026

Uh oh!

lroberts36 commented Sep 8, 2026

Uh oh!

lroberts36 commented Sep 8, 2026

Uh oh!

c-prather left a comment

Choose a reason for hiding this comment

Uh oh!

pgrete left a comment

Choose a reason for hiding this comment

Uh oh!

pgrete Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

lroberts36 Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

pgrete Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

lroberts36 Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lroberts36 commented Sep 2, 2026 •

edited

Loading