Feature/sina/append functionality by gwaegner · Pull Request #1542 · llnl/axom

gwaegner · 2025-04-08T20:49:25Z

Summary

This PR is a feature
It does the following (modify list as needed):
- Adds the ability for Sina to append to existing JSON and HDF5 files using file manipulation (JSON) and data streaming (HDF5)

…eave/axom into append_functionality

…to hdf5_new

doutriaux1 · 2025-08-11T14:56:11Z

@kennyweiss @cyrush @rhornung67 I think @HaluskaR is done. The PR looks good and I think is ready for you to give it a final review. Thanks @HaluskaR

rhornung67 · 2025-08-11T19:07:47Z

@kennyweiss @cyrush @rhornung67 I think @HaluskaR is done. The PR looks good and I think is ready for you to give it a final review. Thanks @HaluskaR

@doutriaux1 please approve if you think it is ready to merge. When you approve, we will do our final reviews. Thank you.

rhornung67 · 2025-08-11T21:31:36Z

This PR needs to be squash merged when done!!!

doutriaux1

@HaluskaR if the pngs I commented on are good and if you adressed @kennyweiss requested change then I'm good with this Pr.

doutriaux1 · 2025-08-05T19:53:18Z

@HaluskaR is that the original file from @gwaegner ? If so can we update it with results from your current implementation?

Good catch. Yes, those are the original results, but the pngs don't seem to be referenced from anywhere, might be how I overlooked them...remove for now and add a few pics from the presentation once that's up and ready to share around? It'd be nice to draw them from "real" data/use cases

…curve_ordering

doutriaux1 · 2025-08-21T20:54:48Z

@kennyweiss @white238 @rhornung67 I think this is ready to go. @HaluskaR we're good right?

doutriaux1 · 2025-08-29T00:37:08Z

@LLNL/axom please review/merge

kennyweiss

Looks like this is almost ready.

Please address the review, and also update the RELEASE_NOTES with the changes in this PR.

Please also update the RELEASE_NOTES for the changes in #1559 (we forgot to do that).

kennyweiss · 2025-08-29T20:11:58Z

+// String of a new Doc to use for appending
+std::string new_data =
+  "{\"records\": ["
+  "   {"
+  "       \"type\": \"foo1\","
+  "       \"id\": \"bar1\","
+  "       \"data\": {\"scal1\": {\"value\": \"goodbye!\"}, \"scal2\": {\"value\": \"goodbye!\"}},"
+  "       \"user_defined\":{\"hello\": \"goodbye\"},"
+  "       \"curve_sets\": {"
+  "           \"1\": {"
+  "               \"dependent\": {"
+  "                   \"0\": {\"value\": [1, 1]},"
+  "                   \"1\": {\"value\": [8, 9]}"
+  "               },"
+  "               \"independent\": {"
+  "                   \"0\": {\"value\": [4, 5]},"
+  "                   \"1\": {\"value\": [7, 9]}"
+  "               }"
+  "           },"
+  "           \"2\": {"
+  "               \"dependent\": {"
+  "                   \"0\": {\"value\": [1, 1]},"
+  "                   \"1\": {\"value\": [10, 10]}"
+  "               },"
+  "               \"independent\": {"
+  "                   \"0\": {\"value\": [4, 4]},"
+  "                   \"1\": {\"value\": [7, 7]}"
+  "               }"
+  "           }"
+  "       }"
+  "   },"
+  "   {"
+  "       \"type\": \"foo2\","
+  "       \"id\": \"bar3\","
+  "       \"curve_sets\": {"
+  "           \"1\": {"
+  "               \"dependent\": {"
+  "                   \"0\": {\"value\": [1, 1]},"
+  "                   \"1\": {\"value\": [10, 10]}"
+  "               },"
+  "               \"independent\": {"
+  "                   \"0\": {\"value\": [4, 4]},"
+  "                   \"1\": {\"value\": [7, 7]}"
+  "               }"
+  "           }"
+  "       }"
+  "   }"
+  "]}";


This one is still not updated.

kennyweiss · 2025-08-29T20:15:26Z

    std::to_string(SINA_FILE_FORMAT_VERSION_MINOR);
 }

+// just need to get something working...


This comment should be revised or removed.

Is this still considered a quick fix or has it been improved.

I'm not even sure what it's referencing. Between this and the raw json-like test document, I'm wondering if a cleanup commit was dropped. Easy enough fix. Thanks for catching these, Kenny.

kennyweiss · 2025-08-29T20:17:06Z

+                                     const std::string &endpoint,
+                                     const int mergeProtocol,
+                                     int record_num,
+                                     const std::string &original_file_path="");  // default'd because it might go away


const std::string &original_file_path=""); // default'd because it might go away

Should this question be resolved before merging?

Unclear comment from me--this referencing a possible addition to Conduit, not a PR change. Will clean that up.

kennyweiss · 2025-08-29T20:20:20Z

+// Helper function--if a node has children, add them to the provided. Assumes both nodes are conduit lists
+void concat_list_node(conduit::Node &concatTo, const conduit::Node &concatFrom)
+{
+  auto itr = concatFrom.children();
+  while(itr.has_next())
+  {
+    concatTo.append() = itr.next();
+  }
+}


Would it make sense to add (debug) checks that both concatTo and concatFrom are conduit lists?

kennyweiss · 2025-08-29T20:21:53Z

+///////////////////// TEMPLATE HELPER BLOCK -- smooth differences between JSON and HDF5 /////////////////////
+// This section exits because the hdf5 uses a dictionary to store records, ex: records/<some_num>/{actual_record}
+// whereas the JSON uses a list. The pathlike relay interface doesn't have access for list entries. This all should
+// be able to go away fairly cleanly if we swap over to dicts in JSON.
+conduit::Node &relayLikeRead(conduit::Node &appendTo, const std::string &endpoint, conduit::Node &readInto, int record_num)
+{
+  UNUSED(readInto);
+  return appendTo["records"].child(record_num)[endpoint];
+}


Nice comment!

I'm confused about the word "template" in "TEMPLATE HELPER BLOCK" -- these functions do not appear to be templated.

Good point, it's a reference to RelayLike further down but that's certainly not clear.

kennyweiss · 2025-08-29T20:22:36Z

+// We need to read exactly one thing outside the records set. Terrible.
+conduit::Node &relayLikeReadEtc(conduit::Node &appendTo, const std::string &endpoint, conduit::Node &readInto){
+  UNUSED(readInto);
+  return appendTo[endpoint];
+}


// We need to read exactly one thing outside the records set. Terrible.

Please update this comment before merging.

kennyweiss · 2025-08-29T20:25:00Z

+#endif
+#ifndef AXOM_USE_HDF5


Please change this to #else

Suggested change

#endif

#ifndef AXOM_USE_HDF5

#else

HaluskaR · 2025-08-29T21:12:34Z

I should delete these entirely.

…xom into feature/sina/append_functionality

kennyweiss

Thanks @HaluskaR -- I had a few small suggestions and one important but easy one about the UNUSED macro, which could potentially conflict with upstream dependencies, so we should fix it before merging.

kennyweiss · 2025-09-09T00:24:59Z

+- Sina: Records can now be provided with a curve ordering to use when writing to file. By default, all records will now use
+  oldest-first ordering (ULTRA-like)
+- Sina: Documents can optionally be written as HDF5 instead of JSON. HDF5 should provide better performance for large, curve-rich sets
+- Sina: Written documents can now be appended to. This is a flexible system with a few different uses, ex: continuous writing of timeseries, overwriting values that change over the course of a simulation, and snapshot handling. See documentation for details.


Thanks for updating the RELEASE-NOTES!

kennyweiss · 2025-09-09T00:28:07Z

+/**
+ * \brief Append the new records or, per-record, new data, user defined content, curves/curve sets,
+ *        and library data of a Sina Document to an existing JSON file. For a full explanation of behavior,
+ *        see append_to_hdf5. Note that a JSON file must be entirely re-written for an append to function;


Minor: Should this be appendDocumentToHDF5 instead of append_to_hdf5?

Suggested change

* see append_to_hdf5. Note that a JSON file must be entirely re-written for an append to function;

* see appendDocumentToHDF5. Note that a JSON file must be entirely re-written for an append to function;

kennyweiss · 2025-09-09T00:30:59Z

+/**
+ * \brief Check a node against some file handle and return a Conduit node populated with any errors that
+ *        would prevent appending. Primarily useful for testing out what may be going wrong with an append.
+ * 
+ * \param appendTo Either a conduit node or a conduit relay filehandle (prefer the latter for HDF5) representing
+ *                 the data on disk
+ * \param appendFrom A conduit node representing the document being appended to the file on disk
+ * \param mergeProtocol protocol for handling duplicate data/files/user_defined/etc. See append methods above.
+ * \return a conduit Node containing a list of any errors encountered in appending. If empty, success.
+ */
+template <typename ConduitRelayLike>
+conduit::Node validateAppendDocument(ConduitRelayLike &appendTo,
+                                     const conduit::Node &appendFrom,
+                                     const std::string &endpoint,
+                                     const int mergeProtocol,
+                                     int record_num,
+                                     // default'd because it might go away with a conduit update
+                                     const std::string &original_file_path = "");


A few of the parameters are not documented: endpoint, record_num, original_file_path

kennyweiss · 2025-09-09T00:32:35Z

+#define UNUSED(x) \
+  (void)(x)  // TODO: axom likely has a better preference on how to suppress the warning


Please remove this UNUSED macro, and replace all uses with AXOM_UNUSED_VAR (already included via axom/core.hpp)

…xom into feature/sina/append_functionality

kennyweiss

Thanks @HaluskaR !

Arlie-Capps

Thanks, @HaluskaR . Lots of work went into this one.

gwaegner and others added 30 commits September 9, 2024 13:00

Save As HDF5

44106aa

Pre-Slash Escaping

de76847

Replace Slashes Working

162e6bc

restoreSlashes implemented

9946477

removed the build directory that was added

bd82cb4

rememrged dev in here

7f3a1ac

removed more CMake files that shouldn't be here

0d8abca

more CMAKEFIles

8850ec4

more files to rmeove

a9c0119

last file?

6514f4b

last file?

d0a59e8

put file back in

0c0a63a

put another one back in

b7e103d

Update

e86911f

Update

0eb264f

Merge branch 'append_functionality' of ssh://czgitlab.llnl.gov:7999/w…

d1cfce3

…eave/axom into append_functionality

Useless

37555e6

merged dev in

da487de

Updated For Merge Request

03187b1

Updated for Merge

47f7a6e

got fortran to output hdf5 files, needs some more clean up

048baea

Merge remote-tracking branch 'origin/hdf5_new' into hdf5_new

cf4affe

cleaner names and defined HDF% and JSON to match Sina

5a8a533

Protocol set

a58bbae

Merge branch 'hdf5_new' of ssh://czgitlab.llnl.gov:7999/weave/axom in…

6ccbab1

…to hdf5_new

Merge branch 'hdf5_new' into append_functionality

e62a5f7

created a clean new branch from Gabriel's work to push to github repo

3f09c5e

Comments Implemented

e0cd5bf

More Comments

d2f38ac

Fix?

6c32398

Merge branch 'develop' into feature/sina/append_functionality

4b1be9c

doutriaux1 approved these changes Aug 11, 2025

View reviewed changes

HaluskaR and others added 2 commits August 13, 2025 15:05

Merge branch 'feature/haluska2/sina-curve-ordering' into append_plus_…

cb026fb

…curve_ordering

Merge branch 'develop' into feature/sina/append_functionality

2443b4e

Merge branch 'develop' into feature/sina/append_functionality

5de5161

kennyweiss requested changes Aug 29, 2025

View reviewed changes

kennyweiss requested a review from BradWhitlock August 29, 2025 20:28

kennyweiss added the Sina Issues related to Axom's 'sina' component label Aug 29, 2025

HaluskaR reviewed Aug 29, 2025

View reviewed changes

Comment thread src/axom/sina/docs/imgs/json_append_10_count_100_time_values.png

HaluskaR Aug 29, 2025

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

I should delete these entirely.

HaluskaR and others added 5 commits September 8, 2025 10:08

Add tests to ensure append mode doesn't interfere with curve ordering

dbb9cac

Merge branch 'feature/sina/append_functionality' of github.com:LLNL/a…

aa2511b

…xom into feature/sina/append_functionality

Sina comment cleanup and update release notes

fb8dda6

style

1ea1949

Merge branch 'develop' into feature/sina/append_functionality

754400e

kennyweiss requested changes Sep 9, 2025

View reviewed changes

HaluskaR and others added 3 commits September 11, 2025 10:49

Replace UNUSED macro, improve docs

cd6007e

Merge branch 'feature/sina/append_functionality' of github.com:LLNL/a…

19f900c

…xom into feature/sina/append_functionality

Merge branch 'develop' into feature/sina/append_functionality

ed6901a

kennyweiss approved these changes Sep 11, 2025

View reviewed changes

format-robot and others added 2 commits September 11, 2025 13:04

style

8581f9b

Updates mailmap

a712dc9

rhornung67 approved these changes Sep 11, 2025

View reviewed changes

Arlie-Capps approved these changes Sep 11, 2025

View reviewed changes

kennyweiss merged commit c87e743 into develop Sep 11, 2025
15 checks passed

kennyweiss deleted the feature/sina/append_functionality branch September 11, 2025 23:18

	* see append_to_hdf5. Note that a JSON file must be entirely re-written for an append to function;
	* see appendDocumentToHDF5. Note that a JSON file must be entirely re-written for an append to function;

		#define UNUSED(x) \
		(void)(x) // TODO: axom likely has a better preference on how to suppress the warning

Uh oh!

Conversation

gwaegner commented Apr 8, 2025

Summary

Uh oh!

doutriaux1 commented Aug 11, 2025

Uh oh!

rhornung67 commented Aug 11, 2025

Uh oh!

rhornung67 commented Aug 11, 2025

Uh oh!

doutriaux1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HaluskaR Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doutriaux1 commented Aug 21, 2025

Uh oh!

doutriaux1 commented Aug 29, 2025

Uh oh!

kennyweiss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kennyweiss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kennyweiss left a comment

Choose a reason for hiding this comment

Uh oh!

Arlie-Capps left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

HaluskaR Aug 12, 2025 •

edited

Loading