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

add InterfacesCore subpackage #39

Closed
wants to merge 6 commits into from
Closed

add InterfacesCore subpackage #39

wants to merge 6 commits into from

Conversation

rafaqz
Copy link
Owner

@rafaqz rafaqz commented Feb 1, 2024

This PR adds an InterfacesCore.jl subpackage.

All it implements is abstract type Interface end and a @interface_type macro.

The reason for this is so we can define the interface type without any package overheads, and add Interfaces.jl as an extension. This means packages can just go using Interfaces and test their implementation, rather than needing to load another package.

We cant use extensions currently because the type would be defined in the extension, and not be accessible to anyone.

@rafaqz rafaqz requested a review from gdalle February 1, 2024 16:43
@gdalle
Copy link
Collaborator

gdalle commented Feb 2, 2024

Not sure I understand the rationale here, can you elaborate?

@rafaqz
Copy link
Owner Author

rafaqz commented Feb 2, 2024

Ok. So I want to add Interfaces.jl to a package, without adding a full dependency on it or adding any overheads.

Theyre not so bad, but I imagine people will want a no cost option initially. And if all the interface tests get really long it will have some small overhead.

So the options to do that are:

  1. make a separate interfaces package
  2. use an extension

Its kind of annoying to have to do 1 everywhere. But 2 doesn't actually work currently, because if you use the @interface macro in an extension, the types it generates are hidden in the extension and not available in the package.

So we need to define the interface type in the main package, e.g. in DimensionalData.jl.

So:

  1. We move the Interface type that all interfaces depend on to InterfacesCore.jl, with a small macro for adding core interfaces.
  2. Depend on InterfacesCore.jl in DimensionalData.jl as a regular dependency (its tiny).
  3. Use the slimmed down @interface_core macro to define our interface type DimArrayInterface without any tests inside DimensionalData.jl scope
  4. We define an extension DimensionalDataInterfacesExt.jl in DimensionalData.jl
  5. The extension loads when the full Interfaces.jl package is loaded
  6. We import the DimArrayInterface type into the extension from DimensionalData.jl
  7. We define the full interface with @interface ... in the extension using the existing interface type
  8. Packages that want to test the interface have to load Interfaces.jl to get the test function, so they will always have the full DimensionalData.jl interfaces available.

I guess this should be the package readme

@gdalle
Copy link
Collaborator

gdalle commented Feb 2, 2024

Is an 8-step procedure really worth the trouble of what is probably a very small package overhead? If Interfaces.jl is to be widely adopted it must be very simple, and that split makes it so much more complicated.

@gdalle
Copy link
Collaborator

gdalle commented Feb 2, 2024

Can we quantify the overhead somehow?

@rafaqz
Copy link
Owner Author

rafaqz commented Feb 2, 2024

Yeah, maybe not worth it its very little overhead. This is Interfaces.jl + DimensionalDataInterfacesExt.jl

julia> using DimensionalData

julia> @time using Interfaces
  0.004137 seconds (4.39 k allocations: 334.969 KiB)

But didn't you put the Graphs.jl interface in a separate package? That's the problem I'm trying to solve.

@gdalle
Copy link
Collaborator

gdalle commented Feb 2, 2024

But didn't you put the Graphs.jl interface in a separate package? That's the problem I'm trying to solve.

Two reasons for that

  • to have my hands free in order to experiment
  • because my interface checks also cover SimpleWeightedGraphs and the like, which are not dependencies of Graphs.jl

But I didn't have to put the checks in the same place as the definition. Ideally the definition should go in Graphs.jl and the checks in each implementing package, as God intended

@rafaqz
Copy link
Owner Author

rafaqz commented Feb 2, 2024

It just feels a bit weird putting the interface tests in runtime code with the rest of the package, and I'm guessing some people wont like that. But maybe its not an issue

@gdalle
Copy link
Collaborator

gdalle commented Feb 2, 2024

I think we'll cross that bridge if we get to it. What I like about your approach is that the design is dead simple, let's not overcomplicate it to gain 4 ms.

If you really care about moving the tests out of runtime code, maybe there could be an extension of Interfaces.jl that depends on Test being loaded?

@rafaqz
Copy link
Owner Author

rafaqz commented Feb 2, 2024

Yes could be interesting to use Test as the weak dep. Have to think about how that works.

For now I'll move the DimensionalData.jl interface to the package.

@rafaqz
Copy link
Owner Author

rafaqz commented Feb 2, 2024

And your right keeping it dead simple might be the most important thing here.

Thanks for the feedback

@rafaqz rafaqz closed this Feb 2, 2024
@rafaqz rafaqz deleted the interfaces_core branch March 2, 2024 12:44
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.

2 participants