-
-
Notifications
You must be signed in to change notification settings - Fork 11
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
Deprecate "safe_mode" in favour of "debug_mode" for Tapir.jl #75
Conversation
For clarity reasons, I think we should merge changes here. As we previously discussed, the term “safe_mode” is misleading. The slightly ugly code will be removed in 2.0, so it won’t hurt too long. |
I don't think a v2.0 of ADTypes is anywhere near on the horizon. The switch to v1.0 was only mildly breaking and it was already a nightmare (I should know, I helped handle most of it). I'm making an executive call here and saying that it will remain called "safe mode" in ADTypes for the foreseeable future. The translation between that and whatever Tapir does will happen in DifferentiationInterface, so feel free to make changes there or to update the docstring here. I'm closing this PR however. |
That’s quite unfortunate and discouraging, these types really “belongs” to individual packages in my view. What happens here effectively sets a precedence that no breaking changes can happen at the interface level in AD packages unless you got approval of ADTypes admin. |
I mean, that's what package admins do, they approve changes or they don't. I'm a package admin of ADTypes and I don't approve this specific change, because it's too much hassle for too little gain (even the PR author was not convinced). Breaking changes can definitely happen in AD packages, and they regularly do. However, our goal is to avoid a breaking release of ADTypes, because that would be extremely disruptive to the whole ecosystem (the last one took weeks to apply and a lot of sweat). If you are an AD developer and want your package to be present in ADTypes (and DifferentiationInterface), that means you need to keep working with some design decisions you made a while ago, even if they are not ideal in retrospect. |
As for the types "belonging to the individual packages", it's like saying that every package should maintain the DifferentiationInterface bindings. It's great in theory, but in practice you need a centralized push (in this case from me) to make things happen. Maybe in a distant future where DI is stable and widely adopted we can decentralize, but definitely not now. |
We can still deprecate without breaking. Since Tapir is generally not used doing a break on its semantics would not be hard to update the world to, we could just blind merge almost all PRs. |
Yeah that's what this PR does. But the benefit of making the constructor so dirty for a 4-character renaming |
It's not a good thing that's for sure. But Tapir isn't in production mode, so I'd be willing to... let it slide... Might as well change the field name too and set a getproperty overload for that deprecation. Or let @willtebbutt make the call if we can just do a break to Tapir users. Since the set of users is currently probably no more than a handful which includes us in the thread and mostly for benchmarks, it falls into the "experimental" territory and I'd let the decision be up to @willtebbutt, since I'd hate to have this 3 years from now because we didn't want to break 1 benchmark code. |
Personally, I would rather either
but I'd really rather avoid this middle ground where we have a lot of ugly code that deprecates stuff for questionable gains. I'm also keen to change the behaviour to require users to specify So if we're actually happy to make a breaking change and not bump the major version of ADTypes.jl, on the basis that Tapir.jl support is "experimental", then I'm in favour of going ahead with that. |
Let's do it then. |
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Also deprecates a default choice in favour of requiring the user to specific whether they want
debug_mode
on / off.The implementation is quite ugly, because we cannot use the usual
@deprecate
macro because Julia does not specialise on kwargs -- if you attempt to use the@deprecate
macro, you'll find that you wind up overwriting existing methods, precompilation fails, and everything breaks. The only way I've been able to find to make this work is to create the new method with two kwargs added in this PR, in which each case is handled explicitly.Unfortunately we cannot rename the
safe_mode
field of theAutoTapir
type todebug_mode
-- that will have to wait until 2.0. As a result I'm really not 100% convinced by this PR, but I'm going to let @gdalle and @yebai debate its merits.