Checkpointing (quasi-Newton solver) #693

cnpetra · 2024-08-28T18:28:12Z

Adds API for checkpointing and support for restart via user options.

closes #686

display iteration number from before restart
provide example for the load/save API
update user manual
metadata

code compiles; does nothing

checkpoint interface complete additional states need to be saved

tepperly

I'll check more later.

src/Optimization/hiopAlgFilterIPM.hpp

src/Optimization/hiopAlgFilterIPM.cpp

tepperly

I think this is all my feedback.

src/Optimization/hiopAlgFilterIPM.cpp

tepperly · 2024-09-04T18:37:10Z

src/Utils/hiopOptions.cpp


+    constexpr char msgcf[] = "Path to checkpoint file. If present character '#' will be replaced "
+      "with the iteration number.";


The implementation only replaces the first occurrence of '#'.

src/Optimization/hiopAlgFilterIPM.cpp

tepperly · 2024-09-04T20:53:30Z

I thought about it some more over lunch, and I think load_state_from_data_store() and save_state_to_data_store() should take a ::axom::sidre::Group & as it's argument rather than a ::axom::sidre::DataStore &. The Group argument should specify the group where the save/load should take place from rather than it being a fixed location relative to the root DataStore.

You can get the DataStore from the Group in case there is a need for that.

IMHO, this is an improvement because if HiOp is part of a complex program, the outer program may want to encapsulate the HiOp part of the checkpoint in some group that's potentially several levels down from the DataStore root group. It also leaves the door open for their to be multiple instances of HiOp active in the running program that all need to be checkpointed. The current design can only support checkpointing of one HiOp instance in a given DataStore.

cnpetra · 2024-09-05T12:36:12Z

I thought about it some more over lunch, and I think load_state_from_data_store() and save_state_to_data_store() should take a ::axom::sidre::Group & as it's argument rather than a ::axom::sidre::DataStore &. The Group argument should specify the group where the save/load should take place from rather than it being a fixed location relative to the root DataStore.

You can get the DataStore from the Group in case there is a need for that.

IMHO, this is an improvement because if HiOp is part of a complex program, the outer program may want to encapsulate the HiOp part of the checkpoint in some group that's potentially several levels down from the DataStore root group. It also leaves the door open for their to be multiple instances of HiOp active in the running program that all need to be checkpointed. The current design can only support checkpointing of one HiOp instance in a given DataStore.

I take it that load_state_from_data_store() and save_state_to_data_store() "APIs" would do the job on your end. I see the flexibility of working with the sidre::group and will make the change. Will have you as a reviewer to have a final look when the PR ready.

tepperly · 2024-09-05T16:03:25Z

src/Optimization/hiopAlgFilterIPM.cpp

+{
+  using IndType = sidre::IndexType;
+  const IndType size = vec->get_local_size();
+  sidre::View* dest = nlp_group->createViewAndAllocate(name, sidre::DOUBLE_ID, size);


In LiDO, we often use the same DataStore for the whole run. If we were to call this multiple times, I think it will fail because the views would already exist.
Right now this method always makes a new view. It would be better if it checked first if the view already exists. If it already exists in the Group, use that one. If it doesn't already exist, create a new view.

last commit does that. note that an exception will be thrown if the view exists and has a different number of elements than HiOp's state variable.

nychiang

Add minor comments

src/Optimization/hiopAlgFilterIPM.hpp

src/Utils/hiopOptions.cpp

nychiang · 2024-09-06T00:27:20Z

src/Optimization/hiopAlgFilterIPM.cpp

+
+}
+
+void hiopAlgFilterIPMQuasiNewton::checkpointing_stuff()


Style Guidelines.
There should be spaces before and after each operator, e.g. line 1675, 1655,...

Style Guidelines. There should be spaces before and after each operator, e.g. line 1675, 1680,...

can you be more specific about the guideline?

I mean space before and after operator "==".

Our CMake build that uses BLT auto formats everything using clang-format (and a configuration file) with a make style. Also make check verifies that the code matches the clang-format style configuration, so a PR can't be merged without complying with the style.

src/Optimization/hiopAlgFilterIPM.cpp

tepperly · 2024-09-09T23:37:16Z

I thought about it some more over lunch, and I think load_state_from_data_store() and save_state_to_data_store() should take a ::axom::sidre::Group & as it's argument rather than a ::axom::sidre::DataStore &. The Group argument should specify the group where the save/load should take place from rather than it being a fixed location relative to the root DataStore.
You can get the DataStore from the Group in case there is a need for that.
IMHO, this is an improvement because if HiOp is part of a complex program, the outer program may want to encapsulate the HiOp part of the checkpoint in some group that's potentially several levels down from the DataStore root group. It also leaves the door open for their to be multiple instances of HiOp active in the running program that all need to be checkpointed. The current design can only support checkpointing of one HiOp instance in a given DataStore.

I take it that load_state_from_data_store() and save_state_to_data_store() "APIs" would do the job on your end. I see the flexibility of working with the sidre::group and will make the change. Will have you as a reviewer to have a final look when the PR ready.

The load/save methods offer the most flexibility for LiDO and its clients. I haven't double checked that we have the right HiOp object handle to call them.

cnpetra · 2024-09-11T21:25:06Z

I thought about it some more over lunch, and I think load_state_from_data_store() and save_state_to_data_store() should take a ::axom::sidre::Group & as it's argument rather than a ::axom::sidre::DataStore &. The Group argument should specify the group where the save/load should take place from rather than it being a fixed location relative to the root DataStore.
You can get the DataStore from the Group in case there is a need for that.
IMHO, this is an improvement because if HiOp is part of a complex program, the outer program may want to encapsulate the HiOp part of the checkpoint in some group that's potentially several levels down from the DataStore root group. It also leaves the door open for their to be multiple instances of HiOp active in the running program that all need to be checkpointed. The current design can only support checkpointing of one HiOp instance in a given DataStore.

I take it that load_state_from_data_store() and save_state_to_data_store() "APIs" would do the job on your end. I see the flexibility of working with the sidre::group and will make the change. Will have you as a reviewer to have a final look when the PR ready.

The load/save methods offer the most flexibility for LiDO and its clients. I haven't double checked that we have the right HiOp object handle to call them.

LiDO for sure instantiates the algorithm class somewhere in there. That "algorithm" object should be used. For example, attach it to the interface. Will provide an example in this PR.

cnpetra · 2024-09-14T00:25:56Z

@tepperly please take a look here and here at the use of the load/save API

tepperly · 2024-09-23T02:59:03Z

I'll try to look at it early this week. It's PA season.

…

On Sun, Sep 22, 2024, 5:14 PM Cosmin G Petra ***@***.***> wrote: @cnpetra <https://github.com/cnpetra> requested your review on: #693 <#693> Checkpointing (quasi-Newton solver). — Reply to this email directly, view it on GitHub <#693 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACM3KAWWTXVWUBXXPWCVWLZX5MNFAVCNFSM6AAAAABNI3FQ5WVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJUGM2TKOJTGQ2DGMI> . You are receiving this because your review was requested.Message ID: ***@***.***>

nychiang

some minor comments.

CMakeLists.txt

src/Optimization/hiopKKTLinSys.cpp

src/Optimization/hiopAlgFilterIPM.hpp

src/Optimization/hiopHessianLowRank.hpp

src/Optimization/hiopHessianLowRank.cpp

src/Optimization/hiopAlgFilterIPM.cpp

tepperly · 2024-09-25T16:08:00Z

@tepperly please take a look here and here at the use of the load/save API

I looked over it again, and it looks good to me.

cnpetra added 2 commits August 26, 2024 15:38

added notes and some drafty interface

7a939b7

added draft of the api for checkpointing

181f7f9

cnpetra marked this pull request as draft August 28, 2024 18:28

cnpetra added 8 commits August 28, 2024 11:30

fixed compilation issues

0e428f5

integrated AXOM

354b82b

code compiles; does nothing

added user options for checkpointing

a14a445

more work on load checkpoint EOD

d497955

Merge branch 'develop' into chkpnt-dev

8b2342c

semi-operation checkpointing

d4900a9

checkpoint interface complete additional states need to be saved

removed save checkpoint callback from the interface

3579828

fixed typos in comments

d213774

tepperly reviewed Sep 4, 2024

View reviewed changes

tepperly reviewed Sep 5, 2024

View reviewed changes

nychiang reviewed Sep 6, 2024

View reviewed changes

cnpetra added 2 commits September 6, 2024 17:03

moved sidre-related code from Algorithm class to a "utils" helper

4a9fbf1

switched to refs; some testing of options-based checkpointing

0498564

cnpetra added 2 commits September 11, 2024 14:09

added sidre copy to/from dense matrices

085eb88

instrumentation for saving quasi-Newton internals to sidre

b21c3c5

updated iteration counter to keep track of total number over restarts

bfe1b40

cnpetra changed the title ~~Checkpoiting (quasi-Newton solver)~~ Checkpointing (quasi-Newton solver) Sep 13, 2024

cnpetra added 5 commits September 13, 2024 08:01

updated doc; replace all #

5bc2af6

added example on how to use checkpoint API

a664e35

clean up

261ccf9

added metadata

0506d38

testing and clean up

b785475

cnpetra added 6 commits September 22, 2024 15:56

Merge branch 'develop' into chkpnt-dev

adf30e6

update user manual with checkpointing

4e673d5

updated pdf user manual

77c88a2

fix ci errors (compilation)

5631b6c

fix adtl compilation issues

ea5cafd

fixed compil error

28c4567

cnpetra marked this pull request as ready for review September 23, 2024 00:13

cnpetra requested review from tepperly and nychiang September 23, 2024 00:13

nychiang approved these changes Sep 23, 2024

View reviewed changes

addresed reviews

27d15f2

cnpetra merged commit 22efbe8 into develop Sep 27, 2024
5 of 6 checks passed

cnpetra mentioned this pull request Oct 3, 2024

Consolidating qn solver #697

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Checkpointing (quasi-Newton solver) #693

Checkpointing (quasi-Newton solver) #693

cnpetra commented Aug 28, 2024 •

edited

Loading

tepperly left a comment

tepperly left a comment

tepperly Sep 4, 2024

tepperly commented Sep 4, 2024

cnpetra commented Sep 5, 2024

tepperly Sep 5, 2024

cnpetra Sep 8, 2024

nychiang left a comment

nychiang Sep 6, 2024 •

edited

Loading

cnpetra Sep 8, 2024

nychiang Sep 9, 2024

tepperly Sep 9, 2024

tepperly commented Sep 9, 2024

cnpetra commented Sep 11, 2024

cnpetra commented Sep 14, 2024 •

edited

Loading

tepperly commented Sep 23, 2024 via email

nychiang left a comment

tepperly commented Sep 25, 2024


		constexpr char msgcf[] = "Path to checkpoint file. If present character '#' will be replaced "
		"with the iteration number.";


		}

		void hiopAlgFilterIPMQuasiNewton::checkpointing_stuff()

Checkpointing (quasi-Newton solver) #693

Checkpointing (quasi-Newton solver) #693

Conversation

cnpetra commented Aug 28, 2024 • edited Loading

tepperly left a comment

Choose a reason for hiding this comment

tepperly left a comment

Choose a reason for hiding this comment

tepperly Sep 4, 2024

Choose a reason for hiding this comment

tepperly commented Sep 4, 2024

cnpetra commented Sep 5, 2024

tepperly Sep 5, 2024

Choose a reason for hiding this comment

cnpetra Sep 8, 2024

Choose a reason for hiding this comment

nychiang left a comment

Choose a reason for hiding this comment

nychiang Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

cnpetra Sep 8, 2024

Choose a reason for hiding this comment

nychiang Sep 9, 2024

Choose a reason for hiding this comment

tepperly Sep 9, 2024

Choose a reason for hiding this comment

tepperly commented Sep 9, 2024

cnpetra commented Sep 11, 2024

cnpetra commented Sep 14, 2024 • edited Loading

tepperly commented Sep 23, 2024 via email

nychiang left a comment

Choose a reason for hiding this comment

tepperly commented Sep 25, 2024

cnpetra commented Aug 28, 2024 •

edited

Loading

nychiang Sep 6, 2024 •

edited

Loading

cnpetra commented Sep 14, 2024 •

edited

Loading