APE 21: APE on ending long term support releases + Updates to APE 2 to reflect new release cycle without LTS #82
Conversation
There was a problem hiding this comment.
I think we have to rename this file to APE_21.rst. Otherwise, the file listing will hurt my head going forward.
I think this is going to supersede #20 , so the proposed addenda there by @mhvk needs to be addressed here as well, if not already.
Why the preference on single-quote over double-quote in the text?
APE_LTS.rst
Outdated
| Alternatives | ||
| ------------ | ||
|
|
||
| If there were any alternative solutions to solving the same problem, they should |
APE_LTS.rst
Outdated
| Decision rationale | ||
| ------------------ | ||
|
|
||
| <To be filled in by the coordinating committee when the APE is accepted or rejected> |
There was a problem hiding this comment.
Being the coauthor and current CoCo member, I would recuse myself from CoCo discussions regarding this APE.
pllim
changed the title
|
Had a quick look and liked this! Agreed that it will be important to be very clear about minor and major. After reading this, and my original proposed addendum to #20, I think the proposal is:
Is this correct? Perhaps good to give examples. E.g., "existing code should continue to work" probably should not include that every |
|
@mhvk - I agree with your assessment, I think we basically have to decide on guidelines for those exceptions of things that can change in a minor release that might be considered as 'breaking'. Things that are borderline would include:
Anything else? I think something related to this APE that we don't have to necessarily figure out here but that we could mention briefly is that if we are going to have rules like this we should also be clearer on what is public API - anything tab-discoverable? Anything in the API docs in docs.astropy.org? (those two don't necessarily match currently though I think I saw @nstarman mention something about public API in another PR). Defining what is public API could almost be a separate APE. |
Co-authored-by: P. L. Lim <[email protected]>
Co-authored-by: P. L. Lim <[email protected]>
|
I think all four are fine in minor releases -- certainly exception and warning messages (I think it is fine to break people's tests if those are so detailed they check messages, just not to break actual code!). But very good point about public API. The most narrow (and probably best) definition is that it is only includes thinkgs exposed at the top level of astropy's submodules (with a few exceptions in |
|
How many will cry if I change |
|
Why are we avoiding double quotes? I thought English prefers double quotes and you only use single quotes if you have to quote inside text that is already in double quotes? |
|
Happy to change to double quotes! Can we just run black on the APE? 😛 Feel free to push a commit to fix this if you have time 😄 |
|
@astrofrog , I took some liberties in 22c9073 , feel free to revert the changes you don't agree with. Thanks! |
I would propose we adopt the PEP8 standard (https://peps.python.org/pep-0008/#public-and-internal-interfaces) and the typing guidelines (https://github.com/python/typing/blob/master/docs/source/libraries.rst#library-interface-public-and-private-symbols). The latter offers some small refinements to the former, namely to ensure that analyzers, static and dynamic, can easily understand what is public and help users with better tab autocomplete / code snippets / generative code, etc. In short what we need to do is define |
|
Hmm, those texts are helpful but not quite conclusive -- or at least I can read it in ways that I'm fairly sure are different from other's! Anyway, to me it sounds like if in a submodule's I guess the only part to then clarify is that a subsubmodule should only define things in p.s. Probably should move this to a different thread; sorry! |
|
@astrofrog , is your plan to discuss this APE at the Coordination Meeting? |
|
Yes but only if we can't agree on it sooner! |
|
I like the proposal for several reasons:
The difficult point is how to define an API break. Tom gave some examples in #82 (comment), are changes to exception or warning messages API breaks ? IMO It's hard to define strict rules here, it depends on the impact of a change, if the code that is changed is widely used or not etc. So when reviewing PRs maintainers will need to think if something is likely to break other people's code. There are cases where breaking the API is needed to fix a bug, depending on the impact some may be fixable in minor versions, other may have to wait for major versions with a deprecation path. I like pandas' note about this:
(And I like the way they have written their policy, it's simple so easily understandable by users) Also we probably need a way to track deprecations so we know what changes need to be done in major versions ? |
APE21.rst
Outdated
| This APE proposes the following: | ||
|
|
||
| * No longer designate any releases as LTS. | ||
| * Keep the 6-month release cycle and bump releases to x.0 every 2 years. |
There was a problem hiding this comment.
Maybe make the two years period more flexible ? 2 years still seems a good period in general but it may make sense sometimes to wait a bit more before publishing a new major version. And with the new scheme delaying a new major version has no consequences.
There was a problem hiding this comment.
But this also means that if someone wants a new thing that will totally break API, they have to wait 2 years? Do we not merge that PR for 2 years?
There was a problem hiding this comment.
Yeah indeed. This case should hopefully not happen too often, but we could imagine releasing a major version only 1/1.5 year after the previous one on exceptional circumstances ? But in general if there is a need to totally break the API there should be a long enough period of time before that with deprecation warnings, so the 2 years cycle seem realistic.
There was a problem hiding this comment.
It's worth noting that with a fixed 2-year cycle, some API changes might wait 1.9 years, and others might be merged 1 week before the deadline.
There would tend to be a bias (maybe strong?) toward the major version feature freeze deadline, because folks might have an idea and want to get it in knowing that it will be another 2 years otherwise. Something to consider!
There was a problem hiding this comment.
Yes, that's why I think we should keep some flexibility in the 2 years cycle.
But also before a breaking change appears in a major release, there should be deprecations in a previous minor release. So major release are not the time to merge anything just because we can break things in this release, we should plan beforehand what major changes are waiting, if deprecations have been released, for how long, etc.
There was a problem hiding this comment.
Agree with @taldcroft here. The most annoying numpy changes we have had to deal with are things that are merged just before the release and hence we didn't catch them in -dev.
Would it be an idea to freeze further ahead of a major release? With perhaps more advertising to test the corresponding -rc?
Alternatively (or additionally), maybe the way to do this is to be more flexible about when major releases happen, and decide on whether the next release will be major ~mid-way between releases, based on what is already ready there and what is being planned (with perhaps an astropy-dev mailing to ask what people are planning).
Part of this might also be to follow pandas' example and really make sure API changes are explained and work-arounds given.
One practical question: how is it going to work with major items? Are they just going to sit on as PR requests for ~1 year? Or is there a separate branch? As long as new features are OK for minor releases, I don't think this is a super-big issue, but probably should be discussed in the document!
There was a problem hiding this comment.
Should have read @saimn's message - agreed that if the cycle is flexible and we insist (as we should) that breaking changes have deprecation periods, that solves most of the problems.
There was a problem hiding this comment.
One practical question: how is it going to work with major items? Are they just going to sit on as PR requests for ~1 year? Or is there a separate branch? As long as new features are OK for minor releases, I don't think this is a super-big issue, but probably should be discussed in the document!
Yes new features are OK for minor releases. It's just the ones that break the API that should wait, with deprecation added in between. And that should be a minority of the PRs hopefully, so I guess we can just milestone them for the next major release and rebase when we start the major release cycle. Another way would be to have a "future" branch but if there are conflicts it will be more painful to resolve them compared to doing that for each PR.
|
Agreed that the pandas wording is very nice & clear. I suggest we use it! I think the main trick is in defining what API is. In my opinion, how something is typeset ( |
The problem is that the wording of an error message is very commonly used in tests, and sometimes used in application code. So these kind of changes can certainly break downstream code, and it once again comes down to an assessment of impact. The only way out of this I can think of is to have some policy for advertising API changes and getting a wider review. For instance, if a PR has a change that has the potential to break application code or break other package tests, then it could be brought to the attention of The "potential for breakage" bar should be set pretty low since we aren't stopping any change from happening, but doing our best to ensure that the community is aware of a change in advance. Definitely warning and error message changes would apply. |
On top of this I think we should explicitly encourage maintainers of downstream packages to run their tests against the development version of |
|
Agreed that we should encourage people to test against I do want to push a bit more to keep the freedom to change formats or error messages in minor releases. If people rely on details like that (presumably mostly in doctests), then I think it is fair to ask them to adjust, just like we adjust when numpy changes its output format (and, yes, sometimes that is painful, but definitely OK for a minor release). On error and warning messages specifically, in astropy we check those internally just to ensure we actually raise the right error, but really why would anyone rely on those outside of astropy? I don't think we have any place where we specifically look for the text of an external error or warning. And it does seem one should be free to correct or clarify exception texts, certainly in a minor release, arguably even in a bug-fix release. Anyway, I really hope we can keep things more or less like they are for minor releases! |
Hypothetically, someone could be using error messages to distinguish between errors they can handle and those they cannot. Although in that case we really ought to be using a custom error class to make it easier on those use cases (they should report that behavior to us anyways). |
|
Agreed that for that case we should have different error types (like we introduced One conclusion from that is that it is good to strongly encourage people to give feedback when things break! |
I agree with this definition. The main place where people may use the messages is tests, but breaking tests is not the same as breaking the code itself. There may be exceptions but it's hard for us to know, so indeed encouraging people to run weekly (?) tests with astropy-dev seems a good solution. |
The problem is that astropy very commonly uses generic exception types like We certainly could embark on a mission to improve that with custom error classes, but that is a different story. |
I agree 100% (as a culprit for some of the changes in question). I just think we need to ensure that such changes are appropriately communicated to stakeholders (putting on a manager's hat). |
|
And I see now that I somewhat repeated #82 (comment), so we're all in agreement. |
|
Is there a way to get some sort of consensus to move this forward? |
As I understand it, we don't want deprecations to last for 2.5 years. If something is deprecated in v5.3 and can't be removed until v7 (because v6 is too early) that's too long if v6 to v7 is 2 years |
|
I agree with @nstarman, it will become difficult to maintain multiple versions of an API for ~2 years. I also agree any deprecation should exist for at least 1 year, so it is reasonable for us to allow for having a major release every year. Since the goal is to drop LTS version support, we will then be expecting downstream users to migrate forward as we make minor releases. Consequently, downstream users should have at least 1 year of warning prior to a breaking change. |
|
I agree with the points from @nstarman and @WilliamJamieson above, which I believe are also in agreement with the summary of the Coordination meeting outcome from @eteq. @saimn - are you OK with the yearly major release? If so it seems like we would have achieved consensus, with the caveat that @astrofrog needs to update the APE's to reflect one year instead of two between major releases. |
|
I've now tried to implement the 'consensus' from the coordination meeting into these documents. The bottom line as far as I have understood is that we are basically following semantic versioning, and specifying that one year is the minimum time between major releases, six months is the default and a maximum time between non-bugfix releases, and deprecations have to be in releases for at least a year before changes are carried out. @taldcroft @WilliamJamieson @nstarman @pllim @saimn @mhvk @eteq - could you take a look at the latest versions and leave comments/suggestions or approve if you are happy with the APEs as-is? (just to see if we are getting close to consensus) |
There was a problem hiding this comment.
I should not approve as I am technically a co-author but I am fine with this. The important thing is that we get buy-in from people very involved with pipelines, who include but not limited to @nden , @perrygreenfield , and @weaverba137 .
Thank you!
APE21.rst
Outdated
| Branches and pull requests | ||
| -------------------------- | ||
|
|
||
| N/A |
There was a problem hiding this comment.
Should we also link astropy/astropy#14713 here?
|
I re-read both APEs and they sound good to me. Thanx for the reminder! |
If one year between major releases is a minimum, then I agree, but that was not my understanding from the above (every major release is yearly). That was also my point earlier in the discussion (#82 (comment)), we just need to keep some flexibility on when we want to make a major release. It will depend on how many deprecated items we have accumulated. And if there is some new feature that is waiting for the end of the (1 year) deprecation period, we can wait 6 months more before releasing the next major version. But most of time we are just deprecating old functions or some renamed arguments and there is no urgency in removing those. Also we will need a way to track the things that have been deprecated to know when they can be removed, and when we decide that next version will be a major one. |
|
I thought the consensus was that it was a minimum as @eteq said:
|
|
How many approvals are needed here? How should we proceed? Thanks! |
|
My understanding is that APE 0 makes it the responsibility of the CoCo to accept/reject APEs - the main purpose of 'approvals' here is just to see how many people are broadly on board with this but it's not like there is a 'minimum' number of approvals needed. I've asked for comments by the end of the week - if there are no objections by then and no further changes, we can hand over to the CoCo to decide how to proceed. |
|
Thank you for the approval! Should we put the final date stamp and status on this PR and merge? |
|
This has been approved by the Coco through the approvals here and internal Coco communication by those Coco members that did not comment here. I will go through the process of merging and try publishing on Zenodo etc. later today (if zenodo let's me - their webinterface has been flaky recently, if not, I might have to re-try in the next few days). |
|
Thank you to everyone who contributed and to the CoCo for approving! |
This is a draft and not ready for review - it follows the related discussion on astropy-dev. If you are interested in joining as a co-author please ping me!
APE_21.rstEDIT: Close #20