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: AmbiguousRuleException bug caused by weak ordering of rules #1124

Conversation

dkuzminov
Copy link
Contributor

@dkuzminov dkuzminov commented Aug 6, 2021

Link to the issue: #1106
Bug description on StackOverflow: https://stackoverflow.com/questions/68514884/ambiguousruleexception-when-there-is-no-ambiguity

Description

The AmbiguousRuleException is being thrown whenever there is no ambiguity. This behaviour is not deterministic, and there is no simple way to predict whether the invocation of the pipeline would produce this error.

The actual problem is incorrect algorithm of rule selection out of the list of possible producers. This list is being sorted, but there is no strict order of the rules unless explicitly specified by ruleorder. The rule that has no explicit ruleorder is neither greater nor smaller than other rules, and the result of sorting such a list becomes incorrect. Consider three rules: smallest, neutral, greatest. There is an explicit ruleorder: greatest > smallest. That means that:

__lt__(smallest, greatest) == True
__lt__(greatest, smallest) == False
__lt__(smallest, neutral) == __lt__(neutral, smallest) == __lt__(greatest, neutral) == __lt__(neutral, greatest) == False

That the result of sorted([smallest, neutral, greatest]) would be [smallest, neutral, greatest] while the result of sorted([greatest, neutral, smallest]) would be [greatest, neutral, smallest], which is obviously incorrect.

This commit addresses the issue and changes the logic of producer selection removing the dependency on the result of incorrect sorting.

QC

The changes cannot be verified with a deterministic test case because the issue depends on the random order of rules processing. Anyway, the change shall not break other test cases.

  • The PR contains a test case for the changes or the changes are already covered by an existing test case.
  • The documentation (docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).

@dkuzminov dkuzminov requested a review from johanneskoester as a code owner Aug 6, 2021
@dkuzminov dkuzminov force-pushed the fix/incorrect-sorting-of-ambiguous-rules branch from c8de975 to 9fd4158 Compare Aug 6, 2021
@dkuzminov dkuzminov changed the title #1106: Fix AmbiguousRuleException bug caused by weak ordering of rules fix AmbiguousRuleException bug caused by weak ordering of rules Aug 6, 2021
@dkuzminov dkuzminov changed the title fix AmbiguousRuleException bug caused by weak ordering of rules fix: AmbiguousRuleException bug caused by weak ordering of rules Aug 6, 2021
@dkuzminov dkuzminov force-pushed the fix/incorrect-sorting-of-ambiguous-rules branch from 9fd4158 to ed948f9 Compare Aug 11, 2021
@sonarcloud
Copy link

sonarcloud bot commented Aug 12, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@johanneskoester johanneskoester merged commit 7f54c39 into snakemake:main Aug 23, 2021
6 checks passed
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

2 participants