nipy / nibabel

Python package to access a cacophony of neuro-imaging file formats
http://nipy.org/nibabel/
Other
634 stars 258 forks source link

Request: stop throwing ExpiredDeprecationError #1287

Closed coalsont closed 5 months ago

coalsont commented 6 months ago

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".

effigies commented 6 months ago

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 img.get_data(). This followed an extensive discussion that started here and ended here. The gist of it is that get_data() returned an array with an unknowable type, and this led to errors in the wild where algorithms were written that assumed data came as a floating point array, and then failed or gave nonsense when the image was integral without scale factors. Silently changing the behavior was roundly rejected by the community, and so the decision was to provide two alternative APIs and loudly deprecate the one that was leading to known errors. Loudly deprecating the API meant issuing DeprecationWarnings, but it is also an unfortunate fact that people ignore warnings, so when the deprecation period expired, instead of simply removing, we created an error that would continue to provide migration advice. This was seen as the least bad option.

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 get_affine() and get_header() could probably have been left alone. Indefinite DeprecationWarnings wouldn't be the worst thing for one-line functions that wrap the updated APIs.

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.

coalsont commented 6 months ago

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: 1) the decision and effort to, in the first place, write a workaround that keeps code working via a deprecated function (and issue warnings) 2) the decision that the maintenance burden on a long-deprecated function has actually become excessive, and that it is now reasonable to cause old tools to break entirely, rather than expend the required effort to modify the workaround

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, numpy.asanyarray(nibimage.dataobj), seems to still work today, and I imagine that it is similar to the initial implementation of get_data() when it was deprecated. In practice, this policy change would mean instead of throwing when deprecation is beyond the promised support, the warning language becomes more severe, so something like "get_data has been deprecated since version \<whatever> and may be REMOVED at ANY TIME since version \<whatever>". As I mentioned above, I think developers do pay attention to warnings, so if a warning isn't enough to get a developer to update their code, there probably isn't a developer still on that project to engage with in the first place, and so an error isn't going to get their attention, either (and has substantial cost to non-developers).

coalsont commented 6 months ago

As for get_data() specifically, in hindsight I would argue that the error should have been much more targeted - it could test whether the internal type is floating point (or perhaps 32bit or better signed integer), and if not, only THEN give a specific, scary warning, and maybe escalate to an error. This doesn't fit neatly into the "deprecated function" framework because that isn't what it is - this approach is hazard mitigation. After all, when the internal type is float (which it often is), then get_fdata and get_data should do exactly the same thing (unless one of them ignored the scale factors, which are rarely used anyway, and could also be tested for before warning/throwing), so there isn't a good reason to make get_data error under that condition (show a less-scary warning, sure, but not error). Tools that fail only when the datatype isn't float can be worked with.

coalsont commented 5 months ago

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.

matthew-brett commented 5 months ago

@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?

coalsont commented 5 months ago

"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 get_data as an example (that generalizes to the larger point of "deprecation should not be the sole justification to throw an error"), I am suggesting the better solution would have been to have it check if the internal type was non-float, and then issue a warning, possibly escalating to an error (or maybe even be an error from the start, maybe something called BehaviorHazardError), but NOT use ExpiredDeprecationError for it (could still separately mark it as deprecated in favor of get_fdata). Then, 2 major versions later, get_data's behavior should NOT have changed to "always throw an error, even for float32 images", the deprecation message should have stayed a warning (though it could have changed the warning text to indicate less intention to keep it working if edits were ever needed), with integer-type images still throwing the other error type, for fragility reasons.

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 get_data was called on a float32 image, it had largely the same behavior as get_fdata, and yet the current deprecation system (incorrectly, IMHO) counts that as if it were "deprecated behavior" anyway.

coalsont commented 5 months ago

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.

skoudoro commented 5 months ago

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.

coalsont commented 5 months ago

@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.

skoudoro commented 5 months ago

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 😅

matthew-brett commented 5 months ago

Yes - the main point though, is that we believe that the user has used the function - here get_data - without understanding the ambiguities of its return value - and they will therefore make themselves prone to unexpected errors when using their code on other data. Of course that might be wrong - they may have understood that - but there's no way of knowing - and it's more likely that they did not understand. They will likely ignore any warnings - most of us do. So the error is the only way to signal they need to go back and review their code to see what they did intend. Very much as Python 3 forced users to consider the distinction between strings and bytes.

coalsont commented 5 months ago

prone to unexpected errors when using their code on other data

So make get_data error only when the datatype is integer, which is prone to cause misbehavior, not on absolutely every call to the function.

So the error is the only way to signal they need to go back and review their code

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".

Very much as Python 3 forced users to consider the distinction between strings and bytes

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?

matthew-brett commented 5 months ago

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.

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?

coalsont commented 5 months ago

So wait, you accept "only error if something awful is very likely to happen", but also argue AGAINST letting get_data work without an error when the image is float (when the main hazard-avoidance change in the more-predictable replacement, get_fdata, is to convert to float)? And what exactly do you see as the reason (or mistake) that resulted in get_header being treated as if "something awful is very likely to happen"? These errors seem entirely inconsistent with your statement, and to me their obvious commonality is "deprecation being treated as an error in its own right". Every function that was ever part of the API originally did something useful, under some (generally common) circumstances, and yet nibabel's current implementation of "deprecation means timebomb" always makes the entire function error unconditionally, with absolutely no attempt to detect when the old behavior would result in any hazard whatsoever.

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 get_header deprecation, and a user reported it, and an outside developer kindly contributed a fix. Then, it broke on get_data deprecation, and again, an outside developer kindly contributed a fix. These contributed fixes are less likely to happen for more obscure code, particularly if it didn't get onto a website like github. Thus, my opinion is that this silliness should really stop at the source, reducing the pain for everyone who uses nibabel.

As I mentioned earlier in the thread, github shows a bunch of issues that specifically say ExpiredDeprecationError, which appears to be entirely specific to nibabel:

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.

matthew-brett commented 5 months ago

I really don't have much comment on get_header - I can see the argument either way there. For get_data, I'm sure it's not worth going back over where the unexpected errors can arise.

Yes, I think we are nicer than other libraries like Numpy, Scipy etc, in that we raise the explicit ExpiredDeprecationWarning rather than just breaking with an AttributeError or similar. We were trying to give the user more information to go on, in fixing up their code.

coalsont commented 5 months ago

I don't see an argument for .get_header() being hazardous compared to .header, or otherwise having deserved an error, can you elaborate? I don't know what you mean about "going back over" get_data, do you mean "let's stop talking about anything remotely involving get_data", or do you mean "the damage is done, leave the current error"? It is a useful (and timely, in a sense) case for talking about the deprecation policy itself, even if get_data ends up being left in its erroring state.

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, get_header never needed to be removed (there was no hazard or maintenance burden as far as I can tell), so presumably the way many other libraries handle things, they would have just left it there and never deleted it (thus, never resulting in an AttributeError or anything else). "Nicer than this other library we have to deal with" doesn't mean the current behavior can't be improved.

matthew-brett commented 5 months ago

Just to be a bit clearer about what I mean. Here is the Numpy way of doing deprecations and errors:

Numpy 1.21.6

[ins] In [2]: np.array(1, dtype=np.float)
<ipython-input-2-11e776fef755>:1: DeprecationWarning: `np.float` is a deprecated alias for the builtin `float`. To silence this warning, use `float` by itself. Doing this will not modify any behavior and is safe. If you specifically wanted the numpy scalar type, use `np.float64` here.
Deprecated in NumPy 1.20; for more details and guidance: https://numpy.org/devdocs/release/1.20.0-notes.html#deprecations
  np.array(1, dtype=np.float)
Out[2]: array(1.)

Numpy 1.26.3:

[ins] In [2]: np.array(1, dtype=np.float)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
Cell In[2], line 1
----> 1 np.array(1, dtype=np.float)

File /Volumes/zorg/mb312/.virtualenvs/test/lib/python3.10/site-packages/numpy/__init__.py:324, in __getattr__(attr)
    319     warnings.warn(
    320         f"In the future `np.{attr}` will be defined as the "
    321         "corresponding NumPy scalar.", FutureWarning, stacklevel=2)
    323 if attr in __former_attrs__:
--> 324     raise AttributeError(__former_attrs__[attr])
    326 if attr == 'testing':
    327     import numpy.testing as testing

AttributeError: module 'numpy' has no attribute 'float'.
`np.float` was a deprecated alias for the builtin `float`. To avoid this error in existing code, use `float` by itself. Doing this will not modify any behavior and is safe. If you specifically wanted the numpy scalar type, use `np.float64` here.
The aliases was originally deprecated in NumPy 1.20; for more details and guidance see the original release note at:
    https://numpy.org/devdocs/release/1.20.0-notes.html#deprecations

So, not an explicit ExpiredDeprecationWarning, just another slightly less descriptive error name for the same thing.

coalsont commented 5 months ago

Yes, I would absolutely argue that it was unnecessary and unfriendly of numpy to actively remove np.float as simply an alias for float. It doesn't seem like it would take much effort for them to continue to support that, nor any substantial or net benefit to breaking it (even if they wanted to move it out of the "real" attributes section to "make things pretty" or something, they probably could have used some handler to swap in the new types for the old ones). And of course, they didn't even wait until a major version bump (at least, not in terms of semantic versioning) when they deliberately broke people's working, hazard-free code, so two strikes against that. Please don't emulate their misguided ideas about deprecation.

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.

effigies commented 5 months ago

Thanks, @coalsont for the discussion, and @skoudoro and @matthew-brett for your contributions.

The specific request boils down to:

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).

ExpiredDeprecationError serves the useful purpose of providing migration instructions after the functionality is removed. We will not remove it in the near term (that would break anybody who decided to catch the exception as part of their migration strategy). If we do eventually remove it, it would be because we switch to a standard approach like what numpy uses, which was not available when we created ExpiredDeprecationError.

As to the specific problem of .get_data(), this discussion has been had, and the frustrations users would experience were factored into the original decision and that hasn't changed. If a user prefers to eliminate the error rather than fix the code they are running, they can install an older version of nibabel. But some thoughtful action is required, and that is intentional.

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 .get_header() and .get_affine(), these were co-deprecated with .get_data() as part of a reconsidered interface; the marginal reduction in migration effort of restoring them, given that .get_data() needs dealing with, does not seem significant.

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.

coalsont commented 5 months ago

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 get_header to continue to work basically forever).

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 ExpiredDeprecationError is just a corollary of my contention that deprecation is not something that deserves automatic removal/expiration (or that even if it does deserve some automatic change in what message is displayed, it doesn't deserve to be an error).

As I mentioned, get_data is merely a timely example - I haven't tried to predict what function will be the next one to throw this particular error, so I can't really speak to that. But, my aim is to stop this frustration from repeatedly occurring in the future. I don't get the sense that you agree with me that the existing examples that threw ExpiredDeprecationError could have been handled in a better, less-frustrating way, so currently I am not hopeful about future repeats. In order to avoid repeating this unnecessary pain for users without any change to your custom deprecation logic, you would have to completely stop using deprecate_with_version for any reason other than "we replaced this code specifically because it has a high ongoing maintenance burden" (and remove it from any current deprecations that approach the triviality of get_header), which includes never using it as a substitute for "the developer needs to know that this function can sometimes be dangerous".

coalsont commented 4 months ago

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:

https://github.com/nipy/nibabel/blob/5d884bda234a22ce7035997c090449851a82c0be/nibabel/tests/test_removalschedule.py#L19-L23

as_int documentation states (other than it being deprecated): "This is useful because the numpy int(val) mechanism is broken for large values in np.longdouble." So, you created a workaround, for people that needed the workaround, and now that (presumably) the more obvious syntax works properly, you are going to force anyone who needed the workaround to take the extra step of going back and removing the workaround, making their code less tolerant to the library versions being used (or if they want to make their code more robust, adding a check of the version of nibabel in order to only try the workaround in pre-deprecation versions). You could have left the workaround as-is, or replaced it with int(whatever), and updated the documentation to "no longer needed, historical workaround for an issue with np.longdouble".

What are the benefits of making code that calls as_int fail, and how do they compare to the user frustration of a trivial-looking API change crashing an entire processing stream? If as_int didn't have any potentially problematic behavior, I would argue it doesn't even deserve to produce a warning (seems similar to the get_header situation, deprecation in favor of cleaner syntax, with no meaningful functionality difference).

effigies commented 4 months ago

For as_int and int_to_float, these were breaking with the numpy 2.0 refactor, we looked into it, and the complication was no longer necessary. As far as I know, nobody uses them but us, but they go through the deprecation process just in case.

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.

coalsont commented 4 months ago

That just leaves TemporaryDirectory, which your documentation says:

Create and return a temporary directory. This has the same behavior as mkdtemp but can be used as a context manager.

Upon exiting the context, the directory and everything contained in it are removed.

Please use the standard library tempfile.TemporaryDirectory

"please" implies the action is not mandatory, but then comes the "or else" threat:

Will raise <class ‘nibabel.deprecator.ExpiredDeprecationError’> as of version: 7.0

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:

https://github.com/nipy/nibabel/blob/5d884bda234a22ce7035997c090449851a82c0be/nibabel/tmpdirs.py#L39-L43

Does the decorator not throw that error?


As for the python glossary link, it has this sentence:

The main difference between a “soft” and a (regular) “hard” deprecation is that the soft deprecation does not imply scheduling the removal of the deprecated API.

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:

In general, incompatibilities should have a large benefit to breakage ratio, and the incompatibility should be easy to resolve in affected code.

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., deprecate_with_version and removal would be for worst-case scenarios only, where there is absolutely no other good choice than to break something.

An error that you don't throw is an error that you don't have to justify to your users.