Skip to content
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

Closed
wants to merge 6 commits into from

Conversation

willtebbutt
Copy link
Contributor

@willtebbutt willtebbutt commented Aug 2, 2024

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

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 the AutoTapir type to debug_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.

@yebai
Copy link

yebai commented Aug 2, 2024

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.

@gdalle
Copy link
Collaborator

gdalle commented Aug 2, 2024

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.

@gdalle gdalle closed this Aug 2, 2024
@yebai
Copy link

yebai commented Aug 2, 2024

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.

@gdalle
Copy link
Collaborator

gdalle commented Aug 2, 2024

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.
In this particular instance, renaming a keyword argument because you named it wrong the first time is not important enough to warrant a breaking release. Therefore it's up to you to do the translation inside Tapir (or DifferentiationInterface's Tapir extension, whichever you prefer).

@gdalle
Copy link
Collaborator

gdalle commented Aug 2, 2024

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.

@ChrisRackauckas
Copy link
Member

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.

@gdalle
Copy link
Collaborator

gdalle commented Aug 2, 2024

Yeah that's what this PR does. But the benefit of making the constructor so dirty for a 4-character renaming safe -> debug was not obvious to me

@ChrisRackauckas
Copy link
Member

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.

@willtebbutt
Copy link
Contributor Author

willtebbutt commented Aug 2, 2024

Personally, I would rather either

  1. do nothing, or
  2. make a breaking change to the Tapir implementation.

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 debug_mode, rather than giving it a default value, which we can't do without a breaking change.

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.

@ChrisRackauckas
Copy link
Member

Let's do it then.

@willtebbutt willtebbutt mentioned this pull request Aug 2, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants