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

Only backjump if the current broken state is an incompatible dependency #155

Conversation

notatallshaw
Copy link
Collaborator

@notatallshaw notatallshaw commented Jul 21, 2024

This appears to fix backjumping, in all cases, while keeping the performance benefits.

I do not have an intuitive explanation, but I created a series of tests and printed out debugging information and looked at when the current backjumping made the wrong choice, and it was always when the current broken state's name was not in the incompatible dependencies.

I have added two tests that currently fail that I constructed from my understanding of when the problem occurs.

I have created another test, not in this PR, based on kedro[test]==0.18.13, however the JSON is massive (450k lines), and it would require a better get_preference to complete in a reasonable number of rounds. Let me know if you want me to try and get that test working for this or another PR.

@frostming
Copy link
Member

This looks a great and neat fix. I am willing to have this merged, @uranusjr @pradyunsg

@notatallshaw
Copy link
Collaborator Author

FYI, I'm still trying to wrangle kedro into a test, but it might take a while, I'm off on vacation Wednesday evening. I see no problem landing it in another PR though.

@notatallshaw notatallshaw force-pushed the skip-backjumping-when-name-is-not-incompatible branch from 8e1d20a to 8147ef6 Compare July 30, 2024 20:43
@pradyunsg pradyunsg merged commit 58c5d90 into sarugaku:main Jul 30, 2024
9 checks passed
@notatallshaw
Copy link
Collaborator Author

Thanks!

I was just about to commit some new tests, new PR coming in a minute

@pradyunsg
Copy link
Contributor

Sounds good -- this change looks fine, and I think I can reason about it being correct?

@notatallshaw
Copy link
Collaborator Author

notatallshaw commented Jul 30, 2024

I think I can reason about it being correct?

I'm going to be honest, I can not, it was never clear to me that backjumping was proven correct within resolvelib. I only found this by very carefully looking at the paths taken with and without backjumping in these minimal examples and looking at the variables when it appeared to take the wrong path.

So my plan is examples and tests:

  • Real world example kedro[test]==0.18.1 now resolves correctly
  • Added test cases here that are fixed by this
  • Opened a separate PR that shows backjumping is still skipping unnecessary paths

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.

3 participants