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

Formats outside @require + Package extensions #51

Merged
merged 6 commits into from
Jun 23, 2023

Conversation

filchristou
Copy link
Contributor

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

  • type definition (which doesn't need the dependencies) and
  • functionality (which uses the dependencies)

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 filchristou changed the title Formats outside @require Formats outside @require + Package extensions Jun 13, 2023
@filchristou
Copy link
Contributor Author

filchristou commented Jun 13, 2023

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

gdalle commented Jun 23, 2023

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

@gdalle gdalle merged commit 3211943 into JuliaGraphs:master Jun 23, 2023
@filchristou filchristou deleted the typedefreqmov branch June 23, 2023 11:43
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.

3 participants