Include detailed tests for forcefield metadata

[justinGilmer]

PR Summary:

Previously, the tests in test_forcefield.py were assumed to test that
forcefield metadata like version, name, etc. were properly loaded
into the Forcefield object.

This is only partially true, as there are cases where the files either
go out of scope before that information is gathered, etc. This PR
includes additional tests to ensure that this information is not being
lost prematurely. Big thanks to: @mattwthompson, @ahy3nz for discovering
this issue, creating MWE's for some of these cases, and taking my
investigation further and most likely pinpointing the issue.

PR Checklist

---

• Includes appropriate unit test(s)
• Appropriate docstring(s) are added/updated
• Code is (approximately) PEP8 compliant
• Issue(s) raised/addressed?

Refer to issues/PRs: #433 #434

Note: This does not have @ahy3nz 's fix incorporated yet, just the test cases that fail and currently work.

[codecov]

Codecov Report

Merging #435 (a283ab6) into master (3b91082) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #435      +/-   ##
==========================================
- Coverage   73.59%   73.58%   -0.02%     
==========================================
  Files          17       17              
  Lines        1803     1802       -1     
==========================================
- Hits         1327     1326       -1     
  Misses        476      476              

[mattwthompson]

Looks like this reproduces the issue (test_load_metadata_from_internal_name, the one using Forcefield(name="") is failing).

I took Alex's suggested patch (#434 (comment)) and made a couple fixes so that tests would pass. I haven't thought much about unintended side effects yet.

Here is the commit; feel free to move it around, copy, etc. : 22855d4

[justinGilmer]

Yeah we essentially had the same design process and handling of the strings. Once this passes and we have the approvals, we can merge. Thanks @mattwthompson and @ahy3nz for both of your contributions!

[mattwthompson]

Does it matter that you're using preprocessed_files and I'm using all_files_to_load? Both seem to pass tests ... it's probably better to use the pre-processed ones in principle?

[mattwthompson]

(Sorry for the misclick 😬)

[justinGilmer]

Does it matter that you're using preprocessed_files and I'm using all_files_to_load? Both seem to pass tests ... it's probably better to use the pre-processed ones in principle?

That was my thought, we already strip out/handle things that might not behave the way we expect once we have pre-processed.

[justinGilmer]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mattwthompson]

LGTM iff tests pass.

There should be some added confidence if another piece of metadata can be added (#433) without re-engineering this machinery.

[ahy3nz]

Does it matter that you're using preprocessed_files and I'm using all_files_to_load? Both seem to pass tests ... it's probably better to use the pre-processed ones in principle?

That was my thought, we already strip out/handle things that might not behave the way we expect once we have pre-processed.

While I agree the pre-processed XML files are safer to try reading, we might have to consider this block of code that is being deleted in this PR. Originally, IIRC @justinGilmer ran into some problems with /tmp blowing up when running a lot of atomtyping and creating a lot of preprocessed XML files in /tmp, so we needed to add this code chunk to remove old XML files. If this chunk of code is removed, this problem may come up again.

748bdd8

        try:
            super(Forcefield, self).__init__(*preprocessed_files)
        finally:
            for ff_file_name in preprocessed_files:
                os.remove(ff_file_name)

[justinGilmer]

I did add a small section at the end of this init method to iterate through the open files and os.remove them. It runs into the problem where I cant really put a try catch finally around the entire part of that code to always ensure that the files are deleted, but it should handle most cases.

https://github.com/mosdef-hub/foyer/pull/435/files#diff-2e32e78335ccab485367397b205f395d5793e27101a8e63226d79081eb4f2356R536-R537

[ahy3nz]

My mistake! Good catch

[daico007]

LGTM!