Fix for the case when an unrelated package is installed.

[cournape]

@agrawalprash thanks for the PR. Can you explain the issue you are trying to solve ?

[johntyree]

This is odd. Pruning should remove packages that don't matter and in any case ALL packages should be either assigned or unassigned so checking both should be unnecessary.

[agrawalprash]

@cournape @johntyree I'm trying to install a package in the scenario described by the test. Without the fix, it fails with the following traceback:

======================================================================
ERROR: test_install_when_some_unrelated_packages_are_installed (simplesat.tests.test_solver.TestSolver)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/prash/work/sat-solver/simplesat/tests/test_solver.py", line 174, in test_install_when_some_unrelated_packages_are_installed
    transaction = self.resolve(request)
  File "/Users/prash/work/sat-solver/simplesat/tests/test_solver.py", line 47, in resolve
    return solver.solve(request)
  File "/Users/prash/work/sat-solver/simplesat/dependency_solver.py", line 311, in solve
    solution = sat_solver.search()
  File "/Users/prash/work/sat-solver/simplesat/sat/minisat.py", line 436, in search
    self.clauses,
  File "/Users/prash/work/sat-solver/simplesat/sat/policy/policy_logger.py", line 23, in get_next_package_id
    pkg_id = self._policy.get_next_package_id(assignments, clauses)
  File "/Users/prash/work/sat-solver/simplesat/sat/policy/undetermined_clause_policy.py", line 69, in get_next_package_id
    self._refresh_decision_set(assignments)
  File "/Users/prash/work/sat-solver/simplesat/sat/policy/undetermined_clause_policy.py", line 136, in _refresh_decision_set
    self._update_cache_from_assignments(assignments)
  File "/Users/prash/work/sat-solver/simplesat/sat/policy/undetermined_clause_policy.py", line 47, in _update_cache_from_assignments
    for clause in self._id_to_clauses[key]:
KeyError: 3

[johntyree]

That error is going to haunt me for the rest of my days.

It means that something got assigned for the first time without us seeing it in the change log and tracking it.

I thought we got all the edge cases but apparently not.

I'm offline now, but my advice is to check when that ID got assigned and maybe sure the policy sees the change log afterwards. Otherwise you're just quieting a symptom of a logical error On May 19, 2016 16:55, "Prashant Agrawal" notifications@github.com wrote:

@cournape https://github.com/cournape @johntyree https://github.com/johntyree I'm trying to install a package in the scenario described by the test. Without the fix, it fails with the following traceback:

ERROR: test_install_when_some_unrelated_packages_are_installed

(simplesat.tests.test_solver.TestSolver)

Traceback (most recent call last): File "/Users/prash/work/sat-solver/simplesat/tests/test_solver.py", line 174, in test_install_when_some_unrelated_packages_are_installed transaction = self.resolve(request) File "/Users/prash/work/sat-solver/simplesat/tests/test_solver.py", line 47, in resolve return solver.solve(request) File "/Users/prash/work/sat-solver/simplesat/dependency_solver.py", line 311, in solve solution = sat_solver.search() File "/Users/prash/work/sat-solver/simplesat/sat/minisat.py", line 436, in search self.clauses, File "/Users/prash/work/sat-solver/simplesat/sat/policy/policy_logger.py", line 23, in get_next_package_id pkg_id = self._policy.get_next_package_id(assignments, clauses) File "/Users/prash/work/sat-solver/simplesat/sat/policy/undetermined_clause_policy.py", line 69, in get_next_package_id self._refresh_decision_set(assignments) File "/Users/prash/work/sat-solver/simplesat/sat/policy/undetermined_clause_policy.py", line 136, in _refresh_decision_set self._update_cache_from_assignments(assignments) File "/Users/prash/work/sat-solver/simplesat/sat/policy/undetermined_clause_policy.py", line 47, in _update_cache_from_assignments for clause in self._id_to_clauses[key]: KeyError: 3

— You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub https://github.com/enthought/sat-solver/pull/215#issuecomment-220279470

[cournape]

Ok, so following @johntyree comment, I think this PR is just hiding a deeper problem, but I see that John beat me to it :)

[agrawalprash]

@cournape @johntyree Thanks. This PR was originally meant to be a GitHub issue but I thought if a one-line fix passes all the tests then everything should be okay. Of course you guys know about this better than me so I'll leave it to you from here onwards.

[cournape]

@noraderam can you look at this one ? Prashant was nice enough to give us simple reproducible case !

[johntyree]

Check master from after I fixed the bug that Didrik found. Might be a regression. On May 19, 2016 5:07 PM, "David Cournapeau" notifications@github.com wrote:

@noraderam https://github.com/noraderam can you look at this one ? Prashant was nice enough to give us simple reproducible case !

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/enthought/sat-solver/pull/215#issuecomment-220282125

[noraderam]

Check master from after I fixed the bug that Didrik found. Might be a regression.

Looks like it's been around for a bit longer than that. According to a git bisect, it was introduced by this commit: https://github.com/enthought/sat-solver/pull/130/commits/45b6d1d8fca03c1c58997a2c98efb1514c5dd4c0

I'll get more into the inner workings to determine what's going on.

[johntyree]

Hmm. That looks like the commit that introduced assignment tracking to that policy by having it inspect the change log of the assignment set.

The clauses argument to build_I'd_to_clauses is incomplete. That's my guess anyway. Good luck!

Check master from after I fixed the bug that Didrik found. Might be a regression.

Looks like it's been around for a bit longer than that. According to a git bisect, it was introduced by this commit: 45b6d1d https://github.com/enthought/sat-solver/pull/130/commits/45b6d1d8fca03c1c58997a2c98efb1514c5dd4c0

I'll get more into the inner workings to determine what's going on.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/enthought/sat-solver/pull/215#issuecomment-220365570

[noraderam]

Good to still have you around, @johntyree 😄 Thanks for the input!

[noraderam]

Looks like the problem occurs in the RulesGenerator, not the policy. For the particular situation outlined, we do not create any rules for the originally installed package. There are then no clauses defined, so consuming the changelog fails.

The added test fails because foo, the previously installed package, exists in the installed_repository but not in the remote repository. Conflicts rules are generally created for previously installed packages, making sure that something providing the package is installed. In this test case only one package "provides" the given requirement. Generally there would be at least two: one in the remote repository and one in the installed repository.*

The scenario tests are not currently set up to handle the situation where a package is not installed but is not available in the remote repository. I can get around that by modifying the Scenario.from_yaml setup.

@agrawalprash, I want to make sure that the situation described here is actually what you're experiencing before proceeding with a fix.

* Adding the line self.repository.add_package(foo) does not make the test start passing because the TestSolver tests use PackageMetadata instead of RepositoryPackageMetadata. The PackageMetadata will be equal to itself regardless of what repository it belongs to. RepositoryPackageMetadata is used in scenario tests, distinguishing between installed and remote.

[noraderam]

Even in the situation described above, the solver should behave correctly because any Request should include the previously installed packages as install jobs.

@agrawalprash, were you using sat-solver directly when you encountered the error? If so, did your Request include both foo and bar?

[johntyree]

The previously installed packages should get all necessary rules regardless. They come from direct inspection of the installed repository.

https://github.com/enthought/sat-solver/blob/c8312ea01fc1112a42049eff7657139fcddfe63a/simplesat/rules_generator.py#L177

https://github.com/enthought/sat-solver/blob/c8312ea01fc1112a42049eff7657139fcddfe63a/simplesat/rules_generator.py#L492

Is {foo: 3} not in the pool somehow? On May 20, 2016 03:54, "Nora Deram" notifications@github.com wrote:

Even in the situation described above, the solver should behave correctly because any Request should include the previously installed packages as install jobs.

@agrawalprash https://github.com/agrawalprash, were you using sat-solver directly when you encountered the error? If so, did your Request include both foo and bar?

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/enthought/sat-solver/pull/215#issuecomment-220444481

[agrawalprash]

@noraderam Originally, I was using sat-solver via another tool to test package installs in canopy_platform. The remote repository indeed didn't contain the installed package and that's a very valid use case for us (e.g. when users install a package and then disable the repository that supplied that package). On inspection, I saw that the request only contained the remote package (bar) and not the local package (foo).

And yes, @pankajp and I also tried to add foo to the remote repository yesterday suspecting that it might fix it, but it didn't because of the reason you gave. From an external user's perspective, it looks like the repository should take care of converting a PackageMetadata to a RepositoryPackageMetadata or whatever wherever appropriate, rather than the user having to cautiously supply the correct type but that may or may not be possible with the current design.

[noraderam]

The previously installed packages should get all necessary rules regardless. They come from direct inspection of the installed repository.

@johntyree add_installed_package_rules adds the conflict rules and the install_requires rules. If only one package is found by what_provides (which is equal the currently installed package) and that package has no dependencies or conflicts, no rules are added. https://github.com/enthought/sat-solver/blob/c8312ea01fc1112a42049eff7657139fcddfe63a/simplesat/rules_generator.py#L375-L376

We depend on the request to include an install operation for the package in order to get an install_one_of rule.

[noraderam]

The remote repository indeed didn't contain the installed package and that's a very valid use case for us (e.g. when users install a package and then disable the repository that supplied that package).

To clarify: I mistakenly initially thought that this was the cause, before I realized that we handle this elsewhere by including installed packages in the Request. There should not be any problem with packages which are not available in a remote repository.

From an external user's perspective, it looks like the repository should take care of converting a PackageMetadata to a RepositoryPackageMetadata or whatever wherever appropriate

Did you actually have a PackageMetadata object? I thought that was only a problem with the test case. Any metadata that gets created by an external tool should be a RepositoryPackageMetadata

[noraderam]

Closing: The underlying issue is now tracked by #217

After offline discussion, we found that the specific situation could not be reproduced using the same external tool; the environment must have been somehow broken.