-
Notifications
You must be signed in to change notification settings - Fork 262
Request: stop throwing ExpiredDeprecationError #1287
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
Comments
Hi Tim, thanks for reaching out. I strongly sympathize with this post, and I too resent needless churn from upstream dependencies. I'd like to explain the logic here. The primary API that has been deprecated that anybody uses is There are other cases (#1046, #1198) where nibabel has made it too easy for careless users to write files that cause problems for other tools, and the only way to deal with that without silently changing behavior is to error out and cause people to consider how best to modify their code. In each case, the breakage has been deliberate but measured to address the problem as narrowly as possible, precisely because we don't want to break code that has been doing the right thing when we can help it. I grant that I do dispute the idea that there is zero cost to leaving deprecated APIs. Python is an evolving ecosystem, at times much faster than I'd like. As our dependencies (mainly numpy) and Python itself update their behavior and modify their APIs, every deprecated function is an additional maintenance burden. Additionally, there have been convoluted hacks required to keep deprecated APIs around as long as we do; finally removing them allowed us to greatly simplify the remaining code (I'm thinking of the GIFTI module here). All this said, we can consider alternative approaches to deprecation. I don't have a proposal just now, and will think over what you've written, but I hope I've made clear that the approach we have taken has been a considered one, not churn for its own sake. |
To be clear, I do appreciate you supporting deprecated functions for as long as you do, and "deprecation" is, in a general sense, the right thing to do in many circumstances, including the ones you have encountered. What I disagree with is the deprecation turning into an error despite the deprecated implementation still being valid, working code, and especially if it is likely to be valid and working long into the future with virtually no edits. Yes, it would be nice if all code in existence could be updated to your improved API before this specific error affects any end users, but that isn't realistic, and errors that aren't the end user's fault can be a considerable additional burden on them. If a developer is actively maintaining a piece of code, I think it is likely they will notice warnings and modify the code to fix them. If a warning is going unnoticed, it is likely because there is no developer working on that code, and thus turning it into an error is likely to mostly hit end users. Fundamentally, I still see deprecation and removal as two separate decisions:
At present, your policy on (2) doesn't seem to take into account the degree of ongoing maintenance burden for a particular workaround, so instead of "deprecated functions will be supported for at least 2 major versions, plus as long as it doesn't require additional effort", you seem to have implemented instead "deprecated functions will be supported for at MOST 2 major versions, and then will artificially crash and burn, regardless of whether any significant effort is required to keep that particular code working". I imagine the idea here is that you want only major versions to break the API, and the only way to do that and limit the maintenance burden is to cause code that works fine to artificially fail in advance of needing to update it for changes in a dependency's API. I'd like to suggest that you consider amending that part of the policy to something like "the non-deprecated API will continue to work for at least 2 major versions", so that you can leave the "cruft" (which keeps abandoned code working) in place, and let it break only when there is disproportionate maintenance effort required to keep it working. The get_data() suggested like-for-like replacement, |
As for |
I guess to summarize (and poke the thread in hopes of additional response), I believe the only user-beneficial reason for a function to throw an error is because that function does something so unpredictable or harmful for inputs with certain characteristics, that nearly any code that used it on such inputs is probably doing something wrong (and thus, it should check for such problematic input characteristics before deciding to error - presumably the function was usable in some cases, or it wouldn't have been part of the API in the first place). Deprecation is not the reason that such a function should throw an error (instead, the sometimes-problematic behavior is the true reason behind both the replacement API, and the conditional error), and thus, deprecation itself is never the true reason for a user-beneficial error condition (maintenance burden can be a sufficient justification for a different category of error, but it is not directly user-beneficial, and "maintenance burden" is also not equivalent to "this function was deprecated a while ago"). Thus, I argue that the ExpiredDeprecationError type should be deleted, and every instance of it replaced with simply a stronger warning (but particularly the automatic "if after X version, throw" check). I believe that it is net detrimental to give deprecation an artificial, predetermined expiration condition. While "zero cost to leaving deprecated APIs" may not be accurate (when what I actually meant was leaving old functions as-is with deprecation being only a warning, for as long as they work without edits), the statement "every deprecated function is an additional maintenance burden" also seems like an exaggeration, and certainly different functions will have different maintenance burdens, with some at least approaching zero. |
@coalsont - just to clarify - you are suggesting we should leave the deprecation warnings in place for ever, more or less? Even where the deprecated behavior is likely to lead to fragile and unpredictable behavior? |
"Fragile and unpredictable" is dependent on the input data or other factors, it is not dependent on the designation of "being deprecated". If there are good reasons to cause an artificial error, that is entirely separate from "ExpiredDeprecationError" or deprecation in general. I am suggesting that deprecation, by itself, should not ever turn into an error on its own. Edit: using nifti I guess part of the confusion is that you are talking about "deprecated behavior", while what the deprecation actually seems to deal with is an entire function. When |
To come at this from a different angle, I think deprecation means only "new code should avoid using this function", regardless of the reason why. So, if you have a messy API with a lot of repeated functions, and you replace it with a new API where the difference between those functions is instead passed as a parameter to a smaller, less repetitive set of functions, it is a good idea for new code to use the new API, but there is no reason for everyone to rewrite all their existing code onto the new API, and therefore no reason to make the deprecated API throw an error (in this trivial case, you could rewrite the old API to just pass through to the new API, and then it should work without edits or hazards or fragility, forever - you don't need to add features from the new API that the old API didn't have when it was deprecated). It doesn't make sense for this to cause old code to fail (or even cause a warning in this case, really), but to me it is in fact deprecation in exactly the same sense as anything else - there are new functions you should use in new code, rather than the old ones. Thus, to my mind, any question of how old code should behave (when/whether the old functions should produce an error, or even a warning, and why) is completely independent of the status of being deprecated (which is only about new code, and thus has no business saying what should happen to old code). My hand-wavy answer is "old code should behave the same way it used to, except in conditions that made it almost certainly wrong, for as long as it can, until the effort required to keep it working is not acceptable", which implies judgement calls and narrow hazard mitigation, instead of counting versions and then exploding. |
old code work with a specific version. why not pin the library (here nibabel) version? Why we should expect old code to work with new version ? this is what I do not get in your explanation. I suppose the expired deprecator error tell you that. Update your code or pin your version and it will always work as expected. I might be wrong or miss something. EDIT: I think I understand your logic and agree if there is a "minor" version change. However, nibabel seems to follow quite well the semantic versioning X.Y.Z. I believe (I need to check) the deprecated error message appears only during a major version change which means in theory "incompatible API changes". I would not expect any code old code to work without any update or version pinning. |
@skoudoro The error is being reported by users against packages that use nibabel, and may no longer have a maintainer. Since the error is artificial, based only on numbers changing, why throw it in the first place, as it unnecessarily creates a situation where these users need to figure out that they have to pin the version of nibabel (and how to do so)? How many other packages will need to also be pinned for a specific old version of nibabel to work, when it could have all been avoided by not throwing an artificial error based on "this function is old"? I am suggesting that long-deprecated functions not be considered part of the "API" for the purpose of "must not break" in semantic versioning, and thus, there would be no need to artificially break them at some predetermined major version boundary when they still work. |
ok, thank for the explanation, I understand much better your point and now I see both sides of the argument. I find it tricky and I do not want to rush an answer 😅 |
Yes - the main point though, is that we believe that the user has used the function - here |
So make
I vehemently reject the idea that an error is a reasonable way to make a "loud warning". Errors should always be reserved only for impending catastrophe, when the code knows something awful is virtually certain to happen if execution continues, and it needs an escape route. Errors prevent end users of scripts and programs from doing their work, and if the code is abandoned, there is no developer to get the attention of in the first place. In addition, I doubt the premise of "too many developers just ignore all warnings", and I reject the implied "and it is my job to force them to pay attention", and in particular what seems to be the implied attitude of "by any means necessary, no matter the cost to end users".
comparing nibabel to python3 seems excessively grandiose, but let's try that comparison: python
nibabel
@matthew-brett I am a bit frustrated that your counterarguments seem to miss the point that this error type is hitting end users rather than just developers, and they may not be equipped to deal with it to implement their workflow after an OS upgrade, or deal with any future timebombs set by nibabel, if this policy continues. Even if you reject my argument that "deprecation is never the fundamental reason behind a good error", why should any developer have to spend any effort defusing these timebombs in old code for data where it can still be expected to work correctly (such as float-encoded images)? Someone tested it on some data back when they wrote it the first time, and it did what they wanted, it can't be the case that the original behavior is always wrong for what they want to do, so why should it always error? |
Right - this is judgement call, with some knobs to tune. My own threshold is "the code knows that something awful is very likely to happen". You might not accept our argument on that. I'm sure you know - but all Python libraries I know do this, deprecate and then remove - hence - for example, we have to keep updating to keep up to date with expiring deprecations on Numpy and Scipy features. Of course Nibabel is somewhere between a library and an application - another knob to tune. Just to be specific - can you say more about the particular application or applications that are crashing, and can't easily be updated? |
So wait, you accept "only error if something awful is very likely to happen", but also argue AGAINST letting No, I am not specifically aware of any other library in any language that thinks that deprecation should automatically become an error at some pre-specified point (granted, my work has not involved writing any python to speak of), though "ad populum" has never been accepted as solid reasoning (for either side). I would make exactly the same argument against them, if they caused such an error in code that I have some responsibility for. Overly-broad errors are bad for compatibility, and old code that has worked for a long time really doesn't need to be broken and therefore use some of a developer's time+attention. Removing deprecated code before it actually presents a maintenance burden is not something I have much data on, but I would hope it is rare (as an admittedly unfair comparison, the latest debian stable still officially supports 32-bit x86...technically i686, which dates to 1995). The package of my direct concern is gradunwarp (https://github.com/Washington-University/gradunwarp). It was abandoned by its original authors long ago, and taken in by the HCP many years back, but for a while we haven't had a python developer actively working on it (it worked properly, and didn't need any new features). Then, it broke due to As I mentioned earlier in the thread, github shows a bunch of issues that specifically say https://github.com/search?q=ExpiredDeprecationError&type=issues Presumably some of these are users reporting after their workflow started failing, and this doesn't even try to count "one-off" scripts that didn't get into any kind of version control, but did find their way into an important workflow anyway. |
I really don't have much comment on Yes, I think we are nicer than other libraries like Numpy, Scipy etc, in that we raise the explicit |
I don't see an argument for More information is nice, yes, but to the user that doesn't know how to fix it, an error is an error, and disrupts their work regardless, and then they need to find a developer to spend time on fixing it. In my opinion, |
Just to be a bit clearer about what I mean. Here is the Numpy way of doing deprecations and errors: Numpy 1.21.6
Numpy 1.26.3:
So, not an explicit |
Yes, I would absolutely argue that it was unnecessary and unfriendly of numpy to actively remove Edit: numpy 1.20 was released in 2021, 1.26 in 2023, so only ~2 years from a trivial, hazardless deprecation warning into an error. I think that counts as a third strike. |
Thanks, @coalsont for the discussion, and @skoudoro and @matthew-brett for your contributions. The specific request boils down to:
As to the specific problem of For what it's worth, I and several others I've seen have put effort into helping users and developers understand the issue and adapt code. As to I think what there is to say has been said. I do appreciate your concerns and frustrations, and I hope you can appreciate that we did not end up here capriciously even if you disagree with the decisions. We will continue to be mindful of the impact of our API changes, and I will definitely keep this conversation in mind when weighing our options. |
That isn't really the essence of my request, I was not arguing that informative messages should be removed, and this particular error class is not the only easy way to provide such a message after code is removed (it is also surprising to me that you put the effort into deletion commits for deprecated functions, and even for writing tests to ensure that deprecations will cause an error, that is more than the "maintenance effort" of "do nothing" for something as simple as The real essence is "deprecations shouldn't have built-in automatic removals or expirations, that should be a manual decision of 'we don't want to put the effort into fixing it for this new problem that broke it'". It is fine to continue to give migration instructions in whatever warning (or eventually, error, if/when truly necessary) is shown, the problem is that premature removal of still-working functions is being treated as a given, "of course we have to break it, because it isn't the current API, it doesn't matter that the implementation still works and abandoned code still uses it", and that presumption is the root cause of easily-avoidable pain. What you are telling users is "I promise to break your code if you haven't made some otherwise-unnecessary edits in 2 major releases". Removing As I mentioned, |
Well, since I found the tests to ensure deprecation turns into an error "on schedule", how about we discuss these errors that are scheduled to happen at 8.0.0, before they turn into similar user frustration: nibabel/nibabel/tests/test_removalschedule.py Lines 19 to 23 in 5d884bd
What are the benefits of making code that calls |
For That said, I'd be fine with soft deprecating these and just having it be a documentation update. We've already fixed them so they'll continue to work for the foreseeable future. |
That just leaves TemporaryDirectory, which your documentation says:
"please" implies the action is not mandatory, but then comes the "or else" threat:
Again: what is the benefit of making this throw an error? Interesting that the discrepancy between the documentation and the test code suggests it has been bumped to 8.0 when it was originally set for 7.0, perhaps to be "nicer" to users (but not as nice as just keeping it). Edit: decorator is set to 7.0, not sure why the test uses 8.0: Lines 39 to 43 in 5d884bd
Does the decorator not throw that error? As for the python glossary link, it has this sentence:
While this unfortunately looks like a recommendation for "scheduled removal" being the "regular" type of deprecation, in context, I believe it is actually about discussing and making changes to the base python language - in essence, if an enhancement isn't a breaking change, the way I read this is that python generally doesn't even use the word "deprecated" in the context of obsolete but usable functionality, in order to avoid ambiguity and scaring users. Following the link to PEP-387 additionally has, in the backwards compatibility section:
The way I read that, it has hidden-ish advice of "first, reduce incompatibilities as much as possible, to improve the benefit to breakage ratio", which means "try to do it without causing any errors at all, and therefore don't (hard) deprecate if you can possibly avoid it" appears to be the (unfortunately somewhat hidden) advice. So by my (admittedly motivated) reading, I think python's docs are also saying "please don't hard deprecate anything that you don't absolutely have to, because users don't like it when things break", i.e., An error that you don't throw is an error that you don't have to justify to your users. |
Nibabel is used in research settings, which somewhat often results in a tool getting written and then abandoned by its original author (while still being useful to users). Fast forward a few years, the original nibabel API deprecates something, another few years, and on a new nibabel version release, suddenly the abandoned code errors in new installations, not because it is doing anything different or the input files changed, but merely because nobody was maintaining the code to edit it for the API enhancement, and people using the tool have to either hold back the nibabel version, or find a developer to edit and test the API change.
As long as you don't have a need to reuse the same function name for a different purpose (and it is almost always easy to find an alternative name instead), I think there is very little to be gained from making calls to it throw an error (as opposed to a warning), while throwing causes pain for end users. "Streamlining" by removing code just because a newer/improved alternative is available doesn't seem particularly compelling to me (it is an extra step to delete the code, and for what real benefit?) - I would think you could have the documentation move deprecated functions to a less prominent location, and probably make most autocompleters not display them at the top of the suggestions, without outright deleting them. What percent install size does it actually save? What other benefits do you see that could justify the pain to users when an old tool stops working?
Relevant search shows 50 issues on github:
https://github.com/search?q=ExpiredDeprecationError&type=issues
Alternative title: "ExpiredDeprecationError considered harmful".
The text was updated successfully, but these errors were encountered: