Closed MichaelWolloch closed 1 year ago
MichaelWolloch
commented
1 year ago Had forgotten to change the BPARAM also in the reference INCAR file for TestScanOptimizeWorkflow. Done now. I am not sure if the other failing tests have something to do with my changes, but doubt it. Unfortunately I have troubles testing this on my machine due to database issues.
MichaelWolloch
commented
1 year ago OK, after downgrading my mongodb installation to 5.x and installing openbabel, I got the tests to run locally. All pass or are skipped, so I am not sure why the testing environment fails.
================================================ test session starts =================================================
platform linux -- Python 3.11.0, pytest-7.2.2, pluggy-1.0.0
rootdir: /fs/home/wolloch/git_test/mwo_atomate/atomate, configfile: setup.cfg
plugins: anyio-3.6.2
collecting 0 items /fs/home/wolloch/git_test/mwo_atomate/atomate/atomate/common/firetasks/tests/test_parse_outputs.py:17: PytestCollectionWarning: cannot collect test class 'TestDrone' because it has a __init__ constructor (from: atomate/common/firetasks/tests/test_parse_outputs.py)
class TestDrone(AbstractDrone):
/fs/home/wolloch/git_test/mwo_atomate/atomate/atomate/utils/tests/test_database.py:22: PytestCollectionWarning: cannot collect test class 'TestToDb' because it has a __init__ constructor (from: atomate/utils/tests/test_database.py)
class TestToDb(CalcDb):
collected 226 items
atomate/common/firetasks/tests/test_glue_tasks.py ....... [ 3%]
atomate/common/firetasks/tests/test_parse_outputs.py . [ 3%]
atomate/common/tests/test_powerups.py ...... [ 6%]
atomate/feff/firetasks/tests/test_tasks.py .. [ 7%]
atomate/feff/fireworks/tests/test_fireworks.py . [ 7%]
atomate/feff/workflows/tests/test_eels_workflows.py ... [ 8%]
atomate/feff/workflows/tests/test_exafs_scattering_paths.py ... [ 10%]
atomate/feff/workflows/tests/test_xas_workflows.py ... [ 11%]
atomate/lammps/tests/test_drone.py s [ 11%]
atomate/lammps/tests/test_workflows.py s [ 12%]
atomate/qchem/firetasks/tests/test_critic2.py s. [ 13%]
atomate/qchem/firetasks/tests/test_fragmenter.py ................ [ 20%]
atomate/qchem/firetasks/tests/test_geo_transformations.py .. [ 21%]
atomate/qchem/firetasks/tests/test_parse_outputs.py .... [ 23%]
atomate/qchem/firetasks/tests/test_run_calc.py .......... [ 27%]
atomate/qchem/firetasks/tests/test_write_inputs.py ......... [ 31%]
atomate/qchem/fireworks/tests/test_core.py ................... [ 39%]
atomate/qchem/tests/test_drones.py ................... [ 48%]
atomate/qchem/tests/test_powerups.py . [ 48%]
atomate/qchem/workflows/tests/test_FF_and_critic.py . [ 49%]
atomate/qchem/workflows/tests/test_double_FF_opt.py . [ 49%]
atomate/qchem/workflows/tests/test_fragmentation.py .. [ 50%]
atomate/qchem/workflows/tests/test_parse_pass_write.py .. [ 51%]
atomate/qchem/workflows/tests/test_reaction_path.py . [ 51%]
atomate/qchem/workflows/tests/test_torsion_potential.py . [ 52%]
atomate/utils/tests/test_database.py .... [ 53%]
atomate/utils/tests/test_loaders.py .. [ 54%]
atomate/utils/tests/test_utils.py ...... [ 57%]
atomate/vasp/firetasks/tests/test_copy.py ...... [ 60%]
atomate/vasp/firetasks/tests/test_exchange.py s [ 60%]
atomate/vasp/firetasks/tests/test_get_interpolated_poscar.py . [ 61%]
atomate/vasp/firetasks/tests/test_lobster_tasks.py ....... [ 64%]
atomate/vasp/firetasks/tests/test_polarization_to_db.py . [ 64%]
atomate/vasp/firetasks/tests/test_write_vasp.py ........ [ 68%]
atomate/vasp/firetasks/tests/test_write_vasp_from_interpolated_poscar.py . [ 68%]
atomate/vasp/fireworks/tests/test_exchange.py . [ 69%]
atomate/vasp/fireworks/tests/test_fireworks_core.py ......... [ 73%]
atomate/vasp/fireworks/tests/test_lobster.py . [ 73%]
atomate/vasp/fireworks/tests/test_nmr.py . [ 73%]
atomate/vasp/tests/test_drones.py ........ [ 77%]
atomate/vasp/tests/test_setup.py . [ 77%]
atomate/vasp/tests/test_vasp_powerups.py .............. [ 84%]
atomate/vasp/workflows/tests/test_adsorbate_workflow.py ..s [ 85%]
atomate/vasp/workflows/tests/test_approx_neb_workflow.py . [ 85%]
atomate/vasp/workflows/tests/test_bulk_modulus_workflow.py . [ 86%]
atomate/vasp/workflows/tests/test_elastic_workflow.py . [ 86%]
atomate/vasp/workflows/tests/test_exchange_workflow.py s [ 87%]
atomate/vasp/workflows/tests/test_ferroelectric_workflow.py . [ 87%]
atomate/vasp/workflows/tests/test_hubbard_hund_linresp.py . [ 88%]
atomate/vasp/workflows/tests/test_insertion_workflow.py . [ 88%]
atomate/vasp/workflows/tests/test_lobster_workflow.py ...... [ 91%]
atomate/vasp/workflows/tests/test_magnetism_workflow.py .s. [ 92%]
atomate/vasp/workflows/tests/test_neb_workflow.py . [ 92%]
atomate/vasp/workflows/tests/test_nmr.py . [ 93%]
atomate/vasp/workflows/tests/test_raman_workflow.py . [ 93%]
atomate/vasp/workflows/tests/test_vasp_workflows.py .............. [100%]
===================================== 219 passed, 7 skipped in 351.20s (0:05:51) =====================================The exact same test also fail for PR #775, so I am really confident now that something is wrong with the CI, rather than my code. I have really no experience with CI, but the versions of python, pytest, and pluggy are significantly older than on my local machine. Maybe one should upgrade those?
janosh
commented
1 year ago Feel free to update the req files. No reason to pin to an old pymatgen.
https://github.com/hackingmaterials/atomate/blob/f4060e55ae3a22289fde9516ff0e8e4ac1d22190/requirements.txt#L1
MichaelWolloch
commented
1 year ago Hi @janosh , and thanks for pointing that out. I did not really notice the requirements file to be honest.
The tests that I claimed passed on my machine were actually using the old pymatgen and custodian versions. Once I updated, I also got the same two failed tests, test_babel_PC_with_RO_depth_0_vs_depth_10 in atomate/qchem/firetasks/tests/test_fragmenter.py and TestNMRWorkflow in atomate/vasp/workflows/tests/test_nmr.py.
I wanted to at least figure out why the vasp related test is failing, and has been failing since may. It turned out that figuring that out was quite a challenge, that's why I am only responding now.
It was pretty clear that the vasprun.xml file could not be loaded into a pymatgen Vasprun object, but I could not make much sense of the traceback and no relevant code in the Vasprun class was changed as far as I could see. Once I figured out that indeed nothing at all changed in the Vasprun class between the last working version (v2023.3.28) and the next release (v2023.5.8) I went through changes in the imports and, by some trial and error (I should really learn how to debug python modules better) figured out that the breaking change was actually something I (and @janosh ) are a bit familiar with: Commit https://github.com/materialsproject/pymatgen/commit/1e9119a07e0282bb89807a874583e66e57ea35f5 !
Apparently the change in the Potcar class did break the reading of vasprun.xml files for chemical shift tensor calculation (LCHIMAG = .TRUE.). I am not sure exactly how, but I spent enough time on this already, so I am content with the knowledge that it did.
I will make a comment in the appropriate issue (https://github.com/materialsproject/pymatgen/issues/3068) and also make a PR for this change I guess. However, for now I will update the requirements to the current versions and let this test fail.
I will also take a look at the other failed test, but since I know nothing about qchem, this will probably not result in much success. However, that test fails pretty straightforwardly with AssertionError: 411 != 509, so maybe I can figure out what happens after all.
janosh
commented
1 year ago @MichaelWolloch Thanks for taking a deep dive on this! It's very helpful to know why the test is failing. I just took a look in pymatgen. All the vasprun.xml files we have set LCHIMAG=False which might explain why we didn't catch this. I'm not at all familiar with chemical shift tensor calculation. It's not clear to me though why a change in POTCAR parsing would affect parsing vasprun.xml. Any ideas @jmmshn? In any case, maybe we can take the vasprun.xml in the failing test and use it for a new test in pymatgen so we don't regress again.
MichaelWolloch
commented
1 year ago I think it somehow has to do with setting charges from POTCAR. At least that what the traceback suggests:
Traceback (most recent call last):
File "/fs/home/wolloch/git_test/mwo_atomate/atomate/atomate/vasp/drones.py", line 270, in generate_doc
d["calcs_reversed"] = [
^
File "/fs/home/wolloch/git_test/mwo_atomate/atomate/atomate/vasp/drones.py", line 271, in <listcomp>
self.process_vasprun(dir_name, taskname, filename)
File "/fs/home/wolloch/git_test/mwo_atomate/atomate/atomate/vasp/drones.py", line 451, in process_vasprun
vrun = Vasprun(vasprun_file, parse_potcar_file=self.parse_potcar_file)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/michael/mambaforge/envs/atomate_test/lib/python3.11/site-packages/pymatgen/io/vasp/outputs.py", line 395, in __init__
self.update_charge_from_potcar(parse_potcar_file)
File "/home/michael/mambaforge/envs/atomate_test/lib/python3.11/site-packages/pymatgen/io/vasp/outputs.py", line 1152, in update_charge_from_potcar
for s in self.structures:
^^^^^^^^^^^^^^^
File "/home/michael/mambaforge/envs/atomate_test/lib/python3.11/site-packages/pymatgen/io/vasp/outputs.py", line 518, in structures
return [step["structure"] for step in self.ionic_steps]
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/michael/mambaforge/envs/atomate_test/lib/python3.11/site-packages/pymatgen/io/vasp/outputs.py", line 518, in <listcomp>
return [step["structure"] for step in self.ionic_steps]
~~~~^^^^^^^^^^^^^
KeyError: 'structure'I agree with making a new test using a LCHIMAG=True vasprun.xml. Actually I also never made a chemical shift tensor calculation before yesterday. I wanted to re-do the calculation with vasp 6.4.1 to see if there was maybe a problem with the old 5.4.4 version used in the atomate test. It was not.
MichaelWolloch
commented
1 year ago Regarding the CI. Apparently the changes to the requirements.txt and requirements-ci.txt made things considerably worse :/ now. The tests cannot even be collected it seems. I think this might be an issue with openbabel, which I needed to install via mamba/conda locally because the pip install fails with a broken wheel. But I think I fixed it by using openbabel-wheel in requirements-ci.txt instead of not specifying openbabel at all.
MichaelWolloch
commented
1 year ago OK, that did not quite do the trick...
@janosh , can you help me with how the CI is set up? I assumed it would install both requirements.txt and requirements-ci.txt. Doing this in that order worked for me in a fresh environment. Or is only pip install -r requirements-ci.txt used in the CI?
janosh
commented
1 year ago Looks like requirements.txt is unused:
So you need to modify setup.py
MichaelWolloch
commented
1 year ago Thanks for the info @janosh, I will try to do update the setup file instead!
I also found out a bit more about the Vaspruns with LCHIMAG. It is even more of an edge case than I thought.
Actually there was already a test for that in pymatgen, and there is a corresponding vasprun.xml file! The file is just called vasprun.xml.chemical_shift.scstep so it did not show up in your search.
Here is the test in pymatgen.io.vasp.tests.test_outputs.py VasprunTest class:
def test_parsing_chemical_shift_calculations(self):
with warnings.catch_warnings():
warnings.simplefilter("ignore")
filepath = self.TEST_FILES_DIR / "nmr" / "cs" / "basic" / "vasprun.xml.chemical_shift.scstep"
vasprun = Vasprun(filepath)
nestep = len(vasprun.ionic_steps[-1]["electronic_steps"])
assert nestep == 10
assert vasprun.convergedAnd this test passes with the current pymatgen release!
However, in that folder, there is no POTCAR file. So when the vasprun.xml file is loaded, the update_charge_from_potcar method of the Vasprun class is not used, and the problematic parsing of the POTCAR does not occur.
So the test correctly parses the vasprun file, but if a POTCAR is in the same folder, as will be the case usually for a real calculation, and as is the case for the atomate workflow test, the parsing fails.
No wonder that this eluded everyone for so long.
jmmshn
commented
1 year ago @MichaelWolloch thanks for catching this. So there was a bug with the POTCAR output from pymatgen while we were fixing a couple of POTCAR issues. Have you confirmed that the POTCARs in those directories were generated using the new pymatgen release as well?
Please confirm and pay special attention to the end of each atomic data block.
MichaelWolloch
commented
1 year ago Ok, I now updated setup.py to use more modern versions.
I decided to include openbabel-wheel into the extra-requires for qchem and complete, because openbabel does not work with pip. In the CI it is instead installed via apt-get, but I think one should be bale to run tests when just installing everything with pip. In fact, I would actually remove requirements.txt and requirements-ci.txt altogether and put everything in setup.py if it were my call. Are there any objections to this?
MichaelWolloch
commented
1 year ago @jmmshn, regarding the POTCARs, this is a very good point. The one I tested was just taken from atomate/vasp/test_files/nmr_wf/cs/outputs. So definitely an old one.
I now did create a new POTCAR using MPNMRSet and the latest pymatgen release. The error stays the same.
However, I now tested a lot more stuff in pymatgen and found that the way it was done in pymatgen version 2023.3.28 was actually wrong as well. The splitting was not done quite right, leading to a problem accessing the headers of the PotcarSingles. (this is of course what @janosh and @jmmshn fixed in their commit https://github.com/materialsproject/pymatgen/commit/1e9119a07e0282bb89807a874583e66e57ea35f5 ! Although they introduced some other issues at that time.) This leads to an error after potcar parsing that prevents the charges from being updated in the structures of the Vasprun. If that error is fixed, the loading of the Vasprun fails again.
The curx of the problem seems to be the fact that even if NSW=0 for a chemical shift calculation, there are more than 1 ionic steps counted by the vasprun parser. Usually each of those steps has a structure, and then the Vasprun.update_charge_from_potcar method tries to update the charges for each of those. For the chemical shift calculations, only the first "ionic step" has actually a structure attached. Thus I did an additional check for a chemical shift calculation.
I think further discussion on that topic should be done on the pymatgen github. Maybe in https://github.com/materialsproject/pymatgen/issues/3068, or in a new PR that I will make there today.
I did add an additional test case for a chemical shift vasprun with a POTCAR in the same folder.
MichaelWolloch
commented
1 year ago All in all, I think this PR is ready.
atomate/.github/workflows/test.yml was updated accordingly.Of course the nmr test will still fail until pymatgen is updated, and there is also the issue of the qchem test, which I am really not qualified to tackle. Maybe @janosh can ping a qchem expert? But this should be a different PR I think.
janosh
commented
1 year ago @MichaelWolloch This is great! Thanks for keeping atomate up to date with pymatgen and custodian.
I see two tests still failing which is the same number as on the main branch and the changes in this PR are definite improvements so I'm in favor of merging as is @Zhuoying.
Maybe @orionarcher can comment or knows who to ask about the failing QChem test?
def test_babel_PC_with_RO_depth_0_vs_depth_10(self):
with patch("atomate.qchem.firetasks.fragmenter.FWAction") as FWAction_patch:
ft = FragmentMolecule(
molecule=self.pc,
depth=0,
open_rings=True,
do_triplets=False,
opt_steps=1000,
)
ft.run_task({})
self.assertEqual(ft.check_db, False)
depth0frags = ft.unique_fragments
> self.assertEqual(len(depth0frags), 509)
E AssertionError: 411 != 509
Andrew-S-Rosen
commented
1 year ago @samblau and @espottesmith are the main points of contact for Q-Chem and may have insight here.
Zhuoying
commented
1 year ago Hi @MichaelWolloch, Thanks for all the efforts/discussions and diving into the bugfix. Really appreciate for catching this bug and hopping between custodian/pymatgen/atomate to fix. I am fine with merging it as it is and leaving Q-chem test-fixing in another PR. From what I see now, your PR with pymatgen to fix the POTCAR for chemical shift calculations is still under review @janosh(https://github.com/materialsproject/pymatgen/pull/3204)? I can put an eye on that as well.
@janosh, I retriggered the test; some neb workflow failed due to path_finder in pymatgen has been moved. Do you have an idea where it has been put now so that I can submit another PR to fix? https://github.com/hackingmaterials/atomate/blob/f4060e55ae3a22289fde9516ff0e8e4ac1d22190/atomate/vasp/firetasks/approx_neb_tasks.py#L7
When the only failed test is on Q-chem test (which I also don't have any experience with), I will ping Q-chem contact to fix them. Thanks all for providing comments/suggestions under this PR.
janosh
commented
1 year ago @Zhuoying The same question came up earlier today from @munrojm. According to @shyuep ChgcarPotential, NEBPathfinder were moved to https://github.com/materialsvirtuallab/pymatgen-analysis-diffusion (although I don't see them there).
MichaelWolloch
commented
1 year ago @Zhuoying The same question came up earlier today from @munrojm. According to @shyuep
ChgcarPotential,NEBPathfinderwere moved to https://github.com/materialsvirtuallab/pymatgen-analysis-diffusion (although I don't see them there).
I think both are in https://github.com/materialsvirtuallab/pymatgen-analysis-diffusion/pymatgen/analysis/diffusion/neb/pathfinder.py.
NEBPathFinder and ChgcarPotential.
This was done just now: https://github.com/materialsvirtuallab/pymatgen-analysis-diffusion/tree/9e8773f9784772489275738e26ae3a00bae7f16b
Zhuoying
commented
1 year ago Hi @MichaelWolloch, thanks for the reminder. I have merged your PR.
Changes on the path to NEBPathfinder and pymatgen-analysis-diffusion version (v2023.8.15) are updated.
https://github.com/hackingmaterials/atomate/blob/52608a1f9d8fc9d5442c3c13698d8adea104f6da/setup.py#L35
Besided qchem and nmr tests, the scan tests are failed as below:
I suppose it is related to https://github.com/materialsproject/pymatgen/pull/3204 and would pass tests after this PR merged? I will keep an eye on that if so. Or please ping me to update pymatgen version in setup.py when this is resolved. @janosh Thanks!
MichaelWolloch
commented
1 year ago Hi @MichaelWolloch, thanks for the reminder. I have merged your PR. Changes on the path to
NEBPathfinderandpymatgen-analysis-diffusionversion (v2023.8.15) are updated.Besided qchem and nmr tests, the scan tests are failed as below:
I suppose it is related to materialsproject/pymatgen#3204 and would pass tests after this PR merged? I will keep an eye on that if so. Or please ping me to update pymatgen version in setup.py when this is resolved. @janosh Thanks!
Hi @Zhuoying , and thanks for merging. The SCAN errors are not related to my PR https://github.com/materialsproject/pymatgen/pull/3204, but to a commit from @janosh last week that sets LELF = False in the MPSCANRelaxSet: https://github.com/materialsproject/pymatgen/commit/1fa96f8535f86d2ee609c906d333f417c8a260e0
So the reference INCARs for SCAN calculations all need to be updated to have LELF = False. I will do another PR quickly to fix this.
It might be worth considering to change the dependencies actually and fix the current version of at least pymatgen and not use >=?
I am not informed really, but since atomate2 exists, maybe development of atomate is not as active any more and one should "freeze" it a bit?
Summary
This fixes 2 bugs that were recently mentioned in issues #776 and #774
MaxForceErrorHandlerfor VASP. Also removed associatedmax_force_threshold(which was used as an input toMaxForceErrorHandlerfromOptimizeFWandRunVaspCustodian. Similarly,RELAX_MAX_FORCE, which was the default formax_force_thresholdwas removed fromatomate.vasp.config.pyandatomate.vasp.vasp_config.pyBPARAMinScanOptimizeFWfrom 15.7 to 11.95. This is the right parameter to selectR2SCAN+rVV10, while the old one was optimized forSCAN+rVV10, which is no longer used inScanOptimizeFW.Since the first bug does break VASP functionality, at least if
RunVaspCustodianis used, and some of the latest custodian releases are used, which does happen for a fresh install since no upper version limit is set, I think this is a quite important fix. Probably a new release should be made as well.