feat!: Use RPITIT for graph traits #156

aborgna-q · 2024-11-14T09:04:11Z

Closes #119

This should simplify the implementation of #155, and any other new wrapper.

BREAKING CHANGE: Graph trait methods now return impl Iterators instead of named associated types.

hugrbot · 2024-11-14T09:05:59Z

This PR contains breaking changes to the public Rust API.

cargo-semver-checks summary


--- failure auto_trait_impl_removed: auto trait no longer implemented ---

Description:
A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented.
      ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/auto_trait_impl_removed.ron

Failed in:
type NodeEdgeRefs is no longer Send, in /home/runner/work/portgraph/portgraph/PR_BRANCH/src/view/petgraph.rs:396
type NodeEdgeRefs is no longer Sync, in /home/runner/work/portgraph/portgraph/PR_BRANCH/src/view/petgraph.rs:396
type NodeEdgeRefs is no longer UnwindSafe, in /home/runner/work/portgraph/portgraph/PR_BRANCH/src/view/petgraph.rs:396
type NodeEdgeRefs is no longer RefUnwindSafe, in /home/runner/work/portgraph/portgraph/PR_BRANCH/src/view/petgraph.rs:396
type EdgeRefs is no longer Send, in /home/runner/work/portgraph/portgraph/PR_BRANCH/src/view/petgraph.rs:326
type EdgeRefs is no longer Sync, in /home/runner/work/portgraph/portgraph/PR_BRANCH/src/view/petgraph.rs:326
type EdgeRefs is no longer UnwindSafe, in /home/runner/work/portgraph/portgraph/PR_BRANCH/src/view/petgraph.rs:326
type EdgeRefs is no longer RefUnwindSafe, in /home/runner/work/portgraph/portgraph/PR_BRANCH/src/view/petgraph.rs:326

--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/struct_missing.ron

Failed in:
struct portgraph::view::filter::FilteredGraphCtx, previously in file /home/runner/work/portgraph/portgraph/BASELINE_BRANCH/src/view/filter.rs:131

--- failure trait_removed_associated_type: trait's associated type was removed ---

Description:
A public trait's associated type was removed or renamed.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/trait_removed_associated_type.ron

Failed in:
associated type PortView::Nodes, previously at /home/runner/work/portgraph/portgraph/BASELINE_BRANCH/src/view.rs:22
associated type PortView::Ports, previously at /home/runner/work/portgraph/portgraph/BASELINE_BRANCH/src/view.rs:27
associated type PortView::NodePorts, previously at /home/runner/work/portgraph/portgraph/BASELINE_BRANCH/src/view.rs:32
associated type PortView::NodePortOffsets, previously at /home/runner/work/portgraph/portgraph/BASELINE_BRANCH/src/view.rs:37
associated type PortView::Nodes, previously at /home/runner/work/portgraph/portgraph/BASELINE_BRANCH/src/view.rs:22
associated type PortView::Ports, previously at /home/runner/work/portgraph/portgraph/BASELINE_BRANCH/src/view.rs:27
associated type PortView::NodePorts, previously at /home/runner/work/portgraph/portgraph/BASELINE_BRANCH/src/view.rs:32
associated type PortView::NodePortOffsets, previously at /home/runner/work/portgraph/portgraph/BASELINE_BRANCH/src/view.rs:37
associated type LinkView::Neighbours, previously at /home/runner/work/portgraph/portgraph/BASELINE_BRANCH/src/view.rs:291
associated type LinkView::NodeConnections, previously at /home/runner/work/portgraph/portgraph/BASELINE_BRANCH/src/view.rs:296
associated type LinkView::NodeLinks, previously at /home/runner/work/portgraph/portgraph/BASELINE_BRANCH/src/view.rs:302
associated type LinkView::PortLinks, previously at /home/runner/work/portgraph/portgraph/BASELINE_BRANCH/src/view.rs:308
associated type LinkView::Neighbours, previously at /home/runner/work/portgraph/portgraph/BASELINE_BRANCH/src/view.rs:291
associated type LinkView::NodeConnections, previously at /home/runner/work/portgraph/portgraph/BASELINE_BRANCH/src/view.rs:296
associated type LinkView::NodeLinks, previously at /home/runner/work/portgraph/portgraph/BASELINE_BRANCH/src/view.rs:302
associated type LinkView::PortLinks, previously at /home/runner/work/portgraph/portgraph/BASELINE_BRANCH/src/view.rs:308
associated type MultiView::NodeSubports, previously at /home/runner/work/portgraph/portgraph/BASELINE_BRANCH/src/view.rs:589
associated type MultiView::NodeSubports, previously at /home/runner/work/portgraph/portgraph/BASELINE_BRANCH/src/view.rs:589

codecov · 2024-11-14T09:06:42Z

Codecov Report

Attention: Patch coverage is 73.02905% with 65 lines in your changes missing coverage. Please review.

Project coverage is 80.49%. Comparing base (c91476b) to head (bd79b3b).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/view/filter.rs	32.14%	37 Missing and 1 partial ⚠️
src/multiportgraph.rs	64.28%	10 Missing ⚠️
src/view/refs.rs	46.66%	8 Missing ⚠️
src/portgraph/iter.rs	94.73%	4 Missing ⚠️
src/portgraph.rs	84.21%	3 Missing ⚠️
src/view.rs	87.50%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #156      +/-   ##
==========================================
- Coverage   80.63%   80.49%   -0.14%     
==========================================
  Files          22       22              
  Lines        5168     5158      -10     
  Branches     5168     5158      -10     
==========================================
- Hits         4167     4152      -15     
- Misses        928      933       +5     
  Partials       73       73

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

doug-q

Looks great, some nits and questions.

doug-q · 2024-11-14T10:29:56Z

benches/benchmarks/portgraph.rs

+        .nodes_iter()
+        .collect_vec()
+        .into_iter()
+        .cycle()


I think this is because nodes_iter() doesn't return an iterator with Clone. Kind of gross, but I guess this is fine. Did you consider nodes_iter() -> impl Iterator + Clone?

We should probably do that. Iterators are normally clonable, so it shouldn't be too problematic

doug-q · 2024-11-14T10:31:45Z

src/multiportgraph.rs

-    fn node_port_capacity(&self, node: NodeIndex) -> usize {
-        self.graph.node_port_capacity(node)
+    delegate! {
+        to (self.graph) {


should these have #[inline] annotations?

delegate does that automatically!

Inserts #[inline(always)] automatically (unless you specify #[inline] manually on the method)

https://docs.rs/delegate/latest/delegate/#features

doug-q · 2024-11-14T10:41:17Z

src/multiportgraph/iter.rs

+    #[inline]
+    /// Returns an iterator over every pair of matching ports connecting `from`
+    /// with `to`.
+    pub fn _get_connections(&self, from: NodeIndex, to: NodeIndex) -> NodeConnections {


Why are these functions pub? They are "Used internally".

I'm not a huge fan of these _... functions, but I don't have a better suggestion.

I didn't think about it since it was in a private module, but they should be pub(crate) instead 👍

I agree that these are annoying to have. In an ideal world we'd be able to use #[refine] instead...
https://rust-lang.github.io/rfcs/3245-refined-impls.html

doug-q · 2024-11-14T10:45:09Z

src/multiportgraph.rs

    #[inline]
-    fn get_connections(&self, from: NodeIndex, to: NodeIndex) -> Self::NodeConnections<'_> {
-        NodeConnections::new(self, to, self.output_links(from))
+    fn get_connections(


I think all of these functions could be

delegate! { to self { #[call(_get_connections)] fn get_connections(...); } }

If you prefer as is that is fine.

doug-q · 2024-11-14T10:52:09Z

src/view/petgraph.rs

@@ -95,39 +95,39 @@ macro_rules! impl_petgraph_traits {
        }

        impl<'g, $($args)*> petgraph::visit::IntoNodeIdentifiers for &'g $graph where $($where)* {
-            type NodeIdentifiers = <$graph as PortView>::Nodes<'g>;
+            type NodeIdentifiers = Box<dyn Iterator<Item = NodeIndex> + 'g>;


This is a shame.

Yes...
We need petgraph 0.7 with RPITIT in its traits -.-'

## 🤖 New release * `portgraph`: 0.12.3 -> 0.13.0 (⚠️ API breaking changes) ### ⚠️ `portgraph` breaking changes ``` --- failure auto_trait_impl_removed: auto trait no longer implemented --- Description: A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented. ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/auto_trait_impl_removed.ron Failed in: type EdgeRefs is no longer Send, in /tmp/.tmpncG4Dv/portgraph/src/view/petgraph.rs:326 type EdgeRefs is no longer Sync, in /tmp/.tmpncG4Dv/portgraph/src/view/petgraph.rs:326 type EdgeRefs is no longer UnwindSafe, in /tmp/.tmpncG4Dv/portgraph/src/view/petgraph.rs:326 type EdgeRefs is no longer RefUnwindSafe, in /tmp/.tmpncG4Dv/portgraph/src/view/petgraph.rs:326 type NodeEdgeRefs is no longer Send, in /tmp/.tmpncG4Dv/portgraph/src/view/petgraph.rs:396 type NodeEdgeRefs is no longer Sync, in /tmp/.tmpncG4Dv/portgraph/src/view/petgraph.rs:396 type NodeEdgeRefs is no longer UnwindSafe, in /tmp/.tmpncG4Dv/portgraph/src/view/petgraph.rs:396 type NodeEdgeRefs is no longer RefUnwindSafe, in /tmp/.tmpncG4Dv/portgraph/src/view/petgraph.rs:396 --- failure inherent_method_missing: pub method removed or renamed --- Description: A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/inherent_method_missing.ron Failed in: FilteredGraph::new_region, previously in file /tmp/.tmpJ9kcgB/portgraph/src/view/region.rs:26 FilteredGraph::new_flat_region, previously in file /tmp/.tmpJ9kcgB/portgraph/src/view/region.rs:105 FilteredGraph::new_subgraph, previously in file /tmp/.tmpJ9kcgB/portgraph/src/view/subgraph.rs:73 FilteredGraph::is_convex, previously in file /tmp/.tmpJ9kcgB/portgraph/src/view/subgraph.rs:100 FilteredGraph::is_convex_with_checker, previously in file /tmp/.tmpJ9kcgB/portgraph/src/view/subgraph.rs:106 FilteredGraph::new_region, previously in file /tmp/.tmpJ9kcgB/portgraph/src/view/region.rs:26 FilteredGraph::new_flat_region, previously in file /tmp/.tmpJ9kcgB/portgraph/src/view/region.rs:105 FilteredGraph::new_subgraph, previously in file /tmp/.tmpJ9kcgB/portgraph/src/view/subgraph.rs:73 FilteredGraph::is_convex, previously in file /tmp/.tmpJ9kcgB/portgraph/src/view/subgraph.rs:100 FilteredGraph::is_convex_with_checker, previously in file /tmp/.tmpJ9kcgB/portgraph/src/view/subgraph.rs:106 --- failure module_missing: pub module removed or renamed --- Description: A publicly-visible module cannot be imported by its prior path. A `pub use` may have been removed, or the module may have been renamed, removed, or made non-public. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/module_missing.ron Failed in: mod portgraph::py_graph, previously in file /tmp/.tmpJ9kcgB/portgraph/src/py_graph.rs:1 mod portgraph::view::subgraph, previously in file /tmp/.tmpJ9kcgB/portgraph/src/view/subgraph.rs:1 mod portgraph::view::region, previously in file /tmp/.tmpJ9kcgB/portgraph/src/view/region.rs:1 --- failure struct_missing: pub struct removed or renamed --- Description: A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/struct_missing.ron Failed in: struct portgraph::view::region::RegionContext, previously in file /tmp/.tmpJ9kcgB/portgraph/src/view/region.rs:35 struct portgraph::view::filter::FilteredGraphCtx, previously in file /tmp/.tmpJ9kcgB/portgraph/src/view/filter.rs:131 struct portgraph::view::subgraph::SubgraphContext, previously in file /tmp/.tmpJ9kcgB/portgraph/src/view/subgraph.rs:53 --- failure trait_removed_associated_type: trait's associated type was removed --- Description: A public trait's associated type was removed or renamed. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/trait_removed_associated_type.ron Failed in: associated type LinkView::Neighbours, previously at /tmp/.tmpJ9kcgB/portgraph/src/view.rs:291 associated type LinkView::NodeConnections, previously at /tmp/.tmpJ9kcgB/portgraph/src/view.rs:296 associated type LinkView::NodeLinks, previously at /tmp/.tmpJ9kcgB/portgraph/src/view.rs:302 associated type LinkView::PortLinks, previously at /tmp/.tmpJ9kcgB/portgraph/src/view.rs:308 associated type LinkView::Neighbours, previously at /tmp/.tmpJ9kcgB/portgraph/src/view.rs:291 associated type LinkView::NodeConnections, previously at /tmp/.tmpJ9kcgB/portgraph/src/view.rs:296 associated type LinkView::NodeLinks, previously at /tmp/.tmpJ9kcgB/portgraph/src/view.rs:302 associated type LinkView::PortLinks, previously at /tmp/.tmpJ9kcgB/portgraph/src/view.rs:308 associated type MultiView::NodeSubports, previously at /tmp/.tmpJ9kcgB/portgraph/src/view.rs:589 associated type MultiView::NodeSubports, previously at /tmp/.tmpJ9kcgB/portgraph/src/view.rs:589 associated type PortView::Nodes, previously at /tmp/.tmpJ9kcgB/portgraph/src/view.rs:22 associated type PortView::Ports, previously at /tmp/.tmpJ9kcgB/portgraph/src/view.rs:27 associated type PortView::NodePorts, previously at /tmp/.tmpJ9kcgB/portgraph/src/view.rs:32 associated type PortView::NodePortOffsets, previously at /tmp/.tmpJ9kcgB/portgraph/src/view.rs:37 associated type PortView::Nodes, previously at /tmp/.tmpJ9kcgB/portgraph/src/view.rs:22 associated type PortView::Ports, previously at /tmp/.tmpJ9kcgB/portgraph/src/view.rs:27 associated type PortView::NodePorts, previously at /tmp/.tmpJ9kcgB/portgraph/src/view.rs:32 associated type PortView::NodePortOffsets, previously at /tmp/.tmpJ9kcgB/portgraph/src/view.rs:37 ``` <details><summary>Changelog</summary> <blockquote> ## [0.13.0](v0.12.3...v0.13.0) - 2025-01-17 ### New Features - [**breaking**] Use RPITIT for graph traits (#156) - [**breaking**] Boundary order checking (#164) ### Performance - [**breaking**] Fix Subgraph O(n) complexity (#157) - [**breaking**] Manual impl of `Region`/`FlatRegion` for improved perf (#162) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/). --------- Co-authored-by: Agustín Borgna <[email protected]>

aborgna-q added 2 commits November 13, 2024 21:58

feat!: Use RPITIT for graph traits

1c3a1bf

Remove context-iterators dependency

17d5024

aborgna-q requested a review from doug-q November 14, 2024 09:59

doug-q approved these changes Nov 14, 2024

View reviewed changes

aborgna-q added 3 commits November 14, 2024 11:09

Require iterators to be Clone

50046c2

Things should be pub(crate)

901cfad

delegate! to internal iterator impls

bd79b3b

aborgna-q added this pull request to the merge queue Nov 14, 2024

Merged via the queue into main with commit ebaed87 Nov 14, 2024
11 of 12 checks passed

aborgna-q deleted the ab/rpitit branch November 14, 2024 11:38

hugrbot mentioned this pull request Nov 14, 2024

chore: release v0.13.0 #158

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Use RPITIT for graph traits #156

feat!: Use RPITIT for graph traits #156

aborgna-q commented Nov 14, 2024

hugrbot commented Nov 14, 2024

codecov bot commented Nov 14, 2024 •

edited

Loading

doug-q left a comment

doug-q Nov 14, 2024

aborgna-q Nov 14, 2024

doug-q Nov 14, 2024

aborgna-q Nov 14, 2024

doug-q Nov 14, 2024

aborgna-q Nov 14, 2024

doug-q Nov 14, 2024

doug-q Nov 14, 2024

aborgna-q Nov 14, 2024

feat!: Use RPITIT for graph traits #156

feat!: Use RPITIT for graph traits #156

Conversation

aborgna-q commented Nov 14, 2024

hugrbot commented Nov 14, 2024

codecov bot commented Nov 14, 2024 • edited Loading

Codecov Report

doug-q left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 14, 2024 •

edited

Loading