Rework get_installation_order to allow for dependency cycles

[pradyunsg]

Switches our ordering strategy to "weight" nodes to be based on: The length of the longest path, from root to the node (ignoring any paths containing cycles).

Our graph is a directed cyclic graph, so, that's fun to think about. :)

[uranusjr]

If you mean the root node in graph being None, my intention is to use it as a defined behaviour, not internal 🙂 Feel free to depend on it.

[pradyunsg]

If you mean the root node in graph being None, my intention is to use it as a defined behaviour, not internal 🙂 Feel free to depend on it.

Alrighty! IMO we should document this expectation on AbstractResolver. :)

---

All good then, this is ready for review... once I fix the failing test.

I don't think we'd ever get a graph where some edges aren't accessible from the root node. passed to the get_installation_order function, which is what the test is doing (which can be seen by renaming the packages). :)

        (
            [
                (None, "pkg1"),
                (None, "pkg2"),
                (None, "pkg3"),
                (None, "pkg4"),
                ("pkg5", "pkg1"),
                ("pkg6", "pkg1"),
                ("pkg7", "pkg1"),
                ("pkg7", "pkg5"),
                ("pkg7", "pkg6"),
            ],
            [
                "pkg1==0.0.1",
                "pkg5==0.0.1",
                "pkg6==0.0.1",
                "pkg7==0.0.1",
            ],
        ),

[uranusjr]

I don't think we'd ever get a graph where some edges aren't accessible from the root node.

You won’t, the resolver guarentees this. (Maybe worthwhile to add a test on ResolveLib though? Not sure.)

[pfmoore]

Maybe worthwhile to add a test on ResolveLib though?

Property based testing would be ideal for this :-)

[pradyunsg]

We want the length for the longest path to any node from root, ignoring any paths containing cycles.

Hmm...

A good test case for this is taking a graph like None -> A -> B -> C -> D -> E. We should have the same/similar result to this, even after adding any of the following edge-choices: None -> [BCDE], [AB] -> D, D -> [ABC].

[pradyunsg]

Sitting and working on this now.

[pradyunsg]

IT WORKS! IT WORKS! IT WORKS!

[uranusjr]

I can’t say I grok the logic, but can’t pick on an obvious problem either. All YAML tests on circular dependencies pass as well. Can you check what is failing this one?

tests/functional/test_yaml.py::test_yaml_based[install/conflicting_triangle]

I categorsied it under get_installation_order as well, but that is still failing. But maybe it’s for another reason now.

[pradyunsg]

tests/functional/test_yaml.py::test_yaml_based[install/conflicting_triangle]

Ah, yes.

That's a test that's supposed to be testing for what conflicts we report (there's a conflicting "response" there). We don't have support for checking for that, in the YAML tests themselves, so... it'll stay the way it is. It's being skipped for all resolvers right now, but exists as a optimistic "we'll get there one day" statement. :)

[pfmoore]

I like the way that algorithm turned out! It's pretty comprehensible, which isn't always the case for graph algorithms 🙂