Rename safe_mode to debug_mode

[yebai]

It’s not obvious what safe mode is, and what limitations it induces. Let’s document that.

EDIT: also, given the significant performance penalty of using safe mode, does it make sense to enable it by default?

[willtebbutt]

Is addressed via #175 and SciML/ADTypes.jl#68 . Will close this when they are merged.

[willtebbutt]

This is going to stay open so that we don't forget to rename to debug mode at some point. I've let @gdalle know that we would like to do this at some point, so when a breaking change in ADTypes next happens, we'll bundle it in with that.

[yebai]

@willtebbutt I noticed new breaking releases of ADTypes, but it didn't seem to incorporate this change.

[willtebbutt]

There don't appear to be any breaking releases -- ADTypes is still in version 1.x.x.

[yebai]

I am slightly confused: v1.5 --> 1.6 is breaking, correct?

[willtebbutt]

No, 1.5 -> 1.6 is a minor (feature) release. 1.5 -> 2.0 is an example of a major (breaking) release. See e.g. https://semver.org/ for details.

[yebai]

This is not breaking in major ways, so I won't suggest waiting for ADTypes 2.0.

It also only affects Tapir's constructor AutoTapir, so if needed, we can support both constructors AutoTapir(debug_mode=false) and AutoTapir(safe_mode=false) but throw a depreciation warning for the latter.

[willtebbutt]

This is not breaking in major ways, so I won't suggest waiting for ADTypes 2.0.

This is something that you'll have to discuss with @gdalle -- I do not have control over this, and I don't view this as being sufficiently important to really push him to make a breaking release sooner rather than later.

It also only affects Tapir's constructor AutoTapir, so if needed, we can support both constructors AutoTapir(debug_mode=false) and AutoTapir(safe_mode=false) but throw a depreciation warning for the latter.

We could potentially just add a constructor, but we can't change the name of the field from safe_mode to debug_mode (as this is breaking). My preference is to change it all, or change none of it, rather than have two things for users to look out for.

[yebai]

1.5 -> 1.6 is a minor (feature) release. 1.5 -> 2.0 is an example of a major (breaking) release.

Is this true in Julia's world? My impression is that Julia 1.7 is fairly breaking (compared to Julia 1.6), for example, which doesn't seem to satisfy semver.

[willtebbutt]

This is definitely also true in Julia -- there have been no breaking changes to public functionality since 1.0 (or, at least, nothing intentional). What kinds of changes are you thinking about in 1.7?

[yebai]

A quick inspection of HISTORY.md:

• Julia 1.6 release note: The postfix conjugate transpose operator ' now accepts Unicode modifiers as suffixes, so e.g. a'ᵀ is parsed as var"'ᵀ"(a), which can be defined by the user. a'ᵀ parsed as a' * ᵀ before, so this is a minor breaking change (https://github.com/JuliaLang/julia/issues/37247).
• Julia 1.9 release note: Specifically, this results in the following breaking changes:

I'm not saying these are major, but it does seem that minor breaking changes do happen from time to time, for good reasons.

EDIT: my impression about Julia 1.7 is mostly compiler-related, which causes a lot of headaches in Libtask.

[yebai]

@gdalle, given Tapir's current relatively small user base, I think it is better to change this now for clarity reasons mentioned in SciML/ADTypes.jl#68.

[willtebbutt]

I'm not saying these are major, but it does seem that minor breaking changes do happen from time to time, for good reasons.

Fair point.

[gdalle]

I would rather add a new keyword argument and deprecate the old one. Even though it's minor, the removal/renaming alone would break things e.g. for people using Tapir through DifferentiationInterface

[willtebbutt]

I've opened a PR here: SciML/ADTypes.jl#75

It's quite ugly which, in combination with the inconsistency it introduces, makes me think we're better off waiting until ADTypes makes a breaking release to do any of this. @gdalle @yebai please let me know what you think.

[gdalle]

Honestly I think we should leave things as they are. Tapir is free to interpret the "safe mode" argument as you see fit, so the renaming is not really worth our sweat.

[gdalle]

Thats why I closed the PR. If a v2 of ADTypes ever gets discussed we can revisit this, but that could be literal years away so in the meantime let's move on

[yebai]

IIRC, there have been quite a few breaking changes to the AutoReverseDiff constructor recently. So, I find it hard to understand why the bar is raised so high for AutoTapir, which is used by much fewer people than AutoReverseDiff.

[gdalle]

Unless I'm mistaken, we did our best to ensure the changes to AutoReverseDiff were not breaking, by preserving the old behavior with the compile field.
It's not a question of package-dependent bar height, it's a question of avoiding breaking changes whenever possible. And renaming a keyword argument because you don't like how it sounds is a very very minimal change, which means you can definitely live with the old kwarg name.

[yebai]

But @willtebbutt’s PR only depreciates a constructor, precisely like those messages I see from AutoReverseDiff. So, it is precisely in the situation of package-dependent bars, right?

[gdalle]

The key difference is that your request is a purely esthetic change, which you can easily live without (whereas the ReverseDiff change had performance implications for static dispatch).
In my view, purely esthetic changes like this one do not justify additional code complexity in this package. ADTypes is a central package and we want to keep it as lightweight and easily maintainable as possible. Every PR is weighed on the cost/benefit tradeoff, and in your case the benefit is basically zero.
I understand if you disagree. But ultimately it's up to me and other maintainers of ADTypes, so unless one of them disagrees, I'm afraid it will stay that way.

[yebai]

I don’t have strong feelings here, but I want to point out that inaccurate terminology causes issues and sometimes subtle bugs. I didn’t see the value making it so hard to depreciate a constructor. We write complex code where necessary, and ADTypes will likely become complex at some point due to various reasons.

[gdalle]

The inaccurate terminology was the reason behind SciML/ADTypes.jl#68, which in my view is a good middle ground.
For instance, if you wanted to turn safe/debug mode off by default, I would definitely consider the change because it has performance benefits. But for a simple renaming, I don't think it's worth it.

[gdalle]

It's not black and white, it's about how much complexity we introduce versus how much benefit we reap. For instance there's gonna be a breaking change for Enzyme pretty soon (SciML/ADTypes.jl#74), but it's

1. gonna be done properly by yanking incorrect releases 1.6.0 and 1.6.1 from the general registry (it's more of a bug fix if you look at it that way)
2. essential for performance with AutoEnzyme + DI