Driver-portable walker logging #5019

jtkrogel · 2024-05-31T20:35:14Z

Proposed changes

This PR introduces a simplified version of the legacy TraceManager that focuses solely on writing per-walker data to HDF5 throughout the run (no streaming/listener functionality). The new implementation, WalkerLogManager, is compatible with and implemented in both the legacy and batched drivers.

This implementation extends the original to include the ability to write full data for the highest, lowest, and median energy walkers on each rank to disk at every step. The intention of writing this abbreviated data is to enable inexpensive access to data from problematic walkers (e.g. population explosions) in production runs.

Capabilities

Writing per-walker scalar data (local energy, etc)
(optional) writing per-walker particle data (positions, spins, gradient, laplacian)
(optional) writing full data for min/max/median energy walkers

Below is an example generated from a short DMC run of diamond on 6 MPI ranks showing the energies of all walkers (blue), highest energy walkers (green), lowest energy walkers (red), median energy walkers (black), vs the mean energy from dmc.dat (magenta line):

Design

Main classes: WalkerLogManager, WalkerLogCollector, WalkerLogBuffer
WalkerLogManager
- Driver-level resource
- Responsible for HDF file I/O
- Finds min/max/median energy walker data
- Writes walker data buffers from each crowd to HDF at the end of each block
WalkerLogCollector
- Crowd-level resource
- Collects per-walker data during MC steps and stores it in local data buffers
WalkerLogBuffer
- Acts as a dynamic buffer for walker data collected during a block
- Resource allocation is first-touch

Tests

Legacy and batched driver deterministic tests
1-mpi/1-thread and 4-mpi/4-thread versions
VMC and DMC
The tests pass if the walker logging data can be used to reconstruct the information outputted to scalar.dat (VMC) or dmc.dat (DMC)

What type(s) of changes does this code introduce?

New feature

Does this introduce a breaking change?

No

What systems has this change been tested on?

Laptop

Checklist

Yes. This PR adds tests to cover any new code, or to catch a bug that is being fixed

Path out of WIP

Tasks for review round 3

Make step counter seem like a global data member
Long names for crowd member functions
Make more getters()
Inline default values for WalkerLogState
Try higher level api for hdf (results in nan)

Elements for reviewers to review

WalkerLog* classes
WalkerLogCollector unit test
Driver integration

src/QMCDrivers/DMC/DMCBatched.h

ye-luo · 2024-06-11T03:03:02Z

src/QMCDrivers/QMCDriverFactory.cpp

+  //add trace information
+  bool allow_walker_logs = das.walkerlogs_tag == "yes" ||
+      (das.walkerlogs_tag == "none" && (das.new_run_type == QMCRunType::VMC || das.new_run_type == QMCRunType::DMC || das.new_run_type == QMCRunType::VMC_BATCH || das.new_run_type == QMCRunType::DMC_BATCH));
+  new_driver->requestWalkerLogs(allow_walker_logs);


Complicated control logic. Prefer not to have another switch. I would like to have the following.
if "walkerlog" appears globally, all the drivers honor it. if not, each driver honor the "walkerlog" within the driver input section.

Agreeing with Ye here - we use similar logic for maxcpusecs

The control logic protects drivers which either don't support walker logging, or shouldn't.

Traces were never enabled for optimizers, for instance. On another note, I really don't want to write code to support walker logging in all of the legacy drivers. CSVMC anyone?

Further comments welcome.

The current walker logging support is limited to VMC/DMC and its functionality and implementation can be completely different for a non-VMC/DMC driver. So its input is more suited for a driver input section rather than globally. If only some VMC/DMC drivers need to turn on logging, the input should be in the driver input section. Right now, we provide convenience to have the input globally and I believe it is better not to another switch to turn it off locally. To me, that is a bad design logic.

As for the other protection, setting allow_walker_logs correctly to false for unsupported drivers is what makes the WalkerLogManager/WalkerLogCollector operations a no-op.

Then default value of allow_walker_logs is already false to all the unsupported drivers. Why do we even need to set it.

Too much details. I would like to discuss the user-facing input logic first and then we can address the implementation issue, maybe at a later PR.

Are you OK with the following input logic

if the global <walkerlog/> exists, turn on logging in all the supported drivers.

when the global doesn't exist, legacy drivers do nothing. batched drivers will look for driver scope <walkerlog/> and act on it.

This will all be simpler once drivers actually parse their own input. Right now driver input parsing is handled in QMCDriverFactory. I suggest we focus on that problem and this one will sort itself out.

src/QMCDrivers/Crowd.h

src/QMCDrivers/DMC/DMCBatched.cpp

ye-luo · 2024-06-11T03:37:10Z

I have merged in develop and fixed cmake issues. Please pull before making further changes.

jtkrogel · 2024-06-11T14:41:41Z

I've addressed Peter's comment from #5035 requesting simplification of WalkerLogState.

jtkrogel · 2024-06-11T14:43:39Z

I've also tried Peter's suggestion to use f.writeEntry(buffer, "data") in WalkerLogBuffer::writeHDF. Making that replacement resulted in NaN's being written to the HDF files (picked up by integration test). Therefore I've kept the original implementation.

jtkrogel · 2024-06-11T14:43:57Z

Comments addressed. Ready to proceed.

ye-luo · 2024-06-11T16:07:02Z

I've also tried Peter's suggestion to use f.writeEntry(buffer, "data") in WalkerLogBuffer::writeHDF. Making that replacement resulted in NaN's being written to the HDF files (picked up by integration test). Therefore I've kept the original implementation.

You need h5d_append and we don't have a high-level API in hdf_archive right now. added issue #5038. WalkerLogBuffer::appendHDF can be a better name to avoid confusion.

ye-luo

Still quite a few fixes are needed but it is not necessary to hold the PR.

ye-luo · 2024-06-11T16:56:28Z

Test this please

PDoakORNL

I think we need to hold some sort of line to model good simulation object life cycle control and input handling for the batched code.

PDoakORNL · 2024-06-05T14:18:32Z

src/QMCHamiltonians/OperatorBase.cpp

@@ -268,7 +268,6 @@ void OperatorBase::deleteTraceQuantities()
  have_required_traces_ = false;
  request_.reset();
 }
-


This shouldn't be in the change set.

Adding an unecessary file to the PR file list for white space change has never been allowed. I have had to fix such changes many times. Finding these sort of typos takes effort by reviewers and they stack up over time.

src/Estimators/WalkerTraceBuffer.h

src/QMCDrivers/Crowd.h

PDoakORNL · 2024-06-12T17:10:33Z

src/QMCApp/QMCMain.h

@@ -75,6 +75,9 @@ class QMCMain : public MPIObjectBase, public QMCAppBase
  ///xml mcwalkerset read-in elements
  std::vector<xmlNodePtr> walker_set_in_;

+  ///walkerlogs xml
+  xmlNodePtr walker_logs_xml_;


please do something like

std::optional<WalkerLogInput> walker_log_input_;

I also don't like storing xml, parsing the xml in-place and store the result seems cleaner. We can make some improvements with a later PR.

PDoakORNL · 2024-06-12T17:41:32Z

src/QMCDrivers/VMC/VMCBatched.cpp

@@ -304,6 +309,12 @@ bool VMCBatched::run()
  IndexType num_blocks = qmcdriver_input_.get_max_blocks();
  //start the main estimator
  estimator_manager_->startDriverRun();
+  //start walker log manager
+  wlog_manager_ = std::make_unique<WalkerLogManager>(walker_logs_input, allow_walker_logs, get_root_name(), myComm);


Please construct this in the QMCDriverNew constructor as EstimatorManagerNew is.

It should not be constructed in the constructor. I have moved this line to process() in #5039 at the moment. When the global input doesn't exist, we may look it up locally.

Process is also an API element that only exists for legacy compatibility, I'd be very suprised if construction actually needed to delayed until then, but lets discuss later.

PDoakORNL · 2024-06-12T17:46:12Z

src/QMCDrivers/QMCDriverNew.h

+  /// whether to allow walker logs
+  bool allow_walker_logs;
+  /// walker logs input
+  WalkerLogInput walker_logs_input;


This flag plus optional input object optional unique_pointer_ too much optional stuff. You can either check the presence of the object or the input, the allow is unecessary.

PDoakORNL · 2024-06-12T17:48:00Z

src/QMCDrivers/QMCDriverNew.h

@@ -241,6 +249,10 @@ class QMCDriverNew : public QMCDriverInterface, public MPIObjectBase
  void putTraces(xmlNodePtr txml) override {}
  void requestTraces(bool allow_traces) override {}

+  void putWalkerLogs(xmlNodePtr wlxml) override;
+
+  void requestWalkerLogs(bool allow_walker_logs_) override { allow_walker_logs = allow_walker_logs_; }


These seem to be API elements required by legacy, they shouldn't do anything for batched and run counter to clean construction. This should be documented here and and note to remove with the legacy drivers added.

PDoakORNL · 2024-06-12T17:50:39Z

src/QMCDrivers/Crowd.cpp

 }

 void Crowd::stopBlock() { estimator_manager_crowd_.stopBlock(); }

+void Crowd::collectStepWalkerLog(int current_step)


logWalkers(int current_step)
Seems to say everything necessary.

PDoakORNL · 2024-06-12T18:05:08Z

src/QMCDrivers/DMC/DMCBatched.cpp

@@ -429,6 +432,14 @@ bool DMCBatched::run()
  IndexType num_blocks = qmcdriver_input_.get_max_blocks();

  estimator_manager_->startDriverRun();
+
+  //start walker log manager
+  wlog_manager_ = std::make_unique<WalkerLogManager>(walker_logs_input, allow_walker_logs, get_root_name(), myComm);


The idea of the log manager being a run scope object is promising but then it should not be a member variable.

Good point. We may get a chance to make it a run scope object after restructuring certain bits. Will give it a try once #5039 got merged.

jtkrogel · 2024-06-13T13:29:08Z

Hi guys. I had thought this was going in given Ye's approval.

If there really is a short list of "must have" items still remaining for this PR after all these passes, please make a short, final list below and I will address them.

I need to get past this point and get back to other work, some of which depends on this code being in place.

ye-luo

I don’t feel there are blocking issues although many improvements are still needed.

PDoakORNL · 2024-06-13T16:59:50Z

Reverting the whitespace OperatorBase.cpp change to reduce the number files is the only must have.

ye-luo · 2024-06-13T18:28:53Z

Test this please

The requested renaming has been completed.

jtkrogel added 30 commits May 20, 2024 16:08

first working impl of TraceManager copy

e36d329

remove unused functions

abf1634

remove combined trace use

8a885a1

remove combined traces entirely

ca70bc8

start trace collector

5b8072b

hot swap trace collector for trace manager

d21b2ce

remove unused code

1545230

remove buffers from master

875ee62

remove unused data members

e0fcd9b

fix dmc walker traces

6e1ad00

consolidate state

d4edf72

operable buffers

6aeb13c

wtrace buffers work

b0b9175

roll tests over to walker traces

a1b48c2

remove old scaffolding

586c7db

cleaning

6fdcae6

update trace collector names

77bfcbd

walker trace manager names

c50cbfc

walker trace manager names, continued

4c881ef

revoke prejudice

dceab3a

migrate to cpp, cleanup includes

e7544be

implementation of min/max/median energy walker data buffering

75f2aa1

fix min/max/med buffering, works now

ecfdca8

cleanup api

6c9c7f4

enable selective data write

196a4b4

banish put()

00986dc

cleanup

4bb5275

fix header

3b92dc8

cleanup api

9d7128a

port to batched drivers

61b0f3c

Correct CMake and include.

157dda3

ye-luo reviewed Jun 11, 2024

View reviewed changes

jtkrogel added 4 commits June 11, 2024 09:56

rename step counter

c792aff

longer name for crowd.collect()

4a7d770

hide data members behind more member functions

35871ac

simplify WalkerLogState

740cf6b

ye-luo previously approved these changes Jun 11, 2024

View reviewed changes

ye-luo added 2 commits June 11, 2024 12:50

Relocate wlog_collector_.startBlock

be01ba4

Make wlog_collector_ private in Crowd.

a832b7c

ye-luo dismissed their stale review via a832b7c June 11, 2024 18:21

ye-luo mentioned this pull request Jun 11, 2024

Improve the integration of WalkerLog classes in batched drivers #5039

Merged

PDoakORNL requested changes Jun 12, 2024

View reviewed changes

ye-luo approved these changes Jun 13, 2024

View reviewed changes

PDoakORNL approved these changes Jun 13, 2024

View reviewed changes

Merge branch 'develop' into walker_traces

9408943

ye-luo enabled auto-merge June 13, 2024 18:51

ye-luo merged commit a1cdeb2 into QMCPACK:develop Jun 13, 2024
38 checks passed

PDoakORNL mentioned this pull request Jun 26, 2024

MCPopulation now gives walkers useful walker and parent ids #5063

Merged

prckent mentioned this pull request Jun 28, 2024

dump walkers periodically for post-processing #4940

Open

jtkrogel deleted the walker_traces branch November 19, 2024 22:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Driver-portable walker logging #5019

Driver-portable walker logging #5019

jtkrogel commented May 31, 2024 •

edited

Loading

ye-luo Jun 11, 2024

prckent Jun 11, 2024 •

edited

Loading

jtkrogel Jun 11, 2024 •

edited

Loading

prckent Jun 11, 2024

jtkrogel Jun 11, 2024

ye-luo Jun 11, 2024

jtkrogel Jun 11, 2024

ye-luo Jun 11, 2024

ye-luo Jun 11, 2024

jtkrogel Jun 11, 2024

ye-luo commented Jun 11, 2024

jtkrogel commented Jun 11, 2024

jtkrogel commented Jun 11, 2024

jtkrogel commented Jun 11, 2024

ye-luo commented Jun 11, 2024 •

edited

Loading

ye-luo left a comment

ye-luo commented Jun 11, 2024

PDoakORNL left a comment

PDoakORNL Jun 5, 2024

PDoakORNL Jun 13, 2024

PDoakORNL Jun 12, 2024

ye-luo Jun 12, 2024

PDoakORNL Jun 12, 2024

ye-luo Jun 12, 2024

PDoakORNL Jun 13, 2024

PDoakORNL Jun 12, 2024

PDoakORNL Jun 12, 2024

PDoakORNL Jun 12, 2024

PDoakORNL Jun 12, 2024

ye-luo Jun 12, 2024

jtkrogel commented Jun 13, 2024

ye-luo left a comment

PDoakORNL commented Jun 13, 2024

ye-luo commented Jun 13, 2024

Driver-portable walker logging #5019

Driver-portable walker logging #5019

Conversation

jtkrogel commented May 31, 2024 • edited Loading

Proposed changes

What type(s) of changes does this code introduce?

Does this introduce a breaking change?

What systems has this change been tested on?

Checklist

Path out of WIP

Tasks for review round 3

Elements for reviewers to review

Choose a reason for hiding this comment

prckent Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

jtkrogel Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ye-luo commented Jun 11, 2024

jtkrogel commented Jun 11, 2024

jtkrogel commented Jun 11, 2024

jtkrogel commented Jun 11, 2024

ye-luo commented Jun 11, 2024 • edited Loading

ye-luo left a comment

Choose a reason for hiding this comment

ye-luo commented Jun 11, 2024

PDoakORNL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtkrogel commented Jun 13, 2024

ye-luo left a comment

Choose a reason for hiding this comment

PDoakORNL commented Jun 13, 2024

ye-luo commented Jun 13, 2024

jtkrogel commented May 31, 2024 •

edited

Loading

prckent Jun 11, 2024 •

edited

Loading

jtkrogel Jun 11, 2024 •

edited

Loading

ye-luo commented Jun 11, 2024 •

edited

Loading