JuliaGraphs / GraphIO.jl

Graph IO functionality for various formats.
Other
61 stars 27 forks source link

Formats outside @require + Package extensions #51

Closed filchristou closed 1 year ago

filchristou commented 1 year ago

Subtypes of AbstractGraphFormat are not available if the appropriate dependencies are not loaded. I think this is a bad design and we should separate

This leads to problem if a 3rd-party decides to extend the functionality of AbstractGraphFormat subtypes. (see https://discourse.julialang.org/t/requires-jl-and-precompilation/53155) Namely, the new package cannot use the defined types, even if the appropriate dependencies are loaded.

After some small discussion here, I made a an extension package NestedGraphsIO.jl that handles IO for MetaGraphs.jl and NestedGraphs.jl. However, I need the changes of this PR in order to access the GraphMLFormat type defined in GraphIO.jl

The changes here are nothing more that just separating the @require from the subtypes definition and the functionality, which I insert in a new file.

filchristou commented 1 year ago

Hello. GraphIO.jl has long been waiting for the package extension feature that was shipped with julia v1.9. I think this could revive the project. Could someone please review my code ?

It is supposed to be backwards compatible. I checked locally with v1.9.0 and v1.8.5. However it would be nice to have that tested automatically with a GHAction. If someone could provide a short tip how to do this I can repush.

I intend to bring more code here from collapsing NestedGraphsIO.jl that supports both NestedGraphs and MetaGraphs (hopefully MetaGraphsNext at some point too).

Remarks

  1. Because package extensions cannot at the moment export types, I define all the types irrespectively of the dependencies. This is what the first commit is about.
  2. I deleted the deprecated.jl file, because... well it's time to move on. that's the only reason why this is a breaking change, i.e. v0.7
  3. It would be nice if we could offer some sort of backwards features as described here. E.g. say the user needs to read an graphml type. He shouldn't need to know that EzXML is a dependency. I think this could be possible with a macro.
  4. Speaking of EzXML.jl, it would be nice to migrate to XML.jl at some point.
simonschoelly commented 1 year ago

I have not had time to get up to date with Julia v1.9, so this is a bit of a learning experience for me.

I totally see the point of exporting these symbols even if the necessary extensions are not installed. One thing I wonder now, if there would be an easy way to warn users of this package, if they try to use a symbol where the necessary packages are not installed. Do you have any idea perhaps? Otherwise we might need to update the README so that this becomes at least clear there, we already had quite a few confused users with the current design.

Also, I think we should adjust the CI/CD tests, so that the test are run for Julia v1.6 (the minimal version we try to support for Graphs.jl), for Julia v1.9 and for the nightly release.

filchristou commented 1 year ago

One thing I wonder now, if there would be an easy way to warn users of this package, if they try to use a symbol where the necessary packages are not installed. Do you have any idea perhaps?

I don't have an obvious solution. I updated the README to reflect what is expected. The discourse post I shared before is relevant.

...adjust the CI/CD tests, so that the test are run for Julia v1.6 (the minimal version we try to support for Graphs.jl), for Julia v1.9 and for the nightly release.

I updated the ci.yaml

filchristou commented 1 year ago

Do you have any idea perhaps?

Some ideas I have but these could be integrated in a later PR in my opinion:

Define a fallback function loadgraph(io::IO, agf::AbstractGraphFormat) where we check the runtime if the necessary dependencies are present for the specific AbstractGraphFormat. If not, we return a helpful message.

Theoretically, we could use some metaprogramming as syntactic sugar to automatically load dependencies. E.g. @readgraphml would translate to using EzXML

gdalle commented 1 year ago

I agree with the proposal, and I trust @filchristou for the copy-paste ^^ The Requires.jl business in the main file GraphIO.jl seems coherent, although I haven't checked which dependencies are needed where since I'm new to the package.

Since this is a big change I'm just gonna add some Aqua testing and formatting to make sure we didn't miss a typo. Then I'll merge