-
Notifications
You must be signed in to change notification settings - Fork 279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DOC: update "how to release" docs #3806
DOC: update "how to release" docs #3806
Conversation
99dcade
to
f6e70ee
Compare
f6e70ee
to
55e6090
Compare
Don't think I can help resolve the open questions but I will say the updated docs read nicely! Except for the |
We can probably get rid of the pr backport stuff since we now use a backport service. |
9590bcc
to
c5ab31d
Compare
doc/source/developing/releasing.rst
Outdated
for a bugfix release. If an API change is deemed to be necessary, the old API | ||
should continue to function but might trigger deprecation warnings. Minor | ||
releases may also remove some previously deprecated functionalities on the | ||
explicit condition that a warning was emitted for that piece of code in at | ||
latest one previous minor release. Such warnings are required to be visible to | ||
users by default (as opposed to builtin ``DeprecationWarning``), and state | ||
when deprecation started, as well as point to the new prefered alternative, | ||
see :ref:`how-to-deprecate`. Minor releases should happen by merging the | ||
``main`` branch into the ``stable`` branch. Minor releases should increment | ||
the ``MINOR`` version number and reset the ``PATCH`` version number to zero. | ||
Version ``3.3.0`` is a minor release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick question, @neutrinoceros -- is this in accordance with a stated policy? Or is this new?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous to yt 4.0 we didn't have a stated policy for actually removing deprecated stuff, so it was never actually removed (AFAICT), so yes, it's new in the sense that it aligns with what we're doing now, but it wasn't explicitly documented yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be worthwhile to reach a consensus on the timescale over which we remove deprecated features, rather than just creating de facto policy in a docs PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean I opened this discussion 2 years ago #2911
I seem to remember it was discussed in a triage meeting and we did reach a consensus on the policy which is already expressed in the issue_deprecation_warning
function. Here I'm just documenting it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add to this, I find that promising an exact version for when we want to remove stuff in the future is counter productive: it's the reason why we haven't been able to cut a 4.1 version yet, almost a year after 4.0. Namely, we promised to make ambiguous field keys raise errors, but we're not there yet (though I'm working on it). A much safer option is to tell that we plan to remove stuff, and try to keep it around for a couple minor versions (or longer if it's not too much of a burden), but in general I think we shouldn't try to predict the future regarding deprecations, but each deprecation cycle can be discussed in its own time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that much of the tension would be reduced by a more regular meeting schedule that involved the community, or a development process that had more participation from the impact individuals. I am not sure that we can resolve this here, but I personally find the discussion to be compelling from both sides. I have in the past been frustrated by the lack of engagement around potentially large issues and conversely by things changing when I have been unable to participate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we made mistakes in the recent past with this, but most deprecations added during the 4.1 cycle don't have a fixed expiration date so we'll have more leeway for discussions in the near future !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be worthwhile to reach a consensus on the timescale over which we remove deprecated features, rather than just creating de facto policy in a docs PR.
Update on this: since #3603 (yt 4.1), no deprecation warnings has had a clear end date. This is intentional: that way we can live with deprecated code as long as it doesn't get in the way of major refactors or features (and we also don't break any promises by dropping it as soon as it does).
Minor releases may also remove some previously deprecated functionalities on the
explicit condition that a warning was emitted for that piece of code in at
latest one previous minor release
To be clear, here I my intention was not to advocate for ever shorter deprecation cycles; I think that the longer they are, the better. I'm happy to rephrase this sentence in a way that better reflects that instead of focussing on the most agressive juriceprudence. How about this ?
"Minor releases may occasionally expire small deprecations, but expiration should only be a last resort if and when deprecated code becomes a maintenance burden."
doc/source/developing/releasing.rst
Outdated
for a bugfix release. Minor releases should *not* include | ||
backwards-incompatible changes and should not change APIs. If an API change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this sentence is important to keep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not consistent with the undergoing evolutions. We scheduled backwards incompatible changes in 4.1 and 4.2, but all of them are preceded by clear deprecation warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not encourage major API changes in minor versions, but there are exceptions when API changes are necessary. Communicating that clearly in the release docs is important because not everybody looks at our governance.
Are you proposing a change in our release structure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not. But "major" is a relative term, while "backward incompatible" isn't.
IMHO, expired deprecations are not necessarily "major" changes, but they are backward incompatible.
If we were strictly following semantic versioning, as currently suggested by the docs, yt 4.1 should be numbered 5.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Major changes are not expired deprecations. Expired deprecations are the "deemed necessary" API change that the next sentence addresses. By removing this sentence completely without revising it we are not discouraging API changes from minor version to minor version, which is not a good practice.
We should also not be deprecating functions unless really necessary from minor version to minor version. Our users are the reason the project is successful, and we need to weigh whether breaking their scripts in a minor version is urgently necessary or if it can wait until a major version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also not be deprecating functions unless really necessary from minor version to minor version.
kinda +1/2 here. IMO deprecations should appear as soon as there's a better ™️ alternative available. But I agree that expiring deprecations between minor versions is not to be encouraged (this was my motivation for #3603).
Major changes are not expired deprecations.
Thank you for clarifying this, then we're actually on the same page I think ?
How can I rephrase this to be more discouraging ? Keeping the existing phrasing still seems inadequate because it's de facto not what we're doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@munkm how about this ?
"Other than expired old deprecations, minor releases should not include backward incompatible changes."
refreshing CI |
This seems to be blocked for lack of a consensus. I still believe the docs should reflect current and actual release practices, but if we can't resolve this discussion I'd rather close this PR than leave it hanging forever. |
I think all of us believe the docs should reflect current release practices. The issues on consensus here were because proposed changes deviated from existing practices at the time. I suspect you can find consensus with reviewers here but you'll need to re-engage in conversation rather than closing the PR. |
@yt-fido test this please |
For the record I did that 2 weeks ago and didn't get any answers since. I don't have the bandwidth needed to cling to dead conversations for years, and I will close this sooner rather than later. |
Your tone here feels argumentative, and I don’t want to escalate conflict. I’m cognizant of the fact that you’ve put a lot of effort into this project and this PR. However, the changes that you’ve proposed still put in place a policy rather than exclusively documenting our release practices. Changing policy necessitates community consensus. Ending the conversation in this PR without closing the loop regarding the feedback that you’ve been given would be fairly disrespectful of the time that I (and the other reviewers) have also invested in review. I think we all want this to succeed, and any critiques that we’ve provided have been for the purpose of strengthening the published docs here. |
I've been trying to close the loop the right way but I've been waiting for an answer on your part for over a year now. At this point, my priority has shifted from making this PR succeed to getting out of the conversation. I am sorry that you feel your time is being disrespected but so do I. |
You're both right that this PR has sat idle for a long while, and it should be addressed for everyone's benefit. Let's figure out a solution, so we can move forward with things. It seems like the hold up is on the policy surrounding deprecation and then removal between subsequent minor releases. Right now, the policy that is effectively being stated in this docs PR is one of allowing for code that can be deprecated in one minor version and then removed in the subsequent minor version. What this means is that a user could be on version 4.0 using some function/class, then upgrade to 4.2 and their scripts could be broken without any notice. I'm not sure I really like that. As a user, I can see where that would be sufficiently problematic to potentially quit using a code. Is there any problem with just having deprecations remain until a major release (i.e., 5.0) when they get removed? I'm not sure where the best place to have this policy discussion is other than a PR conversation, just to make sure everyone knows about a potential policy change. |
After a fresh read of the relevant discussion, let's move this forward -- @neutrinoceros, please make a YTEP PR outlining a deprecation proposal. If you don't have time to do it now, then someone else should volunteer to do it and make sure that @neutrinoceros's proposal is correctly expressed as outlined here. That's the right venue for it and we can make sure it gets out to yt-dev. We can set a reasonable deadline for comments/changes/feedback. On the basis of the results of that we can return to this--we should aim to finish this up within a month if possible. |
To resolve the current issue and merge this PR, is it possible to just remove the text surrounding the deprecation cycle policy that still seems under discussion? I don't think there are any hangups on the remaining text in this particular PR, so once that gets removed, I think we can merge the rest of this. Then we can discuss the deprecation policy in a YTEP or in one of the user+dev group meetings that we'll be having in the next couple of months? How does that sound as a means forward for everyone? |
c5ab31d
to
e35646c
Compare
I reverted my changes to the paragraph "minor releases" (I hope that's the one you were talking about). I did start to draft a YTEP to formalise my proposed deprecation policy, but it's not ready yet. |
@yt-fido test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @neutrinoceros ! We'll bang out the policy elsewhere, but I think this PR is ready to go. Many thanks for your patience on this one!
Okay I think enough time has passed now that I can consider there are no objections to merging in the current state, so let's try getting this in: @yt-fido test this please |
@yt-fido test this please |
@neutrinoceros not a big deal, but next time could you get somebody else to merge? I don't think we were waiting on anything, but self-merging after the big discussion is a bit tricky. |
PR Summary
close #3448
I sill have a couple questions that would need answers to complete this:
pr_backport.py
still usable/maintained ? Do we want to keep it ?I've also written this in the assumption that #3758 gets merged