Global driver_version switch for mapping input to drivers #3897

PDoakORNL · 2022-03-04T21:02:08Z

Note: After discussion and review, the switch was decided to be named driver_version (not epoch)

This removes the

<qmc method=vmc_batch|dmc_batch|linear_batch>

and replaces them with

    <project id="qmc_short" series="0">
     <parameter name="driver_version">batched</parameter>
   </project>
...
<qmc method=vmc|dmc|linear>

Proposed changes

ProjectData can now be queried get_driver_epoch() and explicitly different input parsing can easily be done for batched drivers vs. legacy.

The unit tests pass.
Most of the short and deterministic batched tests pass. I'm not sure any of the system level batched optimizer test pass. They seem extremely slow.

In the future this will greatly simplify preventing legacy/new input mismatches. i.e. #3875

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

New feature
Code style update (formatting, renaming)
Refactoring (no functional changes, no api changes)
Testing changes (e.g. new unit/integration/performance tests)
Documentation changes

Does this introduce a breaking change?

Yes
For legacy no, for batched yes.
Your existing batched driver input will stop working.
You must make the change shown at the top of the PR to have them continue working.

What systems has this change been tested on?

Leconte

Checklist

Update the following with a yes where the items apply. If you're unsure about any of them, don't hesitate to ask. This is
simply a reminder of what we are going to look for before merging your code.

Yes/No. This PR is up to date with current the current state of 'develop'
Yes/No. Code added or changed in the PR has been clang-formatted
Yes/No. This PR adds tests to cover any new code, or to catch a bug that is being fixed
Yes/No. Documentation has been added (if appropriate)

ye-luo · 2022-03-04T21:15:10Z

I still would like to run xxx_batch. I thought the intended behavior was when the global switch is on. "XXX" is redirected to "XXX_batch".

PDoakORNL · 2022-03-04T21:19:46Z

I thought the desire was to drop the vmc_batch name. @jtkrogel convinced me that not having the second driver name was better and it definitely is considering the legacy handling of input. ProjectData is high level and parsed very early, we need to parse qmchamiltonian differently based on whether the driver is batched or not and this greatly simplifies that.

prckent · 2022-03-04T21:19:54Z

xxx_batch will be history. This is the intended change. Much friendlier for users and we want to block off the old drivers in any case.

I think we should try to improve on "driver_epoch". There is a lot more than the driver being switched here, plus this tag will be visible for "eternity" given the transition plan. Ask on Slack for ideas.

jtkrogel · 2022-03-04T21:34:55Z

Any chance we change <parameter name="driver_epoch">batched</parameter> to <parameter name="driver">batched</parameter>? It is about as simple as is needed.

Nexus currently uses driver='batched' and driver='legacy' to switch between these. Whatever is decided, I will update Nexus to match.

prckent · 2022-03-04T21:47:37Z

Nexus currently uses driver='batched' and driver='legacy' to switch between these. Whatever is decided, I will update Nexus to match.

Any arguments to not use these? Consistency is a strong argument for.

markdewing · 2022-03-04T21:52:13Z

src/QMCDrivers/tests/ValidQMCInputSections.h

@@ -103,13 +103,14 @@ constexpr std::array<const char*, 2> valid_dmc_input_sections{
 constexpr int valid_dmc_input_dmc_index       = 0;
 constexpr int valid_dmc_input_dmc_batch_index = 1;

+/** As far as I can tell these are no longer valid */


I need to update the WFOptDriverInput class, and I'll change these tests then as well.

ye-luo · 2022-03-04T22:20:35Z

Technically, determine the behavior fully relying on a global state is convenient but bad both for the code parsing and human reading.

As a user, I request the existing input with "vmc_batch" to continue functioning. It is bad to upset users making breaking changes. The cost of keep "vmc_batch" tag to work is nothing and there is no ambiguity.

Here is what consider a sane way.
When "vmc_legacy" or "vmc_batch" is inputted as a method, it should hit exactly the requested driver.
Only when the input is "vmc", driver factory maps it to "vmc_legacy" or "vmc_batch" based on the global state.

For users new to this complexity, they may choose to just rely on the global flag. This is also a desired method to help migration.

Once the legacy vmc driver is deleted, then we can remove "vmc_legacy/batch", there is no ambiguity.

prckent · 2022-03-07T14:10:12Z

Will post with a suggested transition plan when I have more time this week. I think we could support _batch but with a warning that this will be deprecated in future, similar to how we need a warning when the global flag is not specified. Once Nexus is updated no one should be invoking xxx_batch in any new inputs.

PDoakORNL · 2022-03-07T15:41:59Z

Not having a high level switch between the legacy architecture and the new architecture makes it quite torturous to reject legacy estimators in the qmchamiltonian section as requested in #3875. I support introducing separate code paths for input parsing as the legacy input handling is a mess that is tightly entangled with the legacy simulation objects.

We can support the current _batch as they are now but I don't think we should make the sphaghetti to go back and check previous parsed and constructured objects like the hamiltonian pool.

ye-luo · 2022-03-07T15:53:43Z

Not having a high level switch between the legacy architecture and the new architecture makes it quite torturous to reject legacy estimators in the qmchamiltonian section as requested in #3875.

We do need a high level switch. We agreed on that. I was asking to keep "_batch" functioning. It is only a key mapping issue.

ye-luo · 2022-03-09T16:27:17Z

src/QMCDrivers/QMCDriverFactory.cpp

  if (curName != "qmc")
    qmc_mode = curName;

  const int nchars = qmc_mode.size();
+
+  // Begin to separate batch input reading from the legacy input parsing
+  if (project_data_.get_driver_epoch() == "batched")


Could you add a bool argument (force_batch) of readSection for and expand unit tests of readSection.
Also check getEngineName() after the driver being created in unit tests.

Since you cannot create a QMCDriverFactory without ProjectData and the global driver epoch is set there, I don't think its a good idea to also allow it to be forced directly. There is one source of truth for this parameter and it is from the ProjectData the driver factory is constructed with.

The _batch are supported for the convenience of users and tests currently using them but are not valid input when the driver epoch is set. Based on the driver epoch input, the qmcsystem and hamiltonian node will be parsed differently so we don't want to encourage further use of the _batch or introduce _legacy as a tag for drivers.

I forgot that ProjectData is required by QMCDriverFactory. Then there is no need to add a bool argument. I have not read your latest commit but I guess you have both epoch cases covered in the unit test now. What did you mean by "the qmcsystem and hamiltonian node will be parsed differently" did you mean "hamiltonian of qmcsystem" ?

We are also going to allow an <estimators> node for global estimator definitions. This will only be valid if in the the batched epoch.

I see. global "estimators" will outside "hamiltonian" but inside "qmcsystem"

PDoakORNL · 2022-03-09T21:10:14Z

Having driver refer to a classification for a group of drivers when its a parameter of a project node, but still having singular drivers types: vmc, linear, dmc in the code, documentation, name attibute of qmc node, or when the node name of a qmc section seems like recipe for confusion.

ye-luo · 2022-03-10T15:57:01Z

@PDoakORNL could you run clang-format on all the changed source file ? Some diff looks strange.
I will get you back the review early this afternoon.

ye-luo

The driver factory change looks good.
Having concerns on the changes in ProjectData.

src/QMCDrivers/tests/test_QMCDriverFactory.cpp

src/Utilities/ProjectData.h

ye-luo

See my comments around ProjectData class change.

src/Utilities/tests/test_project_data.cpp

src/Utilities/ProjectData.h

tests/molecules/He_ae/det_He_opt_batch.xml

src/Utilities/ProjectData.cpp

ye-luo · 2022-03-16T17:59:50Z

Test this please

prckent

What did we end up with? I could not determine from the discussion here.

PDoakORNL · 2022-03-16T21:45:03Z

Test this please

ye-luo · 2022-03-16T23:58:25Z

Test this please

ye-luo · 2022-03-17T00:02:02Z

Test this please

…to global_batch_switch

markdewing · 2022-03-17T20:05:00Z

src/QMCDrivers/QMCDriverFactory.h

+   *  In the application context project data can indicate the input be read in the context of
+   *  the batched driver architecture.
+   *  param[in] cur            qmc section node
+   *  param[in] force_batch    forces input to be evaluated as if project driver type = batched


There doesn't seem to be force_batch parameter.

markdewing · 2022-03-17T20:06:37Z

src/QMCDrivers/QMCDriverFactory.h

@@ -48,12 +48,23 @@ class QMCDriverFactory
    QMCRunType new_run_type = QMCRunType::DUMMY;
  };

+  /** Application uses this constructor
+   *  param[in] project_data    this is stored as a reference and this state controls later behavior.
+   *                            For both the driver factory i.e. driver epoch. And the drivers it creates


Leftover reference to 'driver epoch'

markdewing · 2022-03-17T20:07:06Z

src/Utilities/ProjectData.h

@@ -113,6 +132,9 @@ class ProjectData : public OhmmsElementBase

  ///max cpu seconds
  int max_cpu_secs_;
+
+  // The driver epoch of the project


left over mention of epoch

Thanks for catching this Mark. We should confirm grep -i epoch returns nothing.

PDoakORNL · 2022-03-17T21:32:33Z

my refactor didn't reach into the comments. Did a straight text based recursive grep and I don't see any epoch's related to this only the time api.

ye-luo · 2022-03-17T22:57:29Z

Test this please

ye-luo

LGTM. @prckent good for you now?

prckent

I think we are missing a test for the case where driver_version is not specified but vmc_batch is called explicitly (or one of the other _batched drivers). This should work currently but in future when the driver_version tag is made a requirement will break (and be updated to expect_fail status). A quick clone of one of the deterministic tests would handle this.

prckent

The no switch + *_batch call is already tested. (This is what current users of the batched code will have in their inputs).

PDoakORNL added 5 commits March 4, 2022 11:34

add global parameter for driver_epoch, clean ProjectData a bit

07b4b03

cleanup and testing

83e30a8

support global driver epoch switch

c34bfd4

updating system tests for ecpoch_driver

99f8579

fixing linear_batch system tests

78119d5

missed test update.

6dfe6a9

markdewing reviewed Mar 4, 2022

View reviewed changes

return vmc_batch, dmc_batch, linear_batch to previous functionality

95aa7dc

ye-luo reviewed Mar 9, 2022

View reviewed changes

better unit test for batched driver creation

df8f3e6

fixing bad input in test xml

7a802d9

ye-luo added 2 commits March 10, 2022 15:57

Formatting.

01d1f95

Merge remote-tracking branch 'origin/develop' into pr/origin-3897

541c257

ye-luo reviewed Mar 10, 2022

View reviewed changes

src/QMCDrivers/tests/test_QMCDriverFactory.cpp Outdated Show resolved Hide resolved

src/Utilities/ProjectData.h Outdated Show resolved Hide resolved

src/Utilities/ProjectData.h Outdated Show resolved Hide resolved

ye-luo changed the title ~~Global batch switch~~ Global batched driver switch Mar 15, 2022

refactored repetive code

855d249

ye-luo requested changes Mar 15, 2022

View reviewed changes

src/Utilities/tests/test_project_data.cpp Outdated Show resolved Hide resolved

src/Utilities/ProjectData.h Outdated Show resolved Hide resolved

tests/molecules/He_ae/det_He_opt_batch.xml Outdated Show resolved Hide resolved

src/Utilities/ProjectData.cpp Outdated Show resolved Hide resolved

PDoakORNL added 2 commits March 15, 2022 18:20

remove name from ProjectData use enum for DriverEpoch

fe02def

fix default constructor

2cb25fa

PDoakORNL and others added 2 commits March 16, 2022 11:42

remove unneeded include from ProjectData

3531272

Merge branch 'develop' into global_batch_switch

35fcde1

ye-luo previously approved these changes Mar 16, 2022

View reviewed changes

prckent self-requested a review March 16, 2022 18:33

prckent requested changes Mar 16, 2022

View reviewed changes

Update QMCDriverFactory.h

e6bc85f

ye-luo dismissed their stale review via e6bc85f March 17, 2022 00:01

ye-luo previously approved these changes Mar 17, 2022

View reviewed changes

PDoakORNL added 2 commits March 17, 2022 13:15

change driver epoch ->> driver version

48ad456

Merge branch 'global_batch_switch' of github.com:PDoakORNL/qmcpack in…

4381ad3

…to global_batch_switch

PDoakORNL dismissed ye-luo’s stale review via 4381ad3 March 17, 2022 17:29

typo

5338776

markdewing reviewed Mar 17, 2022

View reviewed changes

remove final uses of epoch update copyright

eae3545

PDoakORNL disabled auto-merge March 17, 2022 21:29

ye-luo approved these changes Mar 17, 2022

View reviewed changes

prckent self-requested a review March 18, 2022 02:14

prckent requested changes Mar 18, 2022

View reviewed changes

prckent approved these changes Mar 18, 2022

View reviewed changes

prckent merged commit 750d260 into QMCPACK:develop Mar 18, 2022

ye-luo changed the title ~~Global batched driver switch~~ Global driver_version switch for mapping input to drivers Mar 25, 2022

prckent mentioned this pull request Mar 25, 2022

Update batched driver documentation to include driver_version #3915

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Global driver_version switch for mapping input to drivers #3897

Global driver_version switch for mapping input to drivers #3897

PDoakORNL commented Mar 4, 2022 •

edited by ye-luo

Loading

ye-luo commented Mar 4, 2022

PDoakORNL commented Mar 4, 2022

prckent commented Mar 4, 2022

jtkrogel commented Mar 4, 2022

prckent commented Mar 4, 2022

markdewing Mar 4, 2022

ye-luo commented Mar 4, 2022 •

edited

Loading

prckent commented Mar 7, 2022 •

edited

Loading

PDoakORNL commented Mar 7, 2022

ye-luo commented Mar 7, 2022 •

edited

Loading

ye-luo Mar 9, 2022

PDoakORNL Mar 9, 2022

ye-luo Mar 9, 2022

PDoakORNL Mar 9, 2022

ye-luo Mar 10, 2022

PDoakORNL commented Mar 9, 2022

ye-luo commented Mar 10, 2022

ye-luo left a comment

ye-luo left a comment

ye-luo commented Mar 16, 2022

prckent left a comment

PDoakORNL commented Mar 16, 2022

ye-luo commented Mar 16, 2022

ye-luo commented Mar 17, 2022

markdewing Mar 17, 2022

markdewing Mar 17, 2022

markdewing Mar 17, 2022

prckent Mar 17, 2022

PDoakORNL commented Mar 17, 2022

ye-luo commented Mar 17, 2022

ye-luo left a comment

prckent left a comment

prckent left a comment

Global driver_version switch for mapping input to drivers #3897

Global driver_version switch for mapping input to drivers #3897

Conversation

PDoakORNL commented Mar 4, 2022 • edited by ye-luo 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

ye-luo commented Mar 4, 2022

PDoakORNL commented Mar 4, 2022

prckent commented Mar 4, 2022

jtkrogel commented Mar 4, 2022

prckent commented Mar 4, 2022

Choose a reason for hiding this comment

ye-luo commented Mar 4, 2022 • edited Loading

prckent commented Mar 7, 2022 • edited Loading

PDoakORNL commented Mar 7, 2022

ye-luo commented Mar 7, 2022 • 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

PDoakORNL commented Mar 9, 2022

ye-luo commented Mar 10, 2022

ye-luo left a comment

Choose a reason for hiding this comment

ye-luo left a comment

Choose a reason for hiding this comment

ye-luo commented Mar 16, 2022

prckent left a comment

Choose a reason for hiding this comment

PDoakORNL commented Mar 16, 2022

ye-luo commented Mar 16, 2022

ye-luo commented Mar 17, 2022

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

PDoakORNL commented Mar 17, 2022

ye-luo commented Mar 17, 2022

ye-luo left a comment

Choose a reason for hiding this comment

prckent left a comment

Choose a reason for hiding this comment

prckent left a comment

Choose a reason for hiding this comment

PDoakORNL commented Mar 4, 2022 •

edited by ye-luo

Loading

ye-luo commented Mar 4, 2022 •

edited

Loading

prckent commented Mar 7, 2022 •

edited

Loading

ye-luo commented Mar 7, 2022 •

edited

Loading