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

Stop encouraging type-piracy; suggest "dynamic" vendoring #56

Closed
wants to merge 1 commit into from

Conversation

tkf
Copy link

@tkf tkf commented Sep 14, 2019

I find it a bit uneasy that IntervalSets.jl encourages type-piracy. I think it might be preventing people from using it as a critical dependency. Instead, I suggest pointing people to use what I call "dynamic" vendoring (npm style import?). The idea is to use

include(Base.locate_package(Base.PkgId(Base.UUID("8197267c-284f-5f27-9208-e0e47529a953"), "IntervalSets")))
using .IntervalSets  # notice the leading dot

instead of using IntervalSets so that the package "owns" the types and functions defined in the submodule .IntervalSets.

See more explanation in the README I edited. You can read the rendered version here: https://github.com/tkf/IntervalSets.jl/tree/vendoring#including-intervalsetsjl-as-a-submodule

@ararslan ararslan requested a review from timholy September 15, 2019 01:11
@timholy
Copy link
Member

timholy commented Sep 15, 2019

You're just trying to annoy me wrt Revise, right? 😄

More seriously, this is probably a good thing to add. I've done something similar with https://github.com/JuliaArrays/CustomUnitRanges.jl. It's a bit problematic wrt two packages that need to both manipulate intervals, so I am not sure I'd phrase this quite as strongly as you have. At least to me, there are cases where the using and piracy mode make more sense.

IMO, piracy is not that different from starting Julia with different --projects (loading different versions of the same package can give you different behaviors), and thus not as big a deal as some people make it out to be.

@tkf
Copy link
Author

tkf commented Sep 15, 2019

You're just trying to annoy me wrt Revise, right?

Oops, that was not my intention. But I thought it'd work with Revise because Revise can handle include?

It's a bit problematic wrt two packages that need to both manipulate intervals

Yes, if we were to add this section in the README, I think this point should be emphasized. I included one sentence but it can be explained in more details.

so I am not sure I'd phrase this quite as strongly as you have

I'm OK with phrasing it differently. Maybe a simple fix would be to change the sentence to "... to avoid unnecessary type-piracy." (add "unnecessary").

At least to me, there are cases where the using and piracy mode make more sense.

I think cases like OffsetArrays make sense but users need to trust that package authors have good taste. It also would be problematic if two packages pirated IntervalSets.jl in an incompatible way.

This is why I prefer IntervalSets.jl to dis/encourage type-piracy as the same level as Base.

IMO, piracy is not that different from starting Julia with different --projects

Off topic, but I think it'd be nice if Pkg.jl helps doing type-piracy in more controlled manner. Some metadata fields like extending (specifies pirated packages) and conflicting-with would be nice.

@timholy
Copy link
Member

timholy commented Sep 15, 2019

You're just trying to annoy me wrt Revise, right?

Oops, that was not my intention. But I thought it'd work with Revise because Revise can handle include?

That was mostly a joke. But I am not quite certain what happens if the exact same file gets included by more than one package. Revise's internal structure may make the (unfortunate) assumption that one file gets included by one package. If it's a problem it's surely fixable, perhaps with some effort.

Off topic, but I think it'd be nice if Pkg.jl helps doing type-piracy in more controlled manner. Some metadata fields like extending (specifies pirated packages) and conflicting-with would be nice.

I had the same thought. There might be ways of getting the same behavior as conflicting-with now (both packages deliberately exporting the same function), and it's worth asking exactly what purpose these serve and how they get integrated into the ordinary user experience (warnings? errors? just documentation? how do people know to go look?), but Pkg does seem to be the right place to declare these things and worth mulling over what the behavior would actually be.

In the meantime, I'd be perfectly happy merging a version of this that is a bit more nuanced in terms of its recommendations. And as necessary I will fix Revise 😄.

@dlfivefifty
Copy link
Member

It would be good to understand the concrete examples where type piracy is causing issues. As of now it feels like solving a hypothetical problem with a solution that’s likely to cause non-hypothetical problems...

@tkf
Copy link
Author

tkf commented Sep 15, 2019

@dlfivefifty A (somewhat?) concrete problem would be defining broadcasted(::typeof(+), ::AbstractInterval, ::AbstractInterval) in two different packages. One package may use to rounding towards "outside" and another towards "inside" due to their specific usage. Or similarly there can be packages defining broadcasted(::typeof(sign), ::AbstractInterval) (sign is just an example of a discontinuous function) and returning their own Domain subtypes. If there is another package loading the both packages, it may break at least one of them. Is it concrete enough?

But I think potential problems are enough to avoid encouraging type-piracy. I had to think very carefully if I wanted to use IntervalSets.jl in my code because of that sentence. A "Base" library like IntervalSets.jl becomes powerful if it is used in many packages. So, I think it's better to get rid of the psychological blocker even if it is due to potential problems, to help people depending on this package.

Edit: non-monotonic -> discontinuous

@tkf
Copy link
Author

tkf commented Sep 15, 2019

Pkg does seem to be the right place to declare these things and worth mulling over what the behavior would actually be.

@timholy I opened an issue and replied to your comment in JuliaLang/Pkg.jl#1390

Revise's internal structure may make the (unfortunate) assumption that one file gets included by one package.

I see. It make sense that it can happen.

And as necessary I will fix Revise .

Thank you very much in advance!

@tkf
Copy link
Author

tkf commented Sep 16, 2019

WRT to the original question of type piracy (@tkf), let me point out: having another package that "pirates" this one to add arithmetic operations would be a lot like ColorVectorSpace pirating the types defined in ColorTypes and adding arithmetic. I can't remember a single problem that's ever caused.

@timholy Is this comment #55 (comment) rather talking about this issue?

Assuming that's the case, what do you think about my comment above #56 (comment)? I can probably come up with a few more examples with similar quality. But, if you consider an example is valid only if it already occurred, it's rather pointless to list them all.

(Note to myself: another example might be scalar + and broadcasted + implemented in different packages and an auto differentiation package assuming that both definitions to be compatible.)

@timholy
Copy link
Member

timholy commented Sep 17, 2019

In the case of conflicting definitions, won't you get a warning when you load the second package? But I guess you're saying you want the functionality of both packages within their own domain. Yes, I suppose that's reasonable. (I don't know why you'd want to round inside, but I'm sure there must be circumstances...)

@hyrodium
Copy link
Collaborator

hyrodium commented Jul 30, 2023

This PR is stale and #149 updates the README, so I'll close this PR.

@hyrodium hyrodium closed this Jul 30, 2023
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