Addendum to APE 2 - #20

IMO if a deprecation was overseen in n.1.0, allowing it to be removed in n.2.0 should be fine rather than wait for n+1.0.0

@bsipocz [I answer in the main thread since otherwise your comment will get lost the moment I update the PR]: But we should be aware that this forces packages that want to support both n.0.x and n.>=2 to have astropy version checks in their code (see the example in the previous paragraph). I don't think this is ideal.

Sure, and I didn't mean to allow new deprecations, but to remove already deprecated code that for some reason was missed out in n.1.0 (but that are already deprecated as of n.0.0)

OK, yes, if we miss things in n.1.0, I agree they can still be removed in n.2.0.

This means that before a n.0.0 release someone has to check which warnings should be added or turned on.
There is the possibility with the @deprecated decorator to mark deprecated functions as "pending". This could be used to add the @deprecated decorator during the development, but show the warning only for the n.0.0 release, however the status of which warning is shown by default is not clear: in IPython DeprecationWarning are shown and PendingDeprecationWarning are hidden, whereas in the Python cli both are hidden by default.

Would a PendingDeprecationWarning be raised as exception for our enable_deprecations_as_exceptions?

@bsipocz : it seems so https://github.com/astropy/astropy/blob/151877b02b93dc518937e233430f5a475a56fd22/astropy/tests/helper.py#L254

Then the proposed approach won't solve the issue for the affiliates to support both LTS and latest.

@saimn - good point. I think that to ensure that for a LTS the deprecations do in fact get added, the change log should have a section for "pending deprecations", which gets carried forward to new feature releases, until they are implemented.

Alternatively (or at the same time), perhaps pending deprecations can include a version in which the deprecation will actually happen (and we change our filtering such that these do not raise errors by default in tests.

I have mixed feelings about this PR. There are cases where removing things from one release to the next can be useful, for instance if we add a feature in 3.0 that we decide was a mistake, we need to remove it ASAP not wait for 4 releases. The longer something is in a release the more people will rely on it.

To be honest, our current method has never really been a problem before - there is just one instance of one user having a problem recently, and the complaint was that the removal was not properly described in the what's new. So part of me thinks that the only change we need to make for future is that all non-trivial/non-obscure API changes should be mentioned in the what's new.

I do agree that if all else is equal, and there is no rush, using LTS releases to remove things is fine, but I don't know if it should be a firm policy.

I'm also -1 on using pending deprecation warnings. I mean at the end of the day why not just call it a deprecation warning - it's going to be a warning for the user anyway, so not sure there is much of a difference.

Finally (ok, I'm not usually this negative!) I think that APEs aren't meant to be living/updatable documents. I think this would warrant a new APE, but maybe the other coordinators (@eteq, @taldcroft , @kelle) can pitch in.

@astrofrog - I'm happy to write this more as "ideally should" rather than "must", but I think the trouble in astropy/astropy#5644 (clobber -> overwrite) was real and unnecessary.

@larrybradley - I think we can remove in n.2 or n.3 -- but if we deprecate in n.0, then really there is no reason not to remove in n.1 already -- later just means we have not done due dilligence.

p.s. Just to be absolutely sure, this PR is not meant to address recurrence of the problems with the removal of new_table()

Finally (ok, I'm not usually this negative!) I think that APEs aren't meant to be living/updatable documents. I think this would warrant a new APE, but maybe the other coordinators (@eteq, @taldcroft , @kelle) can pitch in.

The text on deprecations in this APE is more than a little vague. I think we should allow clarifications of actual policy (it certainly makes it more likely anything will happen -- I would not have done anything if I knew it had be a full APE....). As you'll have seen, I did try to ensure that the main text isn't really changed.

But maybe the real problem is the whole APE system. Better might be that any accepted APE leads to a section in the (developer) documentation. This can initially be a copy of the APE, but can (and should) be changed as time progresses. This way, the APE is just a formal way to set/change habits, and gives a history of the rationale, but the actual practical knowledge is in one place.

@astrofrog : I agree the APE should not be too strict and leave some flexibility. In the case addressed here (the clobber -> overwrite change), the APE will not solve the issue of how to support both LTS and latest (if the deprecated code is removed in n.1.0, then some compatibility code is needed to support both LTS and latest). So the main benefit I see is that users will have a LTS release so more time to use the deprecated features and update to the new API.
Re: pending deprecation warnings: it is interesting if these warnings are hidden by default, which in the case in IPython (and in Python both are hidden, but this needs to be checked).

Thinking about keyword deprecation changes in particular, one bulletproof solution could be:

• Allow backward-compatible keyword changes in any feature release (meaning the old keywords still work). Do not issue any warning, just update the docs.
• At the 2nd LTS after the release, introduce a deprecation warning. So if the keyword change is introduced in 1.3.0, then start deprecating not at 2.0 but at 3.0 (never mind that 3.0 is weird this time around, pretend that it is the subsequent LTS after 2.0).
• Then remove the old keywords in 4.0.

The point here is that waiting until the next LTS to start deprecating is not enough, because the next LTS might be the next release. Obviously packages want to support 1.3 when 2.0 comes out. But it is reasonable that affiliated packages could drop support for 1.3 when 3.0 comes out. (Again, I don't mean the real astropy 3.0).

So this is a very slow cycle and in practice it just means leaving some cruft in the code for 4-6 years. But it will prevent the overwrite/clobber problem we saw.

@saimn - , the APE will not solve the issue of how to support both LTS and latest (if the deprecated code is removed in n.1.0, then some compatibility code is needed to support both LTS and latest).

I don't see a problem here for affiliates. The deprecation is introduced in LTS meaning that the new usage/replacement functionality is already available. Removing the old deprecated code practically means that the affiliates have about half a year to adapt the changes or state that they not yet support the latest astropy release, which IMO seems fine.

So just to be clear, in my example:

• When the keyword change happens in the feature release (e.g. 1.3.0), packages that want to support multiple versions do nothing.
• When version 2.0 is released, affiliated packages do nothing.
• When version 3.0 is released, affiliated packages notice deprecation warnings in their tests, and change every instance of clobber to overwrite. This works for astropy versions all the way back to 1.3.

@astrofrog - on whether APEs are living documents, I see your point but at the same time it could certainly happen that we do want to change policy or interface that is in an APE. Providing clarification of deprecation policy seems like something appropriate for a high-level and visible document like APE-2.

Also, I was considering updating APE-6 (ECSV format) soon to provide the capability to fully serialize astropy objects. In the case of APE-6, this document is not just an enhancement but a specification of a standard. Likewise I wonder if the NDData APE will be updated at some point?

One more point is that I think the process for a particular deprecation should depend on the expected impact. In the case of changing the way FITS files are written, the impact is very wide and so a conservative process would make sense. But for other more obscure parameters it might be OK to just deprecate right away.

Discussion in astropy/astropy#6247 has made me think again about this APE update and I still don't understand how this particular change (deprecating only in LTS releases) helps anything from the perspective of affiliated packages.

Maybe I'm being dense, but as far as SunPy is concerned it makes no difference if an API change / deprecation goes into 1.2 or 1.3 or 2.0, they still have to put in version-specific handling in the code to prevent spamming their users with warnings. It depends on policy, but I imagine something like "SunPy supports the astropy LTS release along with the most recent two releases".

@Cadair ?

So, once you've opened the box of version-specific code then how does requiring deprecation in an LTS actually help anything? (We can defer this discussion to after the 2.0 release, but I just wanted to type this while it is fresh in my mind).

Well, it may not work for Sunpy, but for photutils the plan for the upcoming 0.4 is to drop support for anything below 2.0, but then stick with it as an LTS (we did the same previously, supported 1.0.x and latest stable, but haven't tested for anything in between the two).

So LTS releases mean that anything except the new LTS + later minor versions is officially unsupported (immediatly)?

That's what we will do, obviously other packages can go a different route. astropy/photutils#552

(Needless to say with such tightly related packages, anyone cutting edge enough to use the latest greatest photutils should really use the latest gratest astropy. This may not be true for packages that only slightly rely on astropy).

SunPy generally takes a pretty harsh line on Astropy releases, if there is something we need in an astropy version we currently don't have any issue with just enforcing that (our next release will be >=1.3).

I don't think this proposal makes a huge amount of difference to SunPy at the moment.

We discussed this a bit at the CC meeting. I will post our thoughts on this soon.

Based on some discussion at the coordination meeting, I would like to close this for several reasons. First, the APE process isn't really meant to include significant new parts like here. I think the topic of how to handle deprecations is worthy of its own APE.

More specifically related to the changes here, I think we are sometimes removing deprecated functionality too easily. We've had a number of complaints from affiliated package maintainers, e.g. during the removal of clobber. Deprecating functionality is fine, but I don't agree with having rules about removing too fast. If maintaining deprecated arguments or functions is not a burden to current development, then there is no harm in leaving deprecated functionality to allow users more time to update. In fact, I have no problem with deprecated functionality staying in the core package for several years if it was previously widely used (essentially, how soon we remove functionality should depend on usage - and in the case of clobber we did it way too fast).

I would propose creating a new APE specifically about deprecating and removal of deprecation code. I'm fine with having a minimum time until removal enshrined in such an APE, but we should make it clear that above all we support stability and don't remove things unless we need to.

@astrofrog - I think the intent of my addendum was exactly to ensure we wouldn't remove too quickly; I still think overall it is a sensible way, but am of course happy if a new APE gets written for this purpose -- just be sure this actually gets done, as the present APE has been used to argue that we're deprecating just right and one should not presume that people will remember this discussion. In that respect, it seems actually important to clarify here the policy as well, at least pending the new APE. Indeed, I'd go as far as to suggest one should just merge this as a placeholder until the new APE appears, but then I know what time I have to help with a new one...

Anyway, feel free to close if you think that is better.

@astrofrog - OK, comment addressed, to make sure we do not set a default deprecation time period, but rather a minimum one.

Sometimes I wish there is something concrete about deprecation that I can refer to. This has been open for over 2 years. Closure would be nice.

I am just gonna xref astropy/astropy#6000 (comment) here so I can find it in the future. Thanks to @bsipocz for pointing it out.

Well, obviously I agree this should just be merged! But then, I thought we should 1.5 years ago... Let's not let the perfect prevent the "good enough".

The question of when to deprecate and when to remove came up again. @mhvk - do you have time to wrap this up or would be interested in working on it together (the addendum formats now formalized, also I suppose we should run this through astropy-dev, and I would like some rewording about the removal part)? Having something to point to that is not an open PR would be nice.

@bsipocz - yes, it would be good to get this in! I'm all in favour of whatever is minimal combined work. Two approaches would be for you to either suggest text inline or make a PR to this PR with your suggested text changes, after which I could update to get to the new style for changes, or you simply taking this as a basis and making a new PR.

Thanks @mhvk for started the ball rolling even though this particular PR didn't get merged! 🙇‍♀️