special_vars are a flavor of host specific task vars - #85424
Conversation
|
+1 for this approach, I really don't think we should add yet another level of variable precedence ... |
2d07432 to
70f16f4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
851a822 to
1f5352c
Compare
| fb_copy = final_block.copy(exclude_parent=True) | ||
| fb_copy._parent = final_block._parent | ||
| fb_copy.vars |= hv.vars | ||
| all_blocks[hv.host].append(fb_copy) |
There was a problem hiding this comment.
I want to call attention to this change. This can be a performance degradation, but without making copies of the block for each host, this approach falls apart. Generally speaking it is not likely to be a source of trouble, but worth reviewing this carefully.
There was a problem hiding this comment.
In my testing at least the wall clock time dropped with this PR.
We could probably (micro-)optimize the Block.copy() method as well (which I started playing with in #86936).
mkrizek
left a comment
There was a problem hiding this comment.
A couple of minor suggestions but 👍🏻 overall.
| fb_copy = final_block.copy(exclude_parent=True) | ||
| fb_copy._parent = final_block._parent | ||
| fb_copy.vars |= hv.vars | ||
| all_blocks[hv.host].append(fb_copy) |
There was a problem hiding this comment.
In my testing at least the wall clock time dropped with this PR.
We could probably (micro-)optimize the Block.copy() method as well (which I started playing with in #86936).
9ae5660 to
ea290a9
Compare
SUMMARY
special_vars are a flavor of host specific task vars. ci_complete
The issue here is that
IncludedVarsuses 5 things for uniqueness:There is currently a 1:1 relationship between
IncludedFileandspecial_vars. So if theloopis dependent on a host variable,special_varswill differ per host, creating a 1:1 relationship betweenIncludedFileandHost. Whereas the expectation is to have a 1:many relationship.This PR creates a new dataclass specifically for handling this, and removing the
varscomparison, but continuing to link it with the host, but without changing the existing variable precedence.So this change tracks the vars with the host, but doesn't use the vars for comparisons. It then creates copies of the task for the hosts, and merges the vars.
Another solution may be to add a new step in the variable precendence rules for this use, instead of copying the task, and merging the task vars.
Reproducer:
ISSUE TYPE
Addditional Context
Outputs:
devel...
This PR...