fixes for Term extension #185
Conversation
| end | ||
|
|
||
| function Term.Panel(b::XGBoost.Booster) | ||
| Base.show(io::IO, m::MIME"text/plain", dm::XGBoost.DMatrix) = show(io, m, Term.Panel(dm)) |
There was a problem hiding this comment.
This is type piracy and AFAICT should not be done in an extension. A standard workflow would be to tell users to call Term.Panel if they want the Term-style output.
There was a problem hiding this comment.
Why would this not be allowed in an extension? It's together in the repo with the package and doesn't even have separate versioning. It seems like the whole point of an extension.
There was a problem hiding this comment.
No, it's not the point of an extension. The purpose of an extension is to extend functions where the function and/or its arguments are owned by both the package and its weak dependency. Neither the function nor its arguments are owned by the weak dependency, so this is just type piracy.
There was a problem hiding this comment.
Why is it worse to extend Base with a method from XGBoost than a method from Term? Can you give an example of why this is a problem?
There was a problem hiding this comment.
It's fine to define a function Base.show(::IO, ::MIME"text/plain", ::XGBoost.DMatrix) - but it belongs in the main package since it (or rather its generic fallback) can be called even without loading Term.
As an intuitive example, maybe one could imagine two packages for pretty printing Julia objects which you might want to add as weak dependencies - since both packages neither own the extended function (Base.show) nor one of its argument types (IO, MIME"text/plain", and XGBoost.DMatrix) two definitions of this show function will clash and depending on the order in which the extensions are loaded one or the other implementation will be used.
There was a problem hiding this comment.
It remains ambiguous which of our arguments this supports, but from the Pkg docs:
If one considers PlottingContourExt as a completely separate package, it could be argued that defining Plotting.plot(c::Contour.ContourCollection) is type piracy since PlottingContourExt owns neither the method Plotting.plot nor the type Contour.ContourCollection. However, for extensions, it is ok to assume that the extension owns the methods in its parent package. In fact, this form of type piracy is one of the most standard use cases for extensions.
This statement is confusing, since the methods of the parent package clearly belong to the parent package (module specifically) regardless, but I take it to mean that the extension is free to define methods that would be defined in the parent were the dependency direct. I could see an argument as to why this might be a problem if Julia were the sort of language where packages were being dynamically loaded and unloaded but that's not the case. Furthermore, I could easily imagine a situation in which this cripples extension functionality. Suppose, for example, that some kind of matrix multiplication is more efficient in the presence of two packages, this policy would prevent you from implementing the efficient multiplication, mandating the dependency is direct even if it's a heavy dependency for one method. Admittedly the stakes are much lower here, but I don't yet see a compelling argument why this should be forbidden.
There was a problem hiding this comment.
You can do whatever is technically possible. But this show definition is a type piracy. You load another package and modify functions that are neither owned by it nor depend on it.
Suppose, for example, that some kind of matrix multiplication is more efficient in the presence of two packages, this policy would prevent you from implementing the efficient multiplication, mandating the dependency is direct even if it's a heavy dependency for one method.
I'm not sure what you had in mind. In an extension of a matrix package A with a weak dependency of matrix package B you can of course extend Base.:*(::MatrixA, ::MatrixB) without committing type piracy - one argument is owned by the package and the other one by the weak dependency.
There was a problem hiding this comment.
You could have a matrix multiplication that normally falls back to the methods in Base but in presence of the extension gets a new specialized method, for example a Base.*(::AbstractMatrix, ::ParentPackageMatrix). This seems among the most basic possible applications of extensions.
I'm slightly sympathetic to the idea that the way this is implemented makes code loading unpredictable, but as I see this as a useful functionality of extensions I certainly don't feel persuaded by your simply insisting on your definition of ownership. As I said, I might find a good footgun example more compelling.
Note also that some (commonly used) packages such as SixelTerm.jl and KittyTerminalImages.jl change the display behavior of all sorts of objects from different packages, by comparison what is done here seems extremely conservative.
Regardless, please open an issue if you feel strongly this should be changed, if there is some consensus we can just change it. I very much dislike your argument in the general case (again, because it clearly forbids useful behavior, not because I can't be persuaded), but I don't care all that much about these particular show methods.
There was a problem hiding this comment.
Something I am far more sympathetic to here, than in the general case, is that in this example we completely change the output of a function based on which packages are loaded. It's not really a rigorous argument since you'd expect this to happen all the time because of floating point or whatever, but I can relate to the idea that it seems strange to have function output change completely based on which packages are loaded. Again, in this case it really doesn't bother me personally at all, since Julia packages change display behaviors all the time, but I'm a lot less opposed to your point of view in this specific case than your definition of ownership.
There was a problem hiding this comment.
Yeah, actually I think I'm persuaded in this particular case, though I still don't accept your definition of ownership. What I do accept is that completely changing function output based on the hidden global state of loaded code is (at the very least) highly undesirable. If this were a function other than a show(::IO, ::MIME, ::Thing) which I took more seriously I would take it for granted that the way the output changes here is clearly unacceptable.
However, the fact that shenanigans for changing the display behavior is already commonplace in the Julia ecosystem gives me pause. Please open an issue. If there are not serious objections to it let's just change this. Fair?
Somehow I had previously missed the fact that the Term extension was no longer even creating
showmethods. With this fix, Term panels will show for the"text/plain"mime type when Term is loaded.Additionally, there is a small fix for CUDA-backed tables in an issue that's caught in a test but not in CI/CD because of lack of CUDA.