Deprecate ERISieve

[davpoolechem]

Description

This PR is a companion to #2933. The goal of #2933 is to completely eliminate the ERISieve class, for reasons explained there. However, it turns out that eliminating ERISieve requires fiddling with the v2rdm_casscf plugin, as well, since it also uses ERISieve.

This PR is meant to be a deprecation of the ERISieve class that will show up in v1.8 in the meantime, in case ERISieve isn't fully removed by then.

User API & Changelog headlines

• Deprecates the ERISieve class.

Dev notes & details

• Fully deprecates the ERISieve class C++-side by tagging it with the deprecated attribute.
• Deprecates the ERISieve class Python-side by creating helper functions for the Python-facing ERISieve functions that warn about deprecation.

Questions

• Is PSI_DEPRECATED the preferred mechanism by which to perform this deprecation?
• Assuming the answer to the first question is "yes", are there any other functions in ERISieve that should be tagged? I chose specifically the user-facing functions.
• I loathe having to use a global variable for the Python-side helper functions. But, unbelievably, it is what I considered the best one I could think of. I would be very happy to hear out alternative suggestions.
• Using the PSI_API and PSI_DEPRECATED macros together to tag the ERISieve class cause the compiler to complain. On further exploration, replacing PSI_DEPRECATED with its textual definition seems to work fine. Is this an acceptable solution?

Checklist

• Tests added for any new features
• All or relevant fraction of full tests run

Status

• Ready for review
• Ready for merge

[TiborGY]

It looks like the entire ERISieve class is marked as PSI_API, so AFAIK that is giving downstream the permission to rely on both the object and all of its public members being a stable API, not just a few functions.

[davpoolechem]

It looks like the entire ERISieve class is marked as PSI_API, so AFAIK that is giving
downstream the permission to rely on both the object and all of its public members
being a stable API, not just a few functions.

I think that is right, as well. When I said "user-facing", I meant the ERISieve functions that are explicitly enabled Python-side via PyBind, as defined through the export_mints.cc file.

Deprecation as a whole is less my realm of expertise, however, so I'm not 100% if what I'm doing is even the ideal route regardless.

[loriab]

Yes, it would have to be PSI_API for v2rdm to have used it at all. Those are the only fns to escape pb11's settings to hide all symbols. I think it's still ok to replace medium term, but I agree with @TiborGY that should blanket deprecate. And yes, PSI_DEPRECATED is the way to go. It complains at build-time.

[davpoolechem]

All right, blanket deprecated ERISieve!

... but there's a catch. It seems the compiler complains when trying to use PSI_API and PSI_DEPRECATE("message") together on the same class. I essentially had to hand-insert the associated macros to get the code to compile.

[loriab]

Ok, two different levels here:

• v2rdm (and potentially other plugins or codes that link to psi4 (not that I know any in the latter case)) are using ERISeive c-side. They can do so b/c it's PSI_APId so the symbols are exposed in psi4/core.*so. These should be deprecated by PSI_DEPRECATED. If the plugin owner is compiling his plugin but never psi4 itself, he might still miss the notice, but at least we're giving such a fair chance.
• anyone could be using ERISeive py-side if it's exported by pb11. (And this has nothing to do with whether it's PSI_APId.) I think the way to deprecate those is to wrap the fns like https://github.com/psi4/psi4/blob/master/psi4/driver/p4util/python_helpers.py#L1480-L1495

Whenever dealing with deprecations, I think it's a good idea to use the "as soon as v1.x" phrasing. We want to get the urgency across, but we almost never clear things out ASAP so that phrasing keeps the message accurate.

For semi-completeness, I bring up a couple other deprecation/removal items:

• When you've got to make a breaking change py-side or you want to provide some guidance even after removal, there's UpgradeHelpers https://github.com/psi4/psi4/blob/master/psi4/driver/driver_cbs.py#L1062 that stop the calc but provide advice for fixing the input.
• When you want to remove an option, there's a pattern in core.cc
• When you want to remove or change the meaning of a QCVariable, there's a list in python_helpers.py

[davpoolechem]

Ok, two different levels here:

First off, thank you very much for sharing some details on all of this!

• v2rdm (and potentially other plugins or codes that link to psi4 (not that I know any in the latter case)) are using ERISeive c-side. They can do so b/c it's PSI_APId so the symbols are exposed in psi4/core.*so. These should be deprecated by PSI_DEPRECATED. If the plugin owner is compiling his plugin but never psi4 itself, he might still miss the notice, but at least we're giving such a fair chance.

That make sense. I recently blanket-deprecated the ERISieve class with (the equivalent of) PSI_DEPRECATED. The issue I saw, was that using both PSI_API and PSI_DEPRECATED for the same class caused the compiler to complain, although using their textual equivalents seem to work fine. Hopefully, that settles the deprecation C-side.

• anyone could be using ERISeive py-side if it's exported by pb11. (And this has nothing to do with whether it's PSI_APId.) I think the way to deprecate those is to wrap the fns like https://github.com/psi4/psi4/blob/master/psi4/driver/p4util/python_helpers.py#L1480-L1495

Also very good to know! This will be my next focus, then, since I don't have any wrapper functions for the ERISieve Python-side functions right now.

I think I will switch this to a draft PR in the meantime, since it seems some more needs to be done on my end.

[TiborGY]

The issue I saw, was that using both PSI_API and PSI_DEPRECATED for the same class caused the compiler to complain, although using their textual equivalents seem to work fine. Hopefully, that settles the deprecation C-side.

It is strange that this is happening to you, I had no such trouble when deprecating PSI_API functions, see e11f826

[davpoolechem]

The issue I saw, was that using both PSI_API and PSI_DEPRECATED for the same class caused the compiler to complain, although using their textual equivalents seem to work fine. Hopefully, that settles the deprecation C-side.

It is strange that this is happening to you, I had no such trouble when deprecating PSI_API functions, see e11f826

Deprecating a PSI_API function works fine, which I verified myself. Deprecating a PSI_API class is where I find issues.

Something like:

class PSI_API PSI_DEPRECATED("msg") ERISieve {

causes the compiler to complain. That said, replacing the macros with their associated definitions, something like

class __attribute__((visibility("default"))) __attribute__((deprecated("msg"))) ERISieve {

works just fine.

[davpoolechem]

Okay, I opened this up for official review again, since I think I covered all of the bases mentioned by @loriab. I am not confident in every design decision here, so don't be afraid to suggest alternatives if you have them.

[JonathonMisiewicz]

I'm not confident in all the design decisions either, but this is good enough, and I have nothing better to propose. Good riddance.