Allow declaration of "out-of-band" dependencies#726
Conversation
The "out-of-band" declaration on a dependency is intended to indicate that the dependency is a requirement but not a prerequisite for processing. As such, operations like "--packages-up-to" and graph computation will still show the relationship, but the topological ordering operation will not require the dependency as a prerequisite.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #726 +/- ##
=======================================
Coverage 87.21% 87.21%
=======================================
Files 69 69
Lines 4106 4106
Branches 709 709
=======================================
Hits 3581 3581
Misses 414 414
Partials 111 111 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if not ready: | ||
| # if nothing is ready, try removing out-of-band dependencies | ||
| ready = [ | ||
| decorator for decorator, r in queued.items() | ||
| if not r.difference(out_of_band[decorator]) | ||
| ] | ||
| # prefer those with less out-of-band dependencies first | ||
| ready.sort(key=lambda d: ( | ||
| len(out_of_band[d]), d.descriptor.name | ||
| )) | ||
| # take only one at a time | ||
| ready = ready[:1] |
There was a problem hiding this comment.
Should this section be optional with a condition somehow? The out-of-band concept is a bit tricky to grasp, it would be nice to add an example to illustrate what we want.
We discussed it in a meeting, but it's not clear to me from the PR's code completely how this will work.
There was a problem hiding this comment.
I tried to write something to help answer your question, and it led me to really look at what's happening here, and it's just flat-out wrong. So the review process is 100% working here 😆 💯
This code was intended to help minimize the amount of out-of-order dependencies in the topological ordering. Instead of ignoring them outright, the idea was to wait until we have no option left but to break package ordering.
The main oversight here is that a DependencyDescriptor inherits equality operators from str, so tracking the dict of out_of_band decorators is broken. It results in treating nodes within the graph as out-of-order, when we want to treat edges in the graph as out-of-order. I'll fix it.
Second thought: I need to add an explicit test for this, particularly if it includes heuristics and extra complexity to treat the out-of-order dependencies as a last resort.
Third thought: If we're going to ignore these dependencies in topological ordering, we should also ignore them for job execution. Right now, we wouldn't need to change the sequential executor because it assumes that the packages (and therefore jobs) are in topological order and can be executed one-by-one without even thinking about dependencies. The parallel executor, however, needs to be changed to ignore out-of-band dependencies or it will deadlock when it sees them.
The overall picture here is that out-of-band dependencies SHOULD:
- Be considered as part of
--packages-up-to, which should continue to downselect based on dependencies, not prerequisites. - Be represented in
colcon graphandcolcon list -t, which also enumerate dependencies.
The out-of-band dependencies SHOULD NOT:
- Block execution of a package's task (i.e. the package build) if the upstream task has not yet completed (whether not attempted, in progress, or failed).
In our target use case, we will use #725 to interact with job creation in such a way that the new "crate" jobs become de-facto prerequisites (out-of-band is not True) even if building the actual packages targeted by out-of-band dependencies haven't been properly "built" yet.
tl;dr -
- I'll update this PR to fix the bug and add tests.
- I'll open a PR against
colcon-parallel-executorto drop the existing "prerequisite" behavior for out-of-band dependencies.
There was a problem hiding this comment.
Sounds good!
Second thought: I need to add an explicit test for this, particularly if it includes heuristics and extra complexity to treat the out-of-order dependencies as a last resort.
I think this was particular concern, it's unclear when this last resort case happens. The PR should include a test illustrating the case it wants to cover.
The parallel executor, however, needs to be changed to ignore out-of-band dependencies or it will deadlock when it sees them.
I didn't think about this, but yes, we should also consider how this will play out with both executors. Sounds like this last resort mechanism might change the end result based on the order in which the jobs are executed. Something to consider and track.
|
Alright, I simplified this change and added a test. We can revisit the idea behind "best effort" use of out-of-band dependencies later. It could result in a more expected execution order, but isn't strictly necessary to land the feature. |
|
I'm just not happy with how this looks. Converting to draft for now. |
2 participants
The "out-of-band" declaration on a dependency is intended to indicate that the dependency is a requirement but not a prerequisite for processing. As such, operations like "--packages-up-to" and graph computation will still show the relationship, but the topological ordering operation will not require the dependency as a prerequisite.