Fix failing tests from Pydantic v2 migration

[hrushikesh-s]

Summary

• WIP PR to close Update to Pydantic v2 #548

Checklist

Work-in-progress pull requests are encouraged, but please put [WIP] in the pull request
title.

Before a pull request can be merged, the following items must be checked:

• Code is in the standard Python style.
  The easiest way to handle this is to run the following in the correct sequence on
  your local machine. Start with running black on your new code. This will
  automatically reformat your code to PEP8 conventions and removes most issues. Then run
  ruff.
• Docstrings have been added in the Numpy docstring format.
  Run ruff on your code.
• Type annotations are highly encouraged. Run mypy to
  type check your code.
• Tests have been added for any new functionality or bug fixes.
• All linting and tests pass.

Note that the CI system will run all the above checks. But it will be much more
efficient if you already fix most errors prior to submitting the PR. It is highly
recommended that you use the pre-commit hook provided in the repository. Simply run
pre-commit install and a check will be run prior to allowing commits.

[hrushikesh-s]

Hi @janosh ,
Since this PR is to pydantic branch of atomate2, the tests are not being run by github upon making the push action. Any idea of how to make github run the tests without explicitly changing the testing.yaml file ?

[janosh]

@hrushikesh-s Ah, that's a bit annoying. We should add workflow_dispatch as a trigger for manually running tests on select branches. But even that isn't ideal, so maybe we should get rid of the branches: [main] filter for PRs.

[janosh]

Try pulling 5fc8ad7 into your branch.

[janosh]

self should stay as is.

[hrushikesh-s]

@janosh, I'm getting the following assertion error for test_m3gnet_static_maker & test_m3gnet_relax_maker

FAILED tests/forcefields/test_jobs.py::test_m3gnet_static_maker - assert -10.711267471313477 == -10.8454 ± 1.1e-03
FAILED tests/forcefields/test_jobs.py::test_m3gnet_relax_maker - assert -10.710837364196777 == -10.8441 ± 1.1e-03

[janosh]

Weird that the values are not consistent. Feel free to loosen for now:

assert output1.output.energy == approx(-10.8, abs=0.2)

[hrushikesh-s]

@janosh , I'm getting the following errors while running test_nonrad_maker & test_formation_energy_maker function from tests/vasp/flows/test_defect.py

File "/.../atomate2/src/atomate2/common/jobs/defect.py", line 401, in check_charge_state
    raise ValueError(
ValueError: The charge of the output structure is 0, but expected charge state from the Defect object is -2.

File "/.../atomate2/src/atomate2/common/jobs/defect.py", line 401, in check_charge_state
    raise ValueError(
ValueError: The charge of the output structure is 0, but expected charge state from the Defect object is -1.

[janosh]

@hrushikesh-s Not sure what changed. Maybe @jmmshn has troubleshooting suggestions?

[hrushikesh-s]

@janosh, there's one last error that's coming up in test_formation_energy_maker function in test_defect.py:

File "/.../atomate2/src/atomate2/common/jobs/defect.py", line 416, in get_defect_entry
    energy=bulk_c_entry.energy,
AttributeError: 'NoneType' object has no attribute 'energy'

[jmmshn]

Hi @janosh, It looks like there might have been some upstream changes with parsing the net charge of structures from a VASP directory. The code here was just to do a sanity check of the structure charge before things enter the DB.
I'll take a look at this by Wednesday.

[jmmshn]

Actually, I just remembered. I think the tests still pass with this error message.
The problem is that we cannot actually put the POTCARs in the directories due to legal reasons.
So the parsed charges are not going to be correct, so in a sense, this is the expected behavior.
However, I recall the workflow is designed in a way that it still passes CI even if you see those errors.

[hrushikesh-s]

So, do we need to make any changes to deploy.yaml?

[janosh]

@jmmshn Thanks for digging into this so quickly! Re unusable POTCARs in CI, maybe something to simulate with from unittest.mock import patch? Either way, that's for a future PR.

@hrushikesh-s Thanks a lot for your help here! 👍 I'll merge this PR and take a look at the last error over in #548.

[jmmshn]

@hrushikesh-s I don't think so. If CI is actually breaking on this then we will just have to fix the code. I'll mark some time on Wednesday to make sure this all works.

[jmmshn]

@janosh, yeah I think I just ignored it because this failed job was passing CI and it's kind of nice to see the fact that the check is working. Either way I should probably:

• make one of the defect charges parse properly by mocking
• make the other one fail but process that exception better in the testing suite so it doesn't cause this exact confusion.

[janosh]

• make one of the defect charges parse properly by mocking
• make the other one fail but process that exception better in the testing suite so it doesn't cause this exact confusion.

That would be perfect!