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

feat!: decide on explicit requirements first #61

Merged
merged 9 commits into from
Sep 9, 2024

Conversation

baszalmstra
Copy link
Contributor

@baszalmstra baszalmstra commented Aug 6, 2024

Fixes #60

An implementation of the ideas discussed in #60. This PR opts to decide on requirements from the root over any requirement that the root does not directly depend on. This has the effect that the highest versions of requirements from the root are most likely picked instead of them being constrained by transitive dependencies.

The test_explicit_root_requirements test shows this in action. With the code before this PR the test would result in

a=1 # root requirement
b=2	# transitive requirement
c=1 # root requirement

whereas now the result is:

a=1 # root requirement
b=1 # transitive requirement
c=5 # root requirement

Note that the root requirements are optimized at the cost of the transitive requirement.

  • I still have to test what the impact on performance is for this PR.

@baszalmstra baszalmstra marked this pull request as draft August 6, 2024 13:09
@baszalmstra baszalmstra changed the title Feat/decide explicit first feat: decide on explicit requirements first Aug 6, 2024
@baszalmstra
Copy link
Contributor Author

I used the attached notebook to generate the following images to get a better idea what the performance impact of this method:

This shows that when running on a large set of examples with single requirements there is no real difference with the code from main.

Untitled

This shows the differences in solve times for individual packages:

Untitled

I conclude that at least for solving for specific solvables there is no noticable performance difference. 👍

But it would be good to profile this when solving for multiple requirements at the same time which will likely have a larger impact. Ill try that later.

@baszalmstra
Copy link
Contributor Author

I modified the benchmark program to select 1000 (deterministic) random set of requirements from the snapshot. I think this provides a better overview of what is going on.

Untitled
Untitled-1

These results show that on average solving becomes slower and although there are some cases that become (significantly) faster overall the solver becomes slightly slower (and in some cases significantly so).

I am a little unsure what to do. These changes also result in "better" outcomes where the root requirements are optimized. So I'm inclined to merge this.

Thoughts? @jjerphan @wolfv

@jjerphan
Copy link
Member

Hi @baszalmstra,

Thank you for posting those results. Let me block some time this week so that I can review it.

@jjerphan
Copy link
Member

I have not dived into the implementation, I am just looking at the second results given in #61 (comment).

I am a little unsure what to do. These changes also result in "better" outcomes where the root requirements are optimized. So I'm inclined to merge this.

I am also unsure about what to do here. Functionally, this is interesting, but do we want to have this new behavior be the default if some users know regression? Could this be introduced as a new alternative strategy?

@baszalmstra
Copy link
Contributor Author

Yeah, perhaps we can abstract the decision making logic and allow users to inject certain strategies.

jjerphan added a commit to jjerphan/resolvo that referenced this pull request Aug 20, 2024
Taken from mamba-org#61

Co-authored-by: Bas Zalmstra <[email protected]>

Signed-off-by: Julien Jerphanion <[email protected]>
jjerphan added a commit to jjerphan/resolvo that referenced this pull request Aug 20, 2024
Taken from mamba-org#61

Co-authored-by: Bas Zalmstra <[email protected]>

Signed-off-by: Julien Jerphanion <[email protected]>
@baszalmstra
Copy link
Contributor Author

We ran into this again today with pixi.

If you solve for python ~=3.10.0 and pytorch >2,<3 you get the CPU version of pytorch regardless of whether CUDA is available. When you change the pytorch requirement to 2.3.1 you get the CUDA enabled version.

The reason this happens is that there are lots of pytorch variants available so the solver will decide on it very late. Before it decides on pytorch it will decide on an openmp implementation and it will pick one that makes any CUDA enabled pytorch version incompatible. This is not what we want and it's also very hard for a user to make it do the right thing without messing with build strings.

I am therefore inclined to fix-up this PR and merge it, even though the performance might degrade a little. I am quite confident that once we merge #37 the performance regression introduced here will no longer be an issue anyway.

@baszalmstra baszalmstra marked this pull request as ready for review September 9, 2024 14:39
@jjerphan
Copy link
Member

jjerphan commented Sep 9, 2024

Up to you, @baszalmstra. I will come back to resolvo when I can.

@baszalmstra baszalmstra changed the title feat: decide on explicit requirements first feat!: decide on explicit requirements first Sep 9, 2024
@baszalmstra baszalmstra changed the title feat!: decide on explicit requirements first feat: decide on explicit requirements first Sep 9, 2024
@baszalmstra baszalmstra changed the title feat: decide on explicit requirements first feat!: decide on explicit requirements first Sep 9, 2024
@baszalmstra baszalmstra merged commit 187806c into mamba-org:main Sep 9, 2024
13 checks passed
@baszalmstra baszalmstra deleted the feat/decide_explicit_first branch September 9, 2024 14:53
tdejager pushed a commit to prefix-dev/pixi that referenced this pull request Sep 10, 2024
This bumps rattler to the latest version.

The most notable change is:

- mamba-org/resolvo#61
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.

Prefer solving for explicitly required dependencies first
2 participants