Skip to content

Add test demoing that not all chains are found#193

Closed
Peter554 wants to merge 1 commit intopython-grimp:masterfrom
Peter554:demo-not-all-chains-found
Closed

Add test demoing that not all chains are found#193
Peter554 wants to merge 1 commit intopython-grimp:masterfrom
Peter554:demo-not-all-chains-found

Conversation

@Peter554
Copy link
Collaborator

@Peter554 Peter554 commented Mar 1, 2025

This PR isn't intended to be merged (although it could be if we like). It's here to demo a behaviour of find_illegal_dependencies_for_layers that I think might be misunderstood.

I think in discussions before we'd said that find_illegal_dependencies_for_layers finds all chains. I might be misremembering. Anyways, I just want to put this test case here to demo that not all chains are found.

Why does this happen? The code here https://github.com/seddonym/grimp/blob/8c1bb5dd23087c4095c57347da46ae3110db439d/rust/src/graph/higher_order_queries.rs#L151-L157 pops all the imports along a chain when a chain is found.

Is there a way to find all the chains? I think not really (not in any performant way). I believe finding all paths between two nodes in a graph is an NP hard problem. So probably the existing behaviour is the best we can do. There are algorithms to find the K shortest paths (e.g. see https://docs.rs/pathfinding/latest/pathfinding/directed/yen/index.html), but even then you still have to limit the search somewhere (K paths).

Fine to close this PR if you like. I just wanted to put this here as a reminder of this behaviour and to provoke thought.

I wonder - should this behaviour be documented somewhere? I think the grimp docs don't explicitly say that all chains will be found, but equally they don't call out explicitly that some chains will not be found.

Update: I just found the existing test case test_longer_forked_routes_dont_appear, which is basically testing the same thing 🙈

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 1, 2025

CodSpeed Performance Report

Merging #193 will not alter performance

Comparing Peter554:demo-not-all-chains-found (f111c4c) with master (8c1bb5d)

Summary

✅ 17 untouched benchmarks

@seddonym
Copy link
Collaborator

seddonym commented Mar 4, 2025

I wonder - should this behaviour be documented somewhere? I think the grimp docs don't explicitly say that all chains will be found, but equally they don't call out explicitly that some chains will not be found.

It is documented here, right? https://grimp.readthedocs.io/en/stable/usage.html#return-value

@Peter554
Copy link
Collaborator Author

Peter554 commented Mar 4, 2025

I wonder - should this behaviour be documented somewhere? I think the grimp docs don't explicitly say that all chains will be found, but equally they don't call out explicitly that some chains will not be found.

It is documented here, right? https://grimp.readthedocs.io/en/stable/usage.html#return-value

Ah true I just read this bit 👇 and didn't read fully the rest of the text.

Screenshot 2025-03-04 at 10 21 41

Okay let's close this one then - seems like just a misunderstanding on my part 🙈

@Peter554 Peter554 closed this Mar 4, 2025
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