ADTypes.jl
ADTypes.jl copied to clipboard
Modify Tapir.jl Implementation
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:
- rename
safe_modefield of AutoTapir todebug_mode, - 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?
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.
Isn’t this a breaking change tho?
Yup -- see discussion here.
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
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.
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.
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).
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 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.
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.
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.
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?
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
Superseded by #89