-
Notifications
You must be signed in to change notification settings - Fork 94
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
Suicide trigger prevents later cycles starting #6602
base: 8.4.x
Are you sure you want to change the base?
Conversation
23bc488
to
0700c90
Compare
# Don't count self-suicide as a normal trigger. | ||
dep.suicide and trig.task_name == self.name | ||
# Don't count suicide as a normal trigger: | ||
dep.suicide |
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 undoes a change @hjoliver made in #4970
5e7ffad#diff-b46dc58446bab522ef576c6d2e32a4276f57d60ff62e930e6f47762d47bf9507R296
I think this change makes sense, but will need to be reviewed in the context of whatever #4970 was trying to resolve.
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 spotting ...
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.
(leaving you two to resolve the differences between these changes)
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.
@hjoliver what do you think we should do about this?
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.
Sorry, I got distracted on the way to looking at that...
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.
Actually, by testing, it seems that line is no longer needed at all. Correction, 17-c7backcompat-self-suicide-cycling.t
fails without the remaining bit of the line.
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.
Isn't that the test for your earlier change though?
Why did you make this change
Because it appears to work in this case, but on more thought I don't think that its right, I think that I need to have another look. Marked PR as draft pending further thought.
I think that in the example from the ticket
install_cold[^] => foo? => bar?
# The line below stops cycle points 1991 and 1992
# from starting:
bar:fail? => !foo?
If bar fails, then foo will only have absolute triggers, but if bar succeeds then foo has a cycling trigger (bar:fail). The outcome of this method depends on somthing which hasn't yet happened!
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.
Isn't that the test for your earlier change though?
Yes, it's one of the two functional tests added in the old PR.
If bar fails, then foo will only have absolute triggers, but if bar succeeds then foo has a cycling trigger (bar:fail). The outcome of this method depends on somthing which hasn't yet happened!
It's not dynamically determined like that. The trigger exists in the graph, even if runtime events result it in not being needed.
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.
The problem that leaves us with is that the user expects a
to act like it is parentless, but it's not: It's not unreasonable to suggest that the non existance of an optional outcome to optional suicide trigger dependency (b:fail? => !a?
) shouldn't stop a
from triggering in subsequent cycles.
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.
Actually, by testing, it seems that line is no longer needed at all.Correction,17-c7backcompat-self-suicide-cycling.t
fails without the remaining bit of the line.
Does it?
Closes #6594
Check List
CONTRIBUTING.md
and added my name as a Code Contributor.setup.cfg
(andconda-environment.yml
if present).?.?.x
branch.