Fallbacks per solution #287
Conversation
Codecov Report
@@ Coverage Diff @@
## master #287 +/- ##
==========================================
+ Coverage 52.99% 53.83% +0.84%
==========================================
Files 102 102
Lines 7598 7783 +185
==========================================
+ Hits 4026 4189 +163
- Misses 3572 3594 +22
Continue to review full report at Codecov.
|
v4hn
force-pushed
the
mtc-fallback-reworked
branch
from
dc1c9ca to
b016a9a
Compare
rhaschke
left a comment
rhaschke
left a comment
There was a problem hiding this comment.
Thanks a lot for pushing this forward! I generally agree with the tests.
However, I need to add some test cases to point out the undesired behaviour I noticed (see below).
I pushed some minor cosmetic changes as well.
v4hn
force-pushed
the
mtc-fallback-reworked
branch
2 times, most recently
from
c86d799 to
2bac879
Compare
|
Good to see CI succeed for the first time in a while ;) |
|
I pushed a cleanup for |
| // This override is deliberately empty. | ||
| // The method prunes solution paths when a child failed to find a valid solution for it, | ||
| // but in Fallbacks the next child might still yield a successful solution | ||
| // Thus pruning must only occur once the last child is exhausted (inside computeFromExternal) |
There was a problem hiding this comment.
In general, I agree with you. We do need a mechanism for a stage to claim that it is exhausted. onNewFailure() doesn't perfectly fit that purpose, because it's called on any failure:
https://github.com/ros-planning/moveit_task_constructor/blob/f51f6eb982f90f3c31d9adfe1fc6e041d467c5bb/core/src/stage.cpp#L140-L143
I think we should rename the function to onExhausted and only call it when the solutions are actually exhausted. For basic Propagator and Connect stages that's always the case if they fail. However, Generator stages and containers will need special handling.
Actually, for Generator stages exhausted is closely related to !canCompute(). Not sure that assumption holds for generator-like containers as well.
I any case, we might need to distinguish between "currently exhausted" and "finally exhausted". For example, MonitoringGenerators might be currently not able to compute something, but later, if they received new input, they can. The same holds for connect-like containers, where we already have seen that we can re-enable previously pruned solution branches.
Given this new semantics of the function, the Fallbacks container can easily check whether child actually exhausted its solutions and thus propagate this info upwards if needed.
rhaschke
left a comment
rhaschke
left a comment
There was a problem hiding this comment.
I think the function computeFromExternal() only handles propagator stages. Connect-like stages require their own set of functions, because you actually want to consider each pair of (compatible) solutions separately - much like you consider each incoming external state of a propagator child separately now.
v4hn
force-pushed
the
mtc-fallback-reworked
branch
3 times, most recently
from
82d9163 to
6f808af
Compare
|
I pushed a bunch of new tests pointing out that priority propagation from external to internal interfaces of containers doesn't work yet. |
|
Sorry for the mess. It's too late... |
rhaschke
force-pushed
the
mtc-fallback-reworked
branch
from
6555a3e to
08e8529
Compare
|
I couldn't put the topic aside yet and pushed some more commits. Although the current (simple) tests work, the fundamental problem remains that a (fallback) container cannot decide yet whether a child is exhausted on a specific external state (a single one for propagating, a pair for connecting interfaces). If the simple ForwardMockups would get replaced by a SerialContainer that will take several However, I'm loosely convinced that we can solve the issue nevertheless: We need to change the semantics of If you want to keep (for now) the current implementation fixing the active pending state, I have this implemented as an alternative as well in my repo: v4hn/moveit_task_constructor@mtc-fallback-reworked...ubi-agni:mtc-fallback-reworked |
Weren't you the one who said we should coordinate our efforts? 💢 I have a feverish kid sitting on my lap, so I don't the nerves right now to either fixup&polish my local patches or review your commits from the last 17 hours. Let's see whether I can keep my eyes open for long enough this evening to look into it. |
|
Sorry to hear that. If it helps you, just force-push your changes. I'm curious what they address. But, as I said, I need to turn to other stuff for next week... |
|
I think we should split this PR.
|
v4hn
force-pushed
the
mtc-fallback-reworked
branch
from
08e8529 to
ea4a2c0
Compare
|
I finally finished splitting&rebasing the branch. I explicitly excluded v4hn@ece9cd0 because I changed the logic quite a lot in my own commit and I believe your patch can result in pushing multiple states to the same child and thus mess with pruning because there is nothing connecting the entry in pending_ to the corresponding call to |
v4hn
force-pushed
the
mtc-fallback-reworked
branch
from
ea4a2c0 to
cd980ad
Compare
|
@rhaschke if you should have time after summer school, iros and DGR days, this should be ready for merge (once more). |
|
Sorry for the delay. I will try to have a look into this over the weekend latest. |
Both, failed and pruned states might get re-enabled later! This also required rework (simplification) of the sorting function for pending pairs.
- Centrally distinguish between have owner() or not in InterfaceState::updatePriority() - Have a separate updateStatus() method to just update the pruning status - Split Interface::updatePriority() into a method taking the InterfaceState* and one taking an Interface::iterator (for efficiency) - Early return in container.cpp's updateStatePrios()
- Switch directions: FORWARD <-> BACKWARD to make the function reusable for status propagation. - We need to ignore the source state when looking for opposite states of the target state. Thus add both, source and target state arguments.
... to avoid explicit template initialization
... to better indicate that such a state can be immediately re-enabled.
This also requires to drop the assertion in SerialContainer::onNewSolution() that new solutions will have enabled start+end states (a CONNECT stage's solution might not).
As only the InterfaceState* variant is actually called, we can drop the splitting introduced for performance reasons in 29d1e44
Not only propagate updates along solution paths, but also bridge the gap of a `Connecting` stage. - If a state becomes enabled, re-enable opposite `ARMED` states as well. - If a state becomes pruned, also prune opposite states if they don't have alternatives. - Make sure that we don't run into a recursive update loop by disabling notify() callbacks.
Also remove unused pushInterface(dir)
to allow propagating status updates only if the STATUS actually changed.
... to use operator<<
rhaschke
left a comment
rhaschke
left a comment
There was a problem hiding this comment.
It took me quite a while to understand your refactorings as you provided only very sparse documentation, both in commit comments and in code 😞
The logic of switching between start and end queue is probably motivated by this commit comment of 23f32d7:
with a single ordered queue, jobs from one direction were always preferred
However, I don't see this imbalance (yet). I suggest having a chat these days to clarify this.
Considering a propagator stage, I would expect that a single queue will order states from both ends by their priority, which is exactly what I would expect. I think we dropped the stage, which could propagate from either side anyway, didn't we?
For CONNECT-like interfaces, I think we need a different approach, namely a job queue of (start, end) pairs, much like in the Connecting stage.
Hence we will have 3 implementations: for generator, propagator, and connect stages.
To simplify the distinction between those, I suggest having 3 FallbacksPrivate classes deriving from a common base class. When initializing the interface in initializeExternalInterfaces() (which would be implemented by the base class), the actually required class type could be instantiated. This separation would nicely separate data structures required for these variants and avoid the interface-type-based switches. What do you think?
Most review comments I directly implemented in some additional commits. Please have a look. I'm going to merge with #309 as well and check a disabled unit test there...
core/src/container.cpp
Outdated
| current.valid = (*current.state.stage)->pimpl()->canCompute(); | ||
|
|
||
| if(!current.valid) | ||
| advanceCurrentStateToNextChild(); |
There was a problem hiding this comment.
Validation here doesn't consider the PRUNED / FAILED status of the state(s). Connecting states can get reactivated if an opposite state occurs. However, here you immediately advance to the next child.
I don't believe the current approach scales to CONNECT-like interfaces, but it requires an extra pending list of state pairs (from start and end).
|
Superseded by #311. |
While the previous adaption makes moveit#287 fail, it still succeeded here. Now it fails here as well ;-)
2 participants
Here is my recent effort to get the tests proposed in #280 to pass by what I consider correct behavior for
Fallbacks.I also added further tests for various edge-cases.
Please have a look at the set of tests first.
It turned out to be quite a bit more work than I anticipated, especially with Pruning involved...
@j-kuehn you might want to have a look to see what I ended up with. :-)