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
perf: improve job selection performance in case of potential ambiguity that is resolved by comprehensive ruleorder statements. #1147
Conversation
…y that is resolved by comprehensive ruleorder statements.
@dkuzminov you probably want to have a look at this? |
Please format your code with black: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this improvement is incorrect. Assume there is a job primary
that dominates all other jobs. Next, imagine that this rule is disregarded later because of missing input. We would get an incorrect is_strictly_ordered
value because we know nothing about the ordering of the rest of the jobs. We would select the first producer (other than primary
) even if it is ambiguous.
snakemake/dag.py
Outdated
# check if all potential producers are strictly ordered | ||
jobs = sorted(jobs, reverse=True) | ||
primary_job = jobs[0] | ||
is_strictly_ordered = all(primary_job > job for job in jobs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct, as the first element shall be compared to the rest of the elements:
is_strictly_ordered = all(primary_job > job for job in jobs[1:])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
Kudos, SonarCloud Quality Gate passed!
|
Right, I missed that case indeed. I have worked a bit on it. Still not 100% validated in my mind, but I think this is more correct (although it is 10pm here now, so I might miss something again ;-)). |
I think it is fine. |
QC
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).