ADTypes.jl icon indicating copy to clipboard operation
ADTypes.jl copied to clipboard

Modify Tapir.jl Implementation

Open willtebbutt opened this issue 1 year ago • 13 comments

Checklist

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

Additional context

This does two things:

  1. rename safe_mode field of AutoTapir to debug_mode,
  2. remove default value, in order to force users to make a choice.

Follow up from #75 . Per discussion with @ChrisRackauckas , this is breaking.

@yebai @gdalle are you both okay with this?

willtebbutt avatar Aug 02 '24 13:08 willtebbutt

Great stuff. I've spoken with @yebai and he's also on board with this, so I'm happy to press forwards when the above versions have been yanked.

willtebbutt avatar Aug 02 '24 15:08 willtebbutt

Isn’t this a breaking change tho?

wsmoses avatar Aug 02 '24 15:08 wsmoses

Yup -- see discussion here.

willtebbutt avatar Aug 02 '24 16:08 willtebbutt

If we’re going to make a breaking change here we should just rename Tapir while we’re at it. The name tapir is and was used prior to this package, targeting the Julia compiler itself, like earlier seen in https://github.com/JuliaLang/julia/pull/39773 . For more details see either my or @vchuravy’s thesis.

Since there’s going to be major breakage and renaming here anyways when Tapir is merged into Julia, I think one solution is to just deprecate AutoTapir entirely here (which would itself be non breaking), and create a new struct AutoTaped or whatever with the new name. Notably this is not breaking for anybody and the old name/convention can easily wait to 2.0

That way, even if you have to wait to rename your package, being forward looking will reduce the headache for the rest of the Julia ecosystem.

[1] Note that changing the Tapir parallelism name is a nonstarter. It predated the AD tool and has more widespread use and recognition. It would be akin to having a CUDA.jl package which has nothing to do with GPUs

[2] Unfortunately in spite of seeing the conflict in advance and linking to relevant repos like: https://github.com/JuliaRegistries/General/pull/103582#issuecomment-2023954705 neither @vchuravy nor I were mentioned on the weekend it was released — otherwise we would’ve stopped the name conflict earlier

wsmoses avatar Aug 02 '24 16:08 wsmoses

It is a very fraught name choice and I was surprised when I saw it. It would be the right time to consider any change.

ChrisRackauckas avatar Aug 03 '24 01:08 ChrisRackauckas

I was pointed towards the following projects recently:

  • https://github.com/softwaremill/tapir
  • https://deepmind-tapir.github.io/

We are not saying the name of Tapir can’t be changed, but (1) it is hard and disrupting (2) Tapir is not a term or acronym like CUDA. I hope the name change request discussion can be moved elsewhere and the change unblocked here, which is orthogonal.

yebai avatar Aug 03 '24 10:08 yebai

Actually I agree this might be a good opportunity to discuss renaming, even though there's no point in blaming the original discussion that happened upon registration (it was hard for normal folks to know which other projects are relevant or not to Julia, especially when they weren't registered).

gdalle avatar Aug 03 '24 10:08 gdalle

It is generally a bad practice to block PRs with non-related concerns. I suggest that @wsmoses discuss Tapir's naming issue with @willtebbutt and myself in a separate thread; changes here are orthogonal and can be merged first.

yebai avatar Aug 05 '24 14:08 yebai

@yebai in this case I think these are very related.

Specifically in its current form the proposed change in this PR is a breaking one.

My alternative suggestion is that if you do the name change in ADTypes now it is no longer breaking.

Specifically you can keep AutoTapir in ADTypes with the old kwarg but mark it deprecated. You can also create a new AutoTaped/AutoTaper/etc with the new kwarg.

No code will be broken by this. Existing user code using AutoTapir with the old kwarg will continue to work. Your existing package will also continue to work and need not be renamed immediately.

However anyone who was previously using your package will now have the option to swap to a new, in your opinion, more clear kwarg, without breakage. Moreover, it also ensures that things don’t break in the future for the ecosystem when things need to be renamed due to Julia having a base Tapir package, among other issues.

wsmoses avatar Aug 05 '24 14:08 wsmoses

I lean towards deferring changing the constructor name to a later point when a new name for the package is chosen. I don't have strong views on this so happy to discuss if @wsmoses is keen on it.

More generally, I am not sure it is a good practice to recycle a registered package name for a new base package. But that is for the Julia general registry maintainers to evaluate.

yebai avatar Aug 05 '24 15:08 yebai

I agree with Billy that if there is going to be a Tapir name change, it should be done at the same time as the safe mode switch to avoid a breaking release.

gdalle avatar Aug 05 '24 15:08 gdalle

Converting this PR as a draft while you hash it out the naming issue. Essentially the registration was validated by people who saw Tapir-LLVM but were not able to judge its significance to Julia or whether it would ever become a commonplace name. Maybe @wsmoses can try to explain in layman's terms why that is the case?

gdalle avatar Aug 05 '24 16:08 gdalle

I don't have strong views on this so happy to discuss if @wsmoses is keen on it.

Since it sounds from above that folks presently are not opposed to changing (and we've already discussed offline [i.e. slack] the difficulties of Tapir as a name for non-parallel code within Julia), I think at this point it's just a question of what new name you all (@willtebbutt + @yebai) would like to use here.

edit: If my interpretation is mistaken, happy to write things out here too and we can discuss that as well

wsmoses avatar Aug 05 '24 16:08 wsmoses

Superseded by #89

gdalle avatar Sep 25 '24 10:09 gdalle