Fixes for DMRG test suite and related bugs.

[ShaunWeatherly]

Major Changes:

• Modifies the tests in dmrg_molbe_test to run in github actions.
• Exposes useful scratch management options to high level modules, iembe.BE. Namely: one can now specify scratch_dir and cleanup_at_end when instantiating the BE object.
• Adds missing DMRG modules to be_parallel.

Specifics:

• Fixes to mbe.BE.scratch_dir. This keyword was broken by the recent changes to scratch management - now it is correctly passed to WorkDir and used to generate the scratch root.
• Improved caching for different parts of the code in quemb_unittest.yml. Currently separated into pip-cache, quemb-cache and block2-cache. Also makes cache keys more descriptive, which fixes a bug where caches failed to update despite dependencies changing.
• DMRG scratch cleanup now relies on WorkDir.cleanup_at_end, and the old kwarg (force_cleanup) has been removed.
• Fixes reference to rets[0] as the total BE energy in solver, which is incorrect.

Full commit history ~

• Fixes for scratch management in solver and helper modules.

• Added DMRG_ArgsUser to TestBE_DMRG

• Fixed incorrect definition of ebe_tot as rets[0], which is only the correlation energy.

• Make helper.delete_multiple_files recursive.

• Make use_cumulant=True by default in solver_block2.

• Modify molecular_DMRG_test

• Remove block2 versioning

• Test explicit cache save/restore.

• Fully explicit save/restore for cache

• Block2 no-binary

• Test no-binary

• Typo

• Explicit block2 versioning

• Move to test_requirements

• Edit block2 versioning.

• Force latest mkl

• test latest mkl

• Separate caching for block2

• Test caching

• Test caching

• Fine tune caching

• Further fine-tune caching

• Back to block2 versioning

• Formatting and ompnum passing.

• Install quemb in workflows.

• resolve molecular_DMRG_test scratch

• Update example script

• Update .gitignore

• Enable all unittests

---

[ShaunWeatherly]

Note to @lweisburn and @mscho527 , this PR corrects the definition of BE.ebe_tot to be the total BE energy. Previously, some parts of the code were using it as the correlation energy, including in the misc.be2puffin function. I've updated that function to explicitly use the correlation energy (which I assume was the original intent) and I just wanted to double check and make sure I didn't break anything!

[mscho527]

I haven't read the details, but this seems like a substantial change in the code with a single PR. Shall we ask 2+ approvals as we have done for previous large PRs? Also, it will take a bit for me to parse through this.

[mcocdawc]

Thanks for making this test work!

See my comments below

I would await a comment of @mscho527 or @lweisburn regarding the adjusted energy

[mcocdawc]

the cast to Path is fully redundant and I would omit it.

WorkDir is quite flexible in its input arguments and accepts PathLike, i.e. even strings and
scratch_dir / dname returns a Path anyway.

The passing of cleanup_at_end is redundant, since if scratch_dir.cleanup_at_end is True it will remove subdirectories as well.

[ShaunWeatherly]

cleanup_at_end is not redundant, just consider the converse of your example: if scratch_dir.cleanup_at_end is False, one would also like the fragment scratch directories to have scratch_dir.cleanup_at_end to be False, whereas by default they would be set to True and the fragment scratch directories would be removed despite the user setting cleanup_at_end to False.

[mscho527]

Are you @ShaunWeatherly creating WorkDir objects for each fragment? Wouldn't we want one WorkDir object (i.e. molbe.scratch_dir and create subdirectories (dname) under here? Unless we want granular control over the cleanups (i.e. we want to clean up fragment 0 but leave fragment 1), I don't think multiple instances of WorkDir would be necessary?

[mcocdawc]

having a normal subdirectory would also not force you to keep track of scratch_dir.cleanup_at_end.
If scratch_dir.cleanup_at_end is True the subdirectory will be deleted anyway and if scratch_dir.cleanup_at_end is False, then it will be preserved.

[ShaunWeatherly]

Sure, one could just use PathLikes for the fragment scratch directories, but I think using WorkDir instead keeps the code more internally consistent. The most elegent workaround would be to add a create_subdirectory class method to WorkDir, which inherits the properties of the base class.

[mscho527]

I left some comments regarding some questions and confusions, but looks solid so far!

[mscho527]

Shouldn't pip upgrade happen regardless of the cache hit?

[mscho527]

Maybe I'm wrong, but isn't this potentially dangerous? I think we want to test installing quemb everytime, even if it adds a few seconds to the test suite. Since we are testing quemb, I don't feel comfortable caching it based on hashes that only look at specific files.

[ShaunWeatherly]

This cache is really just for the dependencies, we still run pip install . at the beginning of the testsuite to install quemb itself. I can change the name if it's misleading.

[mscho527]

Yes. I think the name is misleading. If the purpose is to just install the dependencies, should line 124 (pip install .) be removed as well?

[mscho527]

Does this key depend on the contents of block2 repository? In other words, if something changes on pyscf/dmrgscf or pypi release of block2, does this key logic distinguish the change?

[ShaunWeatherly]

Those changes would need to be reflected in the list of test_requirements.txt dependencies, but otherwise no explicit information from block2 is used for key generations. This is also true for all of the other dependencies, however.

Ie., if the versioning for libdmet changes, for example, the cache key technically will not change unless we update setup.py with the new versioning.

[mscho527]

Okay, this raises a good point then. @mcocdawc If we want to see intended results with the cache, I think we need to actually give specific version number in test_requirements.txt even if the packages are mentioned in setup.py. As it is currently set up now, even if new versions are uploaded on pypi (or github for libdmet), cache key would stay same (and thus we wouldn't be able to pull newer version on pypi without changing setup.py). I propose that we fix this in a separate PR.

Similarly, @ShaunWeatherly I think dmrgscf should also reference a specific tag or commit. block2 should also have a version specified (both in the workflow and the key).

[mcocdawc]

May I understand what the point of this caching is?
Getting cache invalidation correct is really hard and the proposed setup will force us to regularly manually update the requirements files.
If performance of the test suite is the only reason I fear it might be a bit unnecessarily complicated.
I get why it makes sense for projects with larger dependencies or where they have to compile dependencies, but our build process from scratch is so fast, that I just would do it from scratch.

[mscho527]

Also, another question for @ShaunWeatherly -- if the test suite on Github Actions isn't invoking block2, do we need to install block2?

[mscho527]

Is this redundant? Line 124 installs from the repo as well.

[ShaunWeatherly]

If there are any changes to quemb itself, this is the line that updates those parts of the code. The previous pip install . above is really only used for installing/caching the dependencies in setup.py, hence why it's hash is used in the key.

[mscho527]

I'm not sure why we would want to install and reinstall if there are any changes. The hash key in the quemb cache workflow above cannot detect changes made in the quemb code. I think we should install quemb from the latest commit once other dependencies are resolved (through cache, pip, or what not)

[mscho527]

Is there a particular reason why the default value of allow_existing has to be False? Does this conflict with mixed-basis code and require modification there as is? @lweisburn

[ShaunWeatherly]

This is simply how Oskar has setup WorkDir, where the scratch directory provided by the user was required to be empty (otherwise it would raise a ValueError). This gives you the option to turn off that requirement by setting allow_existing=True

[mscho527]

@mcocdawc Was there a reason why you required scratch directory to be empty?

[mcocdawc]

Initially I required it to prevent silent overwriting of files, as @ShaunWeatherly pointed out in his note.
But even more dangerous is that the WorkDir is deleted in the end if cleanup_at_end is true which it is by default.
This means that WorkDir("/home/${USER}") would delete the home directory.

On the other hand @lweisburn and you (?) @mscho527 need an already populated WorkDir for mixed basis calculations..

So yes, there should be an allow_existing argument which should be False by default.
It should also be added to the from_environment constructor.
I would then also expand the error message along the line of.

ValueError("scratch_area is not empty and allow_existing is False. "
              "You can set `allow_existing=True` in the constructor, but note that "
               "the working directory is deleted if `cleanup_at_end` is True, which "
               "it is by default."
    )

[mscho527]

Aha @mcocdawc I see! @lweisburn we should assess how to let this work with mixed basis code

[lweisburn]

With the mixed basis code, we will have to use this option discussed above with allow_existing to be True, but it makes sense that for other uses for it to be False by default

[mscho527]

Is there an easy way to detect whether block2 is installed or not? If so, I think replacing GITHUB_ACTIONS check with the check would be nice.

[ShaunWeatherly]

The installation of block2 is not the problem, it's that the test simply cannot be run in github actions. Oskar and I spent a lot of time trying to find a better work around, but to no avail. This seems to be the easiest solution.

[mscho527]

Maybe Oskar and you discussed this already, but what do you mean by 'cannot'? I trust you two, but just curious.

Anyway, if I understood correctly, this test essentially tests i) all the way upto calling block2 and ii) after block2 finishes and quemb should do some post-processing, right? I worry that as is, the tests can make us overconfident. If at any point no one performs BE-DMRG calculations anymore, CI test on itself would no longer automatically test the full pipeline. I think this aspect of DMRG test routine should be noted in the docs.

[mscho527]

Reminder to restore analysis requirement before merge

[mcocdawc]

Even more dangerous is the fact that the directory gets cleaned up, if cleanup_at_end=True.
i.e.

WorkDir("/home/${USER}")

will clean up the home directory of the user in the end.

---

I would use the sphinx note syntax https://sublime-and-sphinx-guide.readthedocs.io/en/latest/notes_warnings.html

... note: Use caution ...

[mcocdawc]

Initially I required it to prevent silent overwriting of files, as @ShaunWeatherly pointed out in his note.
But even more dangerous is that the WorkDir is deleted in the end if cleanup_at_end is true which it is by default.
This means that WorkDir("/home/${USER}") would delete the home directory.

On the other hand @lweisburn and you (?) @mscho527 need an already populated WorkDir for mixed basis calculations..

So yes, there should be an allow_existing argument which should be False by default.
It should also be added to the from_environment constructor.
I would then also expand the error message along the line of.

ValueError("scratch_area is not empty and allow_existing is False. "
              "You can set `allow_existing=True` in the constructor, but note that "
               "the working directory is deleted if `cleanup_at_end` is True, which "
               "it is by default."
    )

[mcocdawc]

good catch

[lweisburn]

Note to @lweisburn and @mscho527 , this PR corrects the definition of BE.ebe_tot to be the total BE energy. Previously, some parts of the code were using it as the correlation energy, including in the misc.be2puffin function. I've updated that function to explicitly use the correlation energy (which I assume was the original intent) and I just wanted to double check and make sure I didn't break anything!

Great, thanks for noting this! We'll make sure that this is consistent with other external (mixed-basis) code too