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

Split out petgraph into separate crates #561

Merged
merged 198 commits into from
Sep 3, 2023
Merged

Split out petgraph into separate crates #561

merged 198 commits into from
Sep 3, 2023

Conversation

indietyp
Copy link
Member

@indietyp indietyp commented May 15, 2023

  • visit deprecated, available in petgraph-core
  • data deprecated, available in petgraph-core
  • move IndexType to petgraph-core
  • move EdgeType, and related to petgraph-core
  • rework IndexType to make use of funty

@indietyp indietyp changed the title Split out petgraph into seperate crates Split out petgraph into separate crates May 15, 2023
@indietyp
Copy link
Member Author

indietyp commented May 15, 2023

Progress Report

  • petgraph-core: ✅
  • petgraph-graph: ✅
  • petgraph-graphmap: ✅
  • petgraph-adjacency-matrix: ✅
  • petgraph-csr: ❌
  • petgraph-algorithms: ❌
  • petgraph-matrix-graph: ❌
  • petgraph-dot: ❌

Everything compiles with the default feature sets, and all tests pass.

  • List has been renamed to AdjacencyMatrix, to better reflect the purpose
    quickcheck + serde: untested
  • tests haven't been ported to their respective crates just yet
  • cargo-hack: not run yet, needs quickcheck fix for negative cycle first
  • everything has been converted to no-std, where possible, no-alloc.
  • Setting everything to deprecated as default will un-deprecate one-by-one after Tracking Issue: Trait rework #552 is resolved.
  • List and Csr no longer use Ix as NodeIndex, but (just like graph) use NodeIndex, because this lead to (imo) a violation of the "separation of concerns" principle as well as introduced other problems, as we cannot implement NodeRef or anything like that in the remote crates on Ix, we could implement a default impl in the core crate, but IMO that would be the wrong move (as we would once again muddy the waters of what is a node index, what is an edge index, etc)

@indietyp
Copy link
Member Author

Progress Report

  • petgraph-core: ✅
  • petgraph-graph: ✅
  • petgraph-graphmap: ✅
  • petgraph-adjacency-matrix: ✅
  • petgraph-csr: ✅
  • petgraph-algorithms: ⌛
  • petgraph-matrix-graph: ✅
  • petgraph-dot: ❌

The algorithms crate is by far the most challenging one; some algorithms require specific graphs (like Graph or AdjacencyMatrix). Most likely, these imports can be removed, but this will take a bit more time. While rummaging around the insides, I took the liberty of moving traversal from visit to petgraph-algorithms; while doing so, I plan to rework them to modernize them a bit. (The old will stay in core like they are and are deprecated)

@indietyp
Copy link
Member Author

Progress Report

  • petgraph-core: ✅
  • petgraph-graph: ✅
  • petgraph-graphmap: ✅
  • petgraph-adjacency-matrix: ✅
  • petgraph-csr: ✅
  • petgraph-algorithms: ✅
  • petgraph-matrix-graph: ✅
  • petgraph-dot: ❌
  • All crates have been successfully ported, serde has been reworked.
  • dot still lives in petgraph and is now backed by the dot crate, I am currently contemplating replacing it in favor of graphviz-rust, as it allows serialization and deserialization, in the future we might want to move it into petgraph-io
    (or something similar) and also implement other formats, like GEXF.
  • sadly the dot feature now requires std as it uses std::io::Write :/
  • The only thing now remaining are some compile errors I left behind, quickcheck, testing.
    • for proptest I was thinking to create a companion petgraph-proptest, which implements multiple strategies. Nothing short-term.
  • I have decided against reworking the currently non-generic algorithms, as it would be redundant first converting them and then reworking the traits, they are currently gated behind a temporary feature gat.

@indietyp
Copy link
Member Author

indietyp commented May 19, 2023

Progress Report

  • New crates: petgraph-generators, petgraph-proptest

While trying to upgrade to proptest I noticed that our quicktest is really feasible in quicktest 1.0 and likely also doesn't correctly shrink. I have taken the liberty to transfer to proptest and completely reworked the shrinking and generation.

The code is now generic and can easily be implemented for all kinds of graphs as well is configurable, so if users do not like the default, they can just call default::graph_strategy from the implementation with their arguments and graph type. With this change, we're likely to also support proptest on all existing types!

Crates compile (again). Due to the very nature of what we're trying to do (separating the concerns, making things more maintainable, etc.), there are quite a handful of new crates; they are now grouped under crates/ to keep things clean (commit message of 795a994 mentions inspirations)

The only thing missing are now moving the tests (and testing^^)

@indietyp
Copy link
Member Author

Progress Report

quite a lot of tests have been ported; remaining are:

  • graph (directed)
  • algorithms in directed
  • list
  • quickcheck (to proptest)
  • stable-graph
  • graphmap

@indietyp
Copy link
Member Author

Phew, well, that took a bit longer than expected. Finally, all tests have been converted and rewritten. This whole thing not only separated everything into separate crates but also had the nice side-effect that tests are now more readable and in places where one expects them.

The only thing that needs to be added now is going through with cargo-hack and move the benches.

Base automatically changed from bm/misc to next June 20, 2023 09:44
@indietyp
Copy link
Member Author

Sadly things came in between me and being able to work on this, but after some major rework of all the benchmarks (which if I do say so, are a lot easier to read now), we're ready!

@indietyp indietyp marked this pull request as ready for review August 28, 2023 13:02
@indietyp indietyp requested review from ABorgna and bluss August 28, 2023 14:35
@indietyp
Copy link
Member Author

indietyp commented Aug 28, 2023

This is quite a large PR 😅 (but it's mostly just moving stuff around). I'll wait for any comments until the end of the week (or until I get approval) and then merge them into the next branch.

@indietyp indietyp merged commit 0f9728f into next Sep 3, 2023
5 checks passed
@indietyp indietyp deleted the bm/split branch September 3, 2023 14:08
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.

None yet

1 participant