Ignore pytest deprecation warning for monty dependency of pymatgen

[unkcpz]

It is just annoy to have pymatgen as the dependency we REALLLY need to move it out.

[unkcpz]

This is how they raise deprecation warnings to fail tests run in downstream libraries' CI, it is ridiculous! Took me quite long to figure out why the failed "warning" was only happened on CI but not locally. They literally RAISE the warning, very confuse.

        if (
            _deadline is not None
            and os.getenv("CI") is not None
            and datetime.now() > _deadline
            and _is_in_owner_repo()
        ):
            raise DeprecationWarning(
                f"This function should have been removed on {_deadline:%Y-%m-%d}."
            )

[unkcpz]

@mikibonacci do you think we can move it to aiida-atomistic?

[mikibonacci]

Hi @unkcpz, are you saying we should move all the pymatgen-related tests (and pymatgne dependency) in aiida-atomistic? For me should be fine, however we still have the LegacyStructureData in aiida-core, so I am not sure it is the best way - at least for now.
Additionally, would the tests fail for aiida-atomistic then? I am not sure I got 100% of the issue here.

Thanks a lot for catching this, BTW!

[mikibonacci]

Also: I was fixing the aiida-atomistic PR, but now I see that indeed the tests are failing due to this issue 😞

[bastonero]

Just passing since i saw tests failing on aiida-quantumespresso. Please, do not remove the core StructureData as I have put tons of work on HubbardStructureData and associated plugin (also, now people started using it for XPS, XAS), and we use a lot pymatgen since they implement very useful routines, which we would otherwise need to reimplement. I also don't know how much it will take to transition to the newer StructureData of aiida-atomistic, since i won't have time and i don't know who will have it

[bastonero]

PS: since this is blocking any aiida related plugin, do you think we can quickly fix and merge this PR?

[unkcpz]

I also don't know how much it will take to transition to the newer StructureData of aiida-atomistic, since i won't have time and i don't know who will have it

Make sense. But we have to struggle when pymatgen did bad things fail downstream libs like this one. I was thinking to move all pymatgen related tests to run in a separate CI and make it the test not mandatory to pass if it is the problem from upstream pymatgen. What do you think?? @bastonero

PS: since this is blocking any aiida related plugin, do you think we can quickly fix and merge this PR?

No quick fix from our side, because pymatgen raise the warning where it should be just using warning.warn to print to the pytest summary. Either we wait them to fix or we as I mentioned above we move it to separate CI test bundle. Not sure if they fix the issue, would you mind to open an issue there (the source of the issue is from monty library)?

[bastonero]

Now that I am thinking better, I guess this will affect as well aiida-quantumespresso, since we import pymatgen functions, AFAIR. I honestely still didn't understand well what is the problem, but the fact they raise the warning instead of using the right function. Maybe it's better if you give it go, so you know better how to state the problem?

[unkcpz]

Sure, I'll open the issue. Here it is materialsvirtuallab/monty#738

[agoscinski]

There is a newer version of monty released this year that has changed the dev.py. Maybe we can try to use this one. But in any case I am fine to exclude these tests temporary. Would just be more specific in the skip function (see my comment)

[agoscinski]

Since this a temporary fix that should be removed after it has been fixed upstream. Could we use not has_pymatgen but a custom function

def is_pymatgen_upstream_issue738() -> bool:
    """Checks if we encounter the upstream bug in pymatgen referenced in materialsvirtuallab/monty issue #738.

    We cannot fix the bug for now and have to skip the tests that encounter this bug.
    """
    try:
        from pymatgen.core.structure import Molecule
    except DeprecationWarning as exc:
        exc.args[0] == "This function should have been removed on 2025-01-01."
        return True

    return False

[agoscinski]

and then maybe

Suggested change

	skip_pymatgen_anyhow = pytest.mark.skipif(not has_pymatgen(), reason='Unable to import pymatgen')
	skip_pymatgen_issue738 = pytest.mark.skipif(is_pymatgen_upstream_issue738(), reason='upstream issue
	#738 in materialsvirtuallab/monty ')

[danielhollas]

Incidentally, I am updating the uv lockfile in #6676. In that PR, only Python 3.9 tests are now failing.
I like the suggestion from the monty issue tracker of simply unsetting the CI environment variable. That way we could avoid any changes to the test suite. I'll give it a try.

[danielhollas]

NOTE: A good discussion about the issue is happening in the pymatgen repo here:
materialsproject/pymatgen#4243
I'll make a comment there and try to convince them to make a release for Python 3.9.

[unkcpz]

I like the suggestion from the monty issue tracker of simply unsetting the CI environment variable.

I'd rather not change this even though it seems safe for the moment. The env variable is global from github and we don't know if github will use this for something else of not.

[danielhollas]

@unkcpz I agree it's not an ideal solution. But this workaround is quite targeted, only in two specific job steps, and only for Python 3.9. Since will be dropping Python 3.9 support sometimes this year anyway I believe this is acceptable, and is much simpler then mucking around with the test suite. Please see #6676

[unkcpz]

superseded by #6676