-
Notifications
You must be signed in to change notification settings - Fork 4
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
Rename safe_mode to debug_mode #168
Comments
Is addressed via #175 and SciML/ADTypes.jl#68 . Will close this when they are merged. |
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. |
@willtebbutt I noticed new breaking releases of ADTypes, but it didn't seem to incorporate this change. |
There don't appear to be any breaking releases -- |
I am slightly confused: |
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. |
This is not breaking in major ways, so I won't suggest waiting for ADTypes 2.0. It also only affects Tapir's constructor |
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.
We could potentially just add a constructor, but we can't change the name of the field from |
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 |
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? |
A quick inspection of
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 |
@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. |
Fair point. |
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 |
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. |
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. |
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 |
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. |
Unless I'm mistaken, we did our best to ensure the changes to |
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? |
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). |
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. |
The inaccurate terminology was the reason behind SciML/ADTypes.jl#68, which in my view is a good middle ground. |
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
|
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?
The text was updated successfully, but these errors were encountered: