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

fix: merge nodes more aggressively when unsat #27

Merged
merged 1 commit into from
Jan 31, 2024

Conversation

aochagavia
Copy link
Contributor

Until now the merging algorithm refused to merge nodes with incoming conflicting edges. I don't remember why that was necessary, and can't think of a reason now (if anyone does, please chime in!)... This PR removes that limitation because it improves error messages some more (see the new and updated snapshots).

As an example, consider the difference in output for rip "requests>2.2" "requests<2.1" (taken from this issue). You can also have a look at the updated insta snapshots.

This addresses one of the issues raised in #9, leaving the "deduce version ranges" part for a future PR.

Before

 × Could not solve for requested requirements
  ╰─▶ The following packages are incompatible
      |-- requests <2.1 can be installed with any of the following options:
          |-- requests 2.0.1
          |-- requests 2.0.0
          |-- requests 1.2.3
          |-- requests 1.2.2
          |-- requests 1.2.1
          |-- requests 1.2.0
          |-- requests 1.1.0
          |-- requests 1.0.4
          |-- requests 1.0.3
          |-- requests 1.0.2
          |-- requests 1.0.1
          |-- requests 1.0.0
          |-- requests 0.14.2
          |-- requests 0.14.1
          |-- requests 0.14.0
          |-- requests 0.13.9
          |-- requests 0.13.8
          |-- requests 0.13.7
          |-- requests 0.13.6
          |-- requests 0.13.5
          |-- requests 0.13.4
          |-- requests 0.13.3
          |-- requests 0.13.2
          |-- requests 0.13.1
          |-- requests 0.13.0
          |-- requests 0.12.1
          |-- requests 0.12.0
          |-- requests 0.11.2
          |-- requests 0.11.1
          |-- requests 0.10.8
          |-- requests 0.10.7
          |-- requests 0.10.6
          |-- requests 0.10.4
          |-- requests 0.10.3
          |-- requests 0.10.2
          |-- requests 0.10.1
          |-- requests 0.10.0
          |-- requests 0.9.3
          |-- requests 0.9.2
          |-- requests 0.9.1
          |-- requests 0.9.0
          |-- requests 0.8.9
          |-- requests 0.8.8
          |-- requests 0.8.7
          |-- requests 0.8.6
          |-- requests 0.8.5
          |-- requests 0.8.4
          |-- requests 0.8.3
          |-- requests 0.8.2
          |-- requests 0.8.1
          |-- requests 0.8.0
          |-- requests 0.7.6
          |-- requests 0.7.5
          |-- requests 0.7.4
          |-- requests 0.7.3
          |-- requests 0.7.2
          |-- requests 0.7.1
          |-- requests 0.7.0
          |-- requests 0.6.6
          |-- requests 0.6.5
          |-- requests 0.6.4
          |-- requests 0.6.3
          |-- requests 0.6.2
          |-- requests 0.6.1
          |-- requests 0.6.0
          |-- requests 0.5.1
          |-- requests 0.5.0
          |-- requests 0.4.1
          |-- requests 0.4.0
          |-- requests 0.3.4
          |-- requests 0.3.3
          |-- requests 0.3.2
          |-- requests 0.3.1
          |-- requests 0.3.0
          |-- requests 0.2.4
          |-- requests 0.2.3
          |-- requests 0.2.2
          |-- requests 0.2.1
          |-- requests 0.2.0
      |-- requests >2.2 cannot be installed because there are no viable options:
          |-- requests 2.2.1 | 2.3.0 | 2.4.0 | 2.4.1 | 2.4.2 | 2.4.3 | 2.5.0 | 2.5.1 | 2.5.2 | 2.5.3 | 2.6.0 | 2.6.1 | 2.6.2 | 2.7.0 | 2.8.0 | 2.8.1 | 2.9.0 | 2.9.1 | 2.9.2 | 2.10.0 | 2.11.0 |     
      2.11.1 | 2.12.0 | 2.12.1 | 2.12.2 | 2.12.3 | 2.12.4 | 2.12.5 | 2.13.0 | 2.14.0 | 2.14.1 | 2.14.2 | 2.15.1 | 2.16.0 | 2.16.1 | 2.16.2 | 2.16.3 | 2.16.4 | 2.16.5 | 2.17.0 | 2.17.1 | 2.17.2 |   
      2.17.3 | 2.18.0 | 2.18.1 | 2.18.2 | 2.18.3 | 2.18.4 | 2.19.0 | 2.19.1 | 2.20.0 | 2.20.1 | 2.21.0 | 2.22.0 | 2.23.0 | 2.24.0 | 2.25.0 | 2.25.1 | 2.26.0 | 2.27.0 | 2.27.1 | 2.28.0 | 2.28.1 |   
      2.28.2 | 2.29.0 | 2.30.0 | 2.31.0, which conflicts with the versions reported above.

After

  × Could not solve for requested requirements
  ╰─▶ The following packages are incompatible
      |-- requests <2.1 can be installed with any of the following options:
          |-- requests 0.2.0 | 0.2.1 | 0.2.2 | 0.2.3 | 0.2.4 | 0.3.0 | 0.3.1 | 0.3.2 | 0.3.3 | 0.3.4 | 0.4.0 | 0.4.1 | 0.5.0 | 0.5.1 | 0.6.0 | 0.6.1 | 0.6.2 | 0.6.3 | 0.6.4 | 0.6.5 | 0.6.6 |       
      0.7.0 | 0.7.1 | 0.7.2 | 0.7.3 | 0.7.4 | 0.7.5 | 0.7.6 | 0.8.0 | 0.8.1 | 0.8.2 | 0.8.3 | 0.8.4 | 0.8.5 | 0.8.6 | 0.8.7 | 0.8.8 | 0.8.9 | 0.9.0 | 0.9.1 | 0.9.2 | 0.9.3 | 0.10.0 | 0.10.1 |      
      0.10.2 | 0.10.3 | 0.10.4 | 0.10.6 | 0.10.7 | 0.10.8 | 0.11.1 | 0.11.2 | 0.12.0 | 0.12.1 | 0.13.0 | 0.13.1 | 0.13.2 | 0.13.3 | 0.13.4 | 0.13.5 | 0.13.6 | 0.13.7 | 0.13.8 | 0.13.9 | 0.14.0 |   
      0.14.1 | 0.14.2 | 1.0.0 | 1.0.1 | 1.0.2 | 1.0.3 | 1.0.4 | 1.1.0 | 1.2.0 | 1.2.1 | 1.2.2 | 1.2.3 | 2.0.0 | 2.0.1
      |-- requests >2.2 cannot be installed because there are no viable options:
          |-- requests 2.2.1 | 2.3.0 | 2.4.0 | 2.4.1 | 2.4.2 | 2.4.3 | 2.5.0 | 2.5.1 | 2.5.2 | 2.5.3 | 2.6.0 | 2.6.1 | 2.6.2 | 2.7.0 | 2.8.0 | 2.8.1 | 2.9.0 | 2.9.1 | 2.9.2 | 2.10.0 | 2.11.0 |     
      2.11.1 | 2.12.0 | 2.12.1 | 2.12.2 | 2.12.3 | 2.12.4 | 2.12.5 | 2.13.0 | 2.14.0 | 2.14.1 | 2.14.2 | 2.15.1 | 2.16.0 | 2.16.1 | 2.16.2 | 2.16.3 | 2.16.4 | 2.16.5 | 2.17.0 | 2.17.1 | 2.17.2 |   
      2.17.3 | 2.18.0 | 2.18.1 | 2.18.2 | 2.18.3 | 2.18.4 | 2.19.0 | 2.19.1 | 2.20.0 | 2.20.1 | 2.21.0 | 2.22.0 | 2.23.0 | 2.24.0 | 2.25.0 | 2.25.1 | 2.26.0 | 2.27.0 | 2.27.1 | 2.28.0 | 2.28.1 |   
      2.28.2 | 2.29.0 | 2.30.0 | 2.31.0, which conflicts with the versions reported above.

@aochagavia
Copy link
Contributor Author

aochagavia commented Jan 31, 2024

FYI I'll be trying to come up with a counterexample on paper for a while, where merging stuff with incoming conflicts would be problematic. If I find one, I'll add it to the test suite, so we can put the check back in the code and prevent people in the future from thinking they can just remove it.

@baszalmstra baszalmstra merged commit 357a64d into mamba-org:main Jan 31, 2024
3 checks passed
@aochagavia aochagavia deleted the more-aggressive-merging branch January 31, 2024 12:58
@aochagavia
Copy link
Contributor Author

@AntoinePrv would you mind having a quick look here? I wrote the original implementation following your blog post and took care to avoid merging conflicting nodes, because in your article you were doing so too. Now it seems like it would be beneficial to merge conflicting nodes after all, and I'm unable to see any downsides. So just to be sure: can you recall why you disallowed merging conflicting nodes? If it turns out to be problematic, I'd love to have a counterexample for our test suite.

@AntoinePrv
Copy link
Member

Hi @aochagavia

I believe you refer to this sentence:

Compression is achieved by merging nodes with the same package name that have the same predecessors and the same successors in the graph, and are not in conflict.

It's true that the "are not in conflict" is vague. What is meant is do not merge a node (most often a leaf) with the very nodes it is in conflict with. Otherwise in this example, you'de be merging nodes from requests <2.1 with nodes from requests>2.2.

In practice, we never had this constraint in mamba, this is all good!

You can see Mamba's merging criteria here.

And an output for similar example below.

error    libmamba Could not solve for environment specs
    The following packages are incompatible
    ├─ requests <2.20  is requested and can be installed;
    └─ requests >2.21  is not installable because it conflicts with any installable versions previously reported.

Note that we have a special rule that does not expand the dependencies into a list packages (keep the matchspec) in certain cases (I think the rule is top-level and no child 🤷)

@aochagavia
Copy link
Contributor Author

aochagavia commented Jan 31, 2024

Great! Thanks for the clarification

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

3 participants