Add test demoing that not all chains are found#193
Add test demoing that not all chains are found#193Peter554 wants to merge 1 commit intopython-grimp:masterfrom
Conversation
CodSpeed Performance ReportMerging #193 will not alter performanceComparing Summary
|
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.
Okay let's close this one then - seems like just a misunderstanding on my part 🙈 |

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_layersthat I think might be misunderstood.I think in discussions before we'd said that
find_illegal_dependencies_for_layersfinds 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 🙈