Modify Tapir.jl Implementation #76
Conversation
|
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 JuliaLang/julia#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: JuliaRegistries/General#103582 (comment) 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: 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. |
|
|
||
| Defined by [ADTypes.jl](https://github.com/SciML/ADTypes.jl). | ||
|
|
||
| # Constructors | ||
|
|
||
| AutoTapir(; safe_mode=true) | ||
| AutoTapir(; debug_mode::Bool) |
There was a problem hiding this comment.
@willtebbutt Does this allow constructors like AutoTapir(false)? It feels a bit inconvenient if we force everyone to type AutoTapir(debug_mode=false) all the time, particularly for REPL.
There was a problem hiding this comment.
All other ADTypes constructors expect keyword arguments, the positional arguments are not documented and thus not part of the API. AutoTapir should not be different in this regard
|
@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? |
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 |
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
This does two things:
safe_modefield of AutoTapir todebug_mode,Follow up from #75 . Per discussion with @ChrisRackauckas , this is breaking.
@yebai @gdalle are you both okay with this?