Remove deprecated (to|from|as|get)_string methods

[janosh]

Deprecation period started with #3158 on Jul 15 2023 (6+ months ago).

Migration to the new API in all cases is to replace (to|from|as|get)_string with (to|from|as|get)_str.

[mkhorton]

Hi @janosh, thanks for keeping on top of deprecation notices. I think we may have been a bit premature here:

• There was no notice on the compatibility page.
• An updated atomate2 has not been released yet. I see your PR Prep for next pymatgen release atomate2#690 got merged but since there was no subsequent release, the latest atomate2 is not compatible with latest pymatgen.

I realize from a pymatgen maintainer point of view that it's not our responsibility to keep downstream codes working, but I do want to make sure we're mindful of community usage, especially for breaking changes which are not critical.

[janosh]

apologies, i had forgotten about the existence of the compatibility page. both the initial deprecation and the recent removal were in the release notes of course but perhaps a bit buried.

the deprecation period of 6 months was agreed upon with @shyuep who also green-lighted making the to-string API consistent (i.e. replace mixture of get_str and get_string with get_str everywhere).
i suspect in atomate2, the deprecation warnings did not prompt any code updates until it actually broke because the warnings only show up in the logs when executing a workflow. the logs are quite verbose and so usually only inspected when a workflow fails. other community codes may have transitioned earlier and faster on account of the warnings being less buried. i suspect waiting longer would not have made much difference ito community transition (though I could be wrong)

i'll add an entry to the compatibility page right now

[mkhorton]

Thanks @janosh! No worries, these things happen. But we should be careful, any breaking change has the potential to create a lot of work for a lot of people downstream, so we need to make sure such changes are strongly motivated (in this example, removing the deprecated functions does improve code cleanliness but given that the functions were already marked as deprecated, and their inclusion does not cause any performance issue, they could have been left around longer).

The point on logs is well taken. Ultimately, the lack of full, real integration tests has always been an atomate issue, but one that is difficult to address!

[janosh]

The point on logs is well taken. Ultimately, the lack of full, real integration tests has always been an atomate issue, but one that is difficult to address!

i was chatting with @Andrew-S-Rosen about that at some point who managed to setup very sophisticated CI which we tested in Quantum-Accelerators/quacc#1285. it triggers integration tests on Princeton HPC that actually run VASP (even on 3rd party PRs). IIRC he suggested to run a small subset of atomate2 tests as well just to keep a tap on it. but long-term would be better to setup sth similar in atomate2 itself

[Andrew-S-Rosen]

Yes, I do have a variety of integration tests going, which is how I caught #3586 so quickly (actually, many of my bug reports lately come from this since I am running less calculations myself these days...). I'm now taking advantage of on-prem HPC tests to such a degree that I should probably avoid adding on any other compute for things I don't actively use (i.e. atomate2). But indeed, having some sort of regular integration tests set up and synced to GitHub has been extremely valuable. Always happy to brainstorm with MP team about possible avenues.