Default to not showing deprecation warnings

[oxinabox]

All julia packages follow SemVer.
That means that that adding a deprecation is nonbreaking,
and removing the thing you deprecated is breaking.

As such consider if I was a package author. Where my package's current release is v1.x.
consider if I am renaming foo(args...) to the new generalized_foo(true, args....) in v1.5.0
That means I will not bne removing the old foo function til v2.0.0

People using my package have Compat set as ="^1"
there is nothing wrong with them continuing to use the foo function forever, as long as they do not want to use v2 of my package.
Indeed there is some advantages to continuing using foo since if they want to swap to generalized_foo, they need to restrict their compat to ="^1.5", which might break compatibility with another package they are using with that might have restricted my package to ="~1.1" (idk why, maybe they need everything auditted)

People get really frustrated about deprecation warnings, even to the exent of making PRs to remove them.
And not unreasonably so -- their code works fine, why are they getting spammed with this warning message? A warning message that makes there code much slower infact.
And there code is going to keep working fine since they have the compat bounds set.

One option is to hold off all deprecation warning until the last release before v2.0.0,
say v1.9.0.
However, that just delays the problem.
Even once v2 is out, I don't want to force people to upgrade, if v1, still works fine then that is great.
So I don't want them to do up with there compat still set to ="^1", and for them toi get the 1.9 release added, and then get spammed with deprecation warnings.
They should be able to upgrade at there own pace, when they are ready.

I personally just set --depwarn=no all the time, except when specifically wanting the warnings because I am upgrading on of my dependencies.
and I think everyone else should to.
Thus this PR to change the default

cc @mlubin @sbromberger

[mlubin]

To clarify my point, it's not about my code that's running slower or printing warnings. In the case that triggered this discussion, after updating DataStructures from 0.17.10 to 0.17.11, a JuMP user started to see warnings, and their code was running more than 10x slower than before (JuliaCollections/DataStructures.jl#599 (comment)). This is a terrible user experience, arguably as bad as getting an error message from an API breakage—at least that would be more obvious and require less time to debug.

Supposing that one of my constraints as a package developer is to avoid terrible user experiences like the one above, the best action I can take is to defensively freeze my dependencies to exact patch releases. This is bad for the ecosystem overall because overly tight version requirements will create version conflicts elsewhere.

I support --depwarn=no as the default. Visible deprecations should be opt-in and used when you decide to update your dependencies.

[sbromberger]

Also, just to take issue with the implied conclusion that because

All julia packages follow SemVer.
That means that that adding a deprecation is nonbreaking, and removing the thing you deprecated is breaking.

we cannot introduce breaking changes in a minor version or patch release.

This is explicitly not true for versions < v1.0.0 (see Specification Standard #4):

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

Please learn from my experience and use this standard to your extreme advantage, because once you get to v1, your hands become considerably more tied.

That said, I don't know how I feel about --depwarn=no as a default. It's a useful sign that a) things are in flux in some part of your ecosystem, b) you should probably figure out why, because c) if one of the upstream (transitive) dependencies is moribund, your code will start failing.

Also "that said", I understand and agree with @mlubin's concerns that it is a suboptimal user experience. But there is a risk to running v0 code, and library developers need a chance and flexibility to finalize an API. If an upstream relies on a v0 dependency, then it's up to the upstream to keep on top of things, as there should not be any guarantee of API stability if we're SemVer purists.

[mlubin]

This is explicitly not true for versions < v1.0.0

Julia explicitly doesn't follow that rule. https://julialang.github.io/Pkg.jl/v1/compatibility/#Pre-1.0-behavior-1 says:

When the minor version is not zero, versions with the same minor versions are considered compatible, i.e. the two versions 0.2.1 and 0.2.3 are considered compatible.

[sbromberger]

Julia explicitly doesn't follow that rule

Yes, I understand, but it still relaxes SemVer below v1. In this case it would've been more appropriate to release 0.18.0 instead of 0.17.11.

[oxinabox]

That said, I don't know how I feel about --depwarn=no as a default. It's a useful sign that a) things are in flux in some part of your ecosystem, b) you should probably figure out why,

I see your point, but I don't think normal users should be exposed to this process of discovery.
A developer should opt in to checking for this, via --depwarn=yes.
The final downstream user might not even know what a deprecation is, and thats ok.
Its doesn't make my list of top 20 programming concepts/terms to understand.

Looking at deprecations is its a useful sign yes, to go looking and check what is up.
But if the answer is: (to put a exteme example) "a well maintained dependency of a dependency, is preparing for a v2.0 release and has promised long-term support and backporting of bugfixes to v1 for the next 2 years" then you can happily sit on that information, knowing there will not be any issues.
But you can't then tell all your users, no matter how far downstream to run with --depwarn=no.

And you don't want to cap the version of your dependency of a dependency -- else you will missout on those backported bugfixes.

[mlubin]

@sbromberger, regardless of the specifics of SemVer, it seems that we're in agreement that if deprecation warnings are displayed by default, then a deprecation should be treated as an API change when deciding the version number of the next release. I think this isn't the intention of deprecations, so setting --depwarn=no resolves that inconsistency.

[sbromberger]

it seems that we're in agreement that if deprecation warnings are displayed by default, then a deprecation should be treated as an API change when deciding the version number of the next release.

Well, I only agree with this because of the performance impact of the depwarn. If the only change was a warning being issued, then no – I would disagree that it counts as an API change.

Here's an example: in LightGraphs, right now, we are at 1.3.1 officially. For 2.0.0 we are planning a comprehensive, full-scale API rewrite to most of our codebase. The plan was to release 1.4.0 with deprecation warnings and 2.0.0 with the deprecations (and old code) removed, similar to how Julia 0.7/1.0 was rolled out, except we're beyond version 1.

If depwarns were API changing, then we'd have to release 2.0.0 with the deprecation warnings, and then 3.0.0 to remove them, since the API really changes when we prohibit people from using the old function names. This seems ... silly.

[mlubin]

I don't agree that the performance impact is the only issue. It's also a bad user experience for JuMP users to see warnings that they have no need to understand about JuMP internals (i.e., how JuMP uses certain libraries). That said, the issue still stands on your terms if there's no way to mitigate the performance impact of deprecation warnings in general.

[sbromberger]

the issue still stands on your terms

Well, no. Let me walk it back a bit after having applied the proposed rules to LightGraphs: would your proposal be to release 2.0.0 with deprecation warnings, simultaneously with 3.0.0 with the deprecations/old code removed? Or would it be 2.0.0 with depwarns, 2.1.0 without depwarns? Or something else?

Right now our plans are to release 1.4.0 with depwarns, and 2.0.0 with the code removal.

[tkf]

If you were to do this change, isn't it better to tweak Pkg.test also so that it is run under --depwarn=yes? That is to say, make sure that the developers see the warnings?

Alternatively, maybe the default logger can do a bit more clever thing like printing deprecation warning only if the packages causing it is deved or in LOAD_PATH (e.g., activated).

[sbromberger]

If you were to do this change, isn't it better to tweak Pkg.test also so that it is run under --depwarn=yes? That is to say, make sure that the developers see the warnings?

That can break some tests that rely on test_warn output.

[mlubin]

@sbromberger The hard constraint I would like to impose is to avoid the following situation:

• Library Z has specified a dependency on LightGraphs 1. It's using the library correctly and assumes that future releases of LightGraphs follow SemVer. LightGraphs 1.4 is released. The users of library Z run pkg> up and get LightGraphs 1.4. The users are flooded with warnings about the internals of library Z, and their code might run substantially slower than before.

There are a number of ways to avoid this situation. Turning off deprecation warnings by default is one. "2.0.0 with depwarns, 2.1.0 without depwarns" could also work.

[tkf]

That can break some tests that rely on test_warn output.

Why not use @test_deprecated? Also, I think it's better to make the test robust so that it works under any flags. If you want to cover multiple flags robustly in your tests, use something like https://github.com/tkf/PerformanceTestTools.jl

[sbromberger]

"2.0.0 with depwarns, 2.1.0 without depwarns" could also work.

This raises the question as to when a change is considered breaking. For you, I conclude it's when performance or output changes. But to what extent? Performance improvements are certainly non-breaking. Are small performance regressions? Where do we draw the line?

I'm not necessarily disagreeing. Until this conversation my feeling was the API was breaking "when functions that used to work with a given set of arguments no longer work with those arguments (or at all)". I can see the other view. But you can't hold both views unless you want "2.0.0 with deprecations, 3.0.0 without", which seems like an awful idea.

[sbromberger]

Sorry for the double-post.

cc @StefanKarpinski since he and I have discussed this subject recently.

[sbromberger]

Why not use @test_deprecated?

Because you're testing to make sure a function emits a (non-deprecation) warning correctly, not testing to see whether it's been deprecated.

cf https://github.com/JuliaGraphs/LightGraphs.jl/blob/master/test/distance.jl#L62-L65

[mlubin]

This raises the question as to when a change is considered breaking.

I don't think we need a complete answer to that question for this issue. A 10x performance regression that users noticed and complained about crosses the line for me. Also printing confusing warnings (about a library's internals) is breaking in my book. As a JuMP developer I want to be able to control the output that the user sees. We try very hard to avoid error messages and warnings that relate to library internals.

[tkf]

Why not use @test_deprecated?

Because you're testing to make sure a function emits a (non-deprecation) warning correctly, not testing to see whether it's been deprecated.

This can be easily fixed by improving the tooling or the way the test is written or executed. I don't think it's a good reason to make it hard for the developers to see the deprecation warnings.

[sbromberger]

This can be easily fixed by improving the tooling or the way the test is written or executed.

PRs gratefully accepted.

[KristofferC]

--depwarn=no by default makes sense to me, a large majority of deprecation warnings are likely from how one of your libraries use the API of one of its libraries which as a user you don't care about.

[JeffBezanson]

I agree --depwarn=no seems like the right default now. Adding text output and a large slowdown at runtime is effectively breaking (we have added some load time warnings, which is a borderline case, but I think far, far less breaking). Also everybody should feel free to use @deprecate without worry, otherwise it's as if the feature didn't exist.

[JeffBezanson]

Note julia_cmd needs to be updated.

[tkf]

If --depwarn=no is the default, when do people see the warning?

[KristofferC]

When running with --depwarn=yes? We should make sure that it is possible to opt in to that in Pkg.test.

[KristofferC]

Whitespace needs fixing in NEWS file.

[tkf]

You can already do Pkg.test(julia_args=`--depwarn=yes`). I don't think you need to do anything if it is opt-in.

But I doubt people will do this in CI if this is not the default. It then, in turn, makes it very hard to use it. Imagine that tens of transitive dependencies printing deprecation warnings. How do you find the deprecation warnings from your package? It practically makes @deprecate a no-op.

(OK. It is technically possible to develop a logging filter to focus on your packages. But then you have to be actively configuring packages to filter-in. Warning that requires your focus does not seem to be a good warning system.)

[JeffBezanson]

Triage ok's this; any further improvements can be considered separately.

[tkf]

Did triage acknowledge that, after this change, there is no reliable way for package authors to deliver a warning to other developers of downstream packages? (I'm asking this simply because the only way I can think of for triage to approve this PR is that they missed this aspect.)

[mlubin]

@tfk Developers of downstream packages should turn on deprecation warnings when updating the libraries they use. Worst case, when they update a library, they get a missing method error and have to take a step back and re-run the last working version with deprecation warnings on. Admittedly this is a change in the current workflow, but it's far from having "no reliable way" to deliver the warning.

[tkf]

Developers of downstream packages should turn on deprecation warnings when updating the libraries they use.

As I've explained this before, this requires developers of all transitive dependencies to do the same. This is unlikely to happen in distributed OSS ecosystem built by developers with heterogeneous skill sets.

This also makes updating compat entries harder. Before this PR, you can just look at the result of the standard CI run by CompatHelper.jl (well, ideally, this would be the case). After this PR, you have to run the test suite manually.

It also makes performance regression silent. Note that deprecated method does have performance overhead even under --depwarn=no. You need to start debugging the performance bug or guess that it might be the deprecated method. Recall that this can happen at any transitive dependencies between your package and the upstream package introducing the deprecation warnings.

Please note that I'm only suggesting to include --depwarn=yes in Pkg.test. I'm sorry if I missed any, but so far no one explained exactly why it's harmful to do so during the test. IIUC the motivation of this PR is to hide it from the end-users.

[KristofferC]

Please note that I'm only suggesting to include --depwarn=yes in Pkg.test

This has very much not been clear (at least to me) from reading your comments. That seems reasonable to me.

[tkf]

@KristofferC Sorry, I've been fooling myself that I made my point clear as that's the first thing I said in this thread. But I should have re-iterate my point when replying to you as my first comment was way above in the thread.

[sbromberger]

I'm really worried about how this might slow adoption of our upcoming LG2.0 release. If people (both direct users and authors of libraries that use LG) don't see deprecation warnings, they won't know that a new major version is available, and there will be no reason to look.

This may also force us into supporting 1.x for much longer than we normally would.

(This even impacts a plan we were considering to issue a single depwarn on LightGraphs 1.x saying that this version is not the latest, and to check code against a custom branch before moving to 2.x. I don't know how we get folks to move across major versions without notifying them, and this removes one main avenue for such notification. I guess we could use @warn with maxlog....)

[KristofferC]

I guess we could use @warn with maxlog....)

👍