Feat/wfm trigger #1644
Feat/wfm trigger #1644
Conversation
There was a problem hiding this comment.
Reviewed 2 of 41 files at r1, 39 of 39 files at r2, all commit messages.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @brosenberg42)
a discussion (no related file):
There are still four files with private AutoCloseable _closeable; in them.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/data/entities/persistent/MediaImpl.java line 32 at r2 (raw file):
import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.collect.HashMultimap;
Unused:
import com.google.common.collect.HashMultimap;
import com.google.common.collect.Multimap;
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/enums/MpfConstants.java line 105 at r2 (raw file):
FFPROBE_IGNORE_STDERR = "FFPROBE_IGNORE_STDERR", FFPROBE_STDERR_NUM_LINES = "FFPROBE_STDERR_NUM_LINES", TRIGGER = "TRIGGER";
Needs github.io documentation.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/segmenting/AudioMediaSegmenter.java line 44 at r2 (raw file):
import javax.inject.Inject; import java.util.ArrayList;
Unused.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/segmenting/DefaultMediaSegmenter.java line 42 at r2 (raw file):
import javax.inject.Inject; import java.util.ArrayList;
Unused.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/segmenting/ImageMediaSegmenter.java line 40 at r2 (raw file):
import javax.inject.Inject; import java.util.ArrayList;
Unused.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/segmenting/TriggerProcessor.java line 8 at r2 (raw file):
* 52.227-14, Alt. IV (DEC 2007). * * * * Copyright 2022 The MITRE Corporation. All Rights Reserved. *
Update copyright year in header blocks.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/camelOps/TestDetectionResponseProcessor.java line 73 at r2 (raw file):
import org.mitre.mpf.wfm.util.FrameTimeInfo; import org.mitre.mpf.wfm.util.IoUtils; import org.mitre.mpf.wfm.util.JsonUtils;
These aren't used:
import org.mitre.mpf.wfm.util.JsonUtils;
import org.mitre.mpf.wfm.util.ObjectMapperFactory;
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/camelOps/TestDetectionTaskSplitter.java line 969 at r2 (raw file):
// parent only
Please consider using the following:
// nothing run yet
job.setCurrentTaskIndex(0);
Assert.assertEquals(-1, detectionSplitter.getLastProcessedTaskIndex(job, parentMedia));
Assert.assertEquals(-1, detectionSplitter.getLastProcessedTaskIndex(job, childMedia1));
Assert.assertEquals(-1, detectionSplitter.getLastProcessedTaskIndex(job, childMedia2));
// after parent only extraction
job.setCurrentTaskIndex(1);
Assert.assertEquals(0, detectionSplitter.getLastProcessedTaskIndex(job, parentMedia));
Assert.assertEquals(-1, detectionSplitter.getLastProcessedTaskIndex(job, childMedia1));
Assert.assertEquals(-1, detectionSplitter.getLastProcessedTaskIndex(job, childMedia2));
// after parent only detection
job.setCurrentTaskIndex(2);
Assert.assertEquals(1, detectionSplitter.getLastProcessedTaskIndex(job, parentMedia));
Assert.assertEquals(-1, detectionSplitter.getLastProcessedTaskIndex(job, childMedia1));
Assert.assertEquals(-1, detectionSplitter.getLastProcessedTaskIndex(job, childMedia2));
// after child only detection
job.setCurrentTaskIndex(3);
Assert.assertEquals(1, detectionSplitter.getLastProcessedTaskIndex(job, parentMedia));
Assert.assertEquals(2, detectionSplitter.getLastProcessedTaskIndex(job, childMedia1));
Assert.assertEquals(2, detectionSplitter.getLastProcessedTaskIndex(job, childMedia2));
// after parent and child detection
job.setCurrentTaskIndex(4);
Assert.assertEquals(3, detectionSplitter.getLastProcessedTaskIndex(job, parentMedia));
Assert.assertEquals(3, detectionSplitter.getLastProcessedTaskIndex(job, childMedia1));
Assert.assertEquals(3, detectionSplitter.getLastProcessedTaskIndex(job, childMedia2));
Among other things, I added the last section for task 4.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 8 at r2 (raw file):
* 52.227-14, Alt. IV (DEC 2007). * * * * Copyright 2022 The MITRE Corporation. All Rights Reserved. *
Update copyright year in header blocks.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 125 at r2 (raw file):
.setLangTrigger("EN", 0) .getTriggeredTracks(); assertTrue(tracks.isEmpty());
No tracks are ever passed into the first task so how would we know if the trigger filter is applied to them? (For fun I added tracks with a task id of -1 and they did get filtered.)
More generally, it seems like this test is making sure that when a user configures a trigger on the first task it doesn't cause any problems (but it won't do anything either). Maybe we just need to rename the test. Consider triggerOnFirstTaskDoesNothingButIsAllowed.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 138 at r2 (raw file):
verify(_mockInProgressJobs, never()) .getTracksStream(anyLong(), anyLong(), anyInt(), anyInt());
Is InProgressJobs.getTracksStream() only called when triggers activate?
Can we instead check if TriggerProcessor.getTracks() is called? I think the latter is more intuitive. It's not clear that InProgressJobs.getTracksStream() has anything to do with triggers and seems like it could be called for other reasons.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 201 at r2 (raw file):
.getTriggeredTracks(); assertContainsTracks(tracks, 8, 9);
I added the last line here:
.setLangTrigger("ES", 1)
.addTrack("ES", 1)
.addTrack("ES", 1)
.addTrack(10, "EN", 0)
And then updated the final assert to:
assertContainsTracks(tracks, 8, 9, 10);
I did that just to make sure the untriggered tracks are accumulated across multiple previous stages. Please add that in if you think it's a good idea.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 206 at r2 (raw file):
@Test public void stopsCheckingPrevTasksWhenNoTriggerInMiddle() {
Please list the tracks and triggers in ascending task order for consistency and clarity.
What does the "no trigger in middle" refer to? Task 1?
Does the following have any effect?
.setTrigger("", 2)
The blank trigger is an all match:
if (triggerProperty == null || triggerProperty.isBlank()) {
return ALL_MATCH;
}
No tracks are added before task 2, so the trigger has nothing to filter out even if it wasn't an all match.
I rewrote the test as follows, based on what I think the intent of the test is:
@Test
public void stopsCheckingPrevTasksWhenNoTriggerInMiddle() {
var tracks = builder()
.addTrack("EN", 0)
.setTrigger("", 1)
.addTrack(11, "EN", 1)
.addTrack("ES", 1)
.setLangTrigger("EN", 2)
.setCurrentTask(2)
.getTriggeredTracks();
assertContainsTracks(tracks, 11);
}
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 229 at r2 (raw file):
@Test
Remove newline.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 251 at r2 (raw file):
private TestCaseBuilder initDoesNotPassForwardPreviouslyTriggeredTracks() {
Please list the tracks and triggers in ascending task order for consistency and clarity.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 268 at r2 (raw file):
private static void assertContainsTracks(Collection<Track> tracks, int... ids) {
This is somewhat confusing since we're treating start frames like ids. In reality, two separate tracks could start on the same frame. To be clear, we should call ids what they are: startFrames, or at least add a method comment explaining the relationship between start frames and "ids".
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 359 at r2 (raw file):
for (var triggerEntry : _triggers.entrySet()) { if (triggerEntry.getKey() == _currentTask) {
Why is this check here? Is this because all tracks from the prev. task flow into this one so there is no need to check the trigger for tracks from the prev. task?
There was a problem hiding this comment.
Reviewable status: 32 of 43 files reviewed, 18 unresolved discussions (waiting on @jrobble)
a discussion (no related file):
Previously, jrobble (Jeff Robble) wrote…
There are still four files with
private AutoCloseable _closeable;in them.
I was only able to find 3.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/data/entities/persistent/MediaImpl.java line 32 at r2 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Unused:
import com.google.common.collect.HashMultimap; import com.google.common.collect.Multimap;
Done.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/segmenting/AudioMediaSegmenter.java line 44 at r2 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Unused.
Done.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/segmenting/DefaultMediaSegmenter.java line 42 at r2 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Unused.
Done.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/segmenting/ImageMediaSegmenter.java line 40 at r2 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Unused.
Done.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/segmenting/TriggerProcessor.java line 8 at r2 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Update copyright year in header blocks.
Done.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/camelOps/TestDetectionResponseProcessor.java line 73 at r2 (raw file):
Previously, jrobble (Jeff Robble) wrote…
These aren't used:
import org.mitre.mpf.wfm.util.JsonUtils; import org.mitre.mpf.wfm.util.ObjectMapperFactory;
Done.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/camelOps/TestDetectionTaskSplitter.java line 969 at r2 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Please consider using the following:
// nothing run yet job.setCurrentTaskIndex(0); Assert.assertEquals(-1, detectionSplitter.getLastProcessedTaskIndex(job, parentMedia)); Assert.assertEquals(-1, detectionSplitter.getLastProcessedTaskIndex(job, childMedia1)); Assert.assertEquals(-1, detectionSplitter.getLastProcessedTaskIndex(job, childMedia2)); // after parent only extraction job.setCurrentTaskIndex(1); Assert.assertEquals(0, detectionSplitter.getLastProcessedTaskIndex(job, parentMedia)); Assert.assertEquals(-1, detectionSplitter.getLastProcessedTaskIndex(job, childMedia1)); Assert.assertEquals(-1, detectionSplitter.getLastProcessedTaskIndex(job, childMedia2)); // after parent only detection job.setCurrentTaskIndex(2); Assert.assertEquals(1, detectionSplitter.getLastProcessedTaskIndex(job, parentMedia)); Assert.assertEquals(-1, detectionSplitter.getLastProcessedTaskIndex(job, childMedia1)); Assert.assertEquals(-1, detectionSplitter.getLastProcessedTaskIndex(job, childMedia2)); // after child only detection job.setCurrentTaskIndex(3); Assert.assertEquals(1, detectionSplitter.getLastProcessedTaskIndex(job, parentMedia)); Assert.assertEquals(2, detectionSplitter.getLastProcessedTaskIndex(job, childMedia1)); Assert.assertEquals(2, detectionSplitter.getLastProcessedTaskIndex(job, childMedia2)); // after parent and child detection job.setCurrentTaskIndex(4); Assert.assertEquals(3, detectionSplitter.getLastProcessedTaskIndex(job, parentMedia)); Assert.assertEquals(3, detectionSplitter.getLastProcessedTaskIndex(job, childMedia1)); Assert.assertEquals(3, detectionSplitter.getLastProcessedTaskIndex(job, childMedia2));Among other things, I added the last section for task 4.
Done.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 8 at r2 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Update copyright year in header blocks.
Done.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 125 at r2 (raw file):
Previously, jrobble (Jeff Robble) wrote…
No tracks are ever passed into the first task so how would we know if the trigger filter is applied to them? (For fun I added tracks with a task id of
-1and they did get filtered.)More generally, it seems like this test is making sure that when a user configures a trigger on the first task it doesn't cause any problems (but it won't do anything either). Maybe we just need to rename the test. Consider
triggerOnFirstTaskDoesNothingButIsAllowed.
It is testing the case where there are no input tracks to test against the trigger.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 138 at r2 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Is
InProgressJobs.getTracksStream()only called when triggers activate?Can we instead check if
TriggerProcessor.getTracks()is called? I think the latter is more intuitive. It's not clear thatInProgressJobs.getTracksStream()has anything to do with triggers and seems like it could be called for other reasons.
As I mention in the method name, this is a test for handling jobs with no triggers at all. We can't check for TriggerProcessor.getTracks(). TriggerProcessor is the class we are testing. It is not a mock.
Since this is a unit test for TriggerProcessor it is safe to assume everything is related to triggers. The other uses for InProgressJobs.getTracksStream() are not relevent in a unit test.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 201 at r2 (raw file):
Previously, jrobble (Jeff Robble) wrote…
I added the last line here:
.setLangTrigger("ES", 1) .addTrack("ES", 1) .addTrack("ES", 1) .addTrack(10, "EN", 0)And then updated the final assert to:
assertContainsTracks(tracks, 8, 9, 10);I did that just to make sure the untriggered tracks are accumulated across multiple previous stages. Please add that in if you think it's a good idea.
Done.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 206 at r2 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Please list the tracks and triggers in ascending task order for consistency and clarity.
What does the "no trigger in middle" refer to? Task 1?
Does the following have any effect?
.setTrigger("", 2)The blank trigger is an all match:
if (triggerProperty == null || triggerProperty.isBlank()) { return ALL_MATCH; }No tracks are added before task 2, so the trigger has nothing to filter out even if it wasn't an all match.
I rewrote the test as follows, based on what I think the intent of the test is:
@Test public void stopsCheckingPrevTasksWhenNoTriggerInMiddle() { var tracks = builder() .addTrack("EN", 0) .setTrigger("", 1) .addTrack(11, "EN", 1) .addTrack("ES", 1) .setLangTrigger("EN", 2) .setCurrentTask(2) .getTriggeredTracks(); assertContainsTracks(tracks, 11); }
Middle refers to the median of the task indices. Your version of the test doesn't fail when TriggerProcessor gets unnecessary tracks out of Redis.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 229 at r2 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Remove newline.
Done.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 251 at r2 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Please list the tracks and triggers in ascending task order for consistency and clarity.
Done.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 268 at r2 (raw file):
Previously, jrobble (Jeff Robble) wrote…
This is somewhat confusing since we're treating start frames like ids. In reality, two separate tracks could start on the same frame. To be clear, we should call
idswhat they are:startFrames, or at least add a method comment explaining the relationship between start frames and "ids".
Done.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 359 at r2 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Why is this check here? Is this because all tracks from the prev. task flow into this one so there is no need to check the trigger for tracks from the prev. task?
The current task info is in DetectionContext.
There was a problem hiding this comment.
Reviewable status: 29 of 46 files reviewed, 18 unresolved discussions (waiting on @brosenberg42 and @jrobble)
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/enums/MpfConstants.java line 105 at r2 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Needs github.io documentation.
Where?
There was a problem hiding this comment.
Reviewed 10 of 11 files at r3, 7 of 8 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @brosenberg42)
a discussion (no related file):
Previously, brosenberg42 wrote…
I was only able to find 3.
I may have had an extra .orig file due to a merge. I don't see any in your most recent commit.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/camel/TrackOutputHelper.java line 83 at r5 (raw file):
boolean isSuppressed = isSuppressed(job, media, taskIdx); boolean isMergeTarget = isMergeTarget(job, media, taskIdx); boolean needToCheckTrackTriggers
Should we call the variable needToCheckSuppressedTriggers and the method needToCheckTriggers()? The variable has something to do with track suppression because of isSuppressed &&, while the method does not.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/camel/TrackOutputHelper.java line 170 at r5 (raw file):
private boolean needToCheckSuppressedTriggers(BatchJob job, Media media, int taskIdx) {
Should we just call this needToCheckTriggers()? I don't see what this method itself has to do with track suppression.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/camel/TrackOutputHelper.java line 223 at r5 (raw file):
} else { return taskCount - 2;
Do you do this because the only non-DETECTION algorithm is MARKUP?
I'm wondering if it would be better to iterate backwards until we find a DETECTION algorithm just in case we ever start supporting other non-DETECTION algorithms.
Minimally, leave a comment here explaining that the only non-DETECTION algorithm is MARKUP.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/data/RedisImpl.java line 121 at r5 (raw file):
public synchronized Optional<String> getTrackType( long jobId, long mediaId, int taskIndex, int actionIndex) { var tracks = getTrackListOps(jobId, mediaId, taskIndex, actionIndex);
This isn't a track at this point. Consider calling it trackListOps instead.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/enums/MpfConstants.java line 105 at r2 (raw file):
Previously, brosenberg42 wrote…
Where?
I think it's worth creating a Trigger Guide to explain the motivation, WFM properties, and provide some examples, similar to the Feed Forward Guide. It doesn't have to be long. I just think there is enough to talk about to warrant a separate guide, and the content doesn't fit anywhere else.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/segmenting/TriggerProcessor.java line 51 at r5 (raw file):
public class TriggerProcessor { public static final Predicate<Track> ALL_MATCH = t -> true;
I don't think this needs to be public.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 125 at r2 (raw file):
Previously, brosenberg42 wrote…
It is testing the case where there are no input tracks to test against the trigger.
Okay, I get it now.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 138 at r2 (raw file):
Previously, brosenberg42 wrote…
As I mention in the method name, this is a test for handling jobs with no triggers at all. We can't check for
TriggerProcessor.getTracks().TriggerProcessoris the class we are testing. It is not a mock.
Since this is a unit test for TriggerProcessor it is safe to assume everything is related to triggers. The other uses forInProgressJobs.getTracksStream()are not relevent in a unit test.
I was thinking that you can verify that TriggerProcessor.getTracks() is never called (you're currently verifying that InProgressJobs.getTracksStream() is never called), but as you said, it's not a mock so that's not an option without doing something like Mockito.spy().
Looking more closely at how these tests are structured, the only way InProgressJobs.getTracksStream() is called is through TriggerProcessor.getTracks(). My initial concern is that maybe sometime in the future we call InProgressJobs.getTracksStream() for some other reason not related to triggers. In that case, one of two things can happen. First, it won't matter since that code flow is not executed as part of this test. Second, it will be executed and this test will fail. If the latter, we can simply update this test.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 206 at r2 (raw file):
Middle refers to the median of the task indices.
Since the indices are 0, 1, 2, 3, and 4, I assume you're referring to index 2, which would mean the "no trigger" is the same as the ALL_MATCH trigger (both blank and null trigger property represent ALL_MATCH).
A line-ending comment could help here:
.setTrigger("", 2) // blank is the same as no triggerYour version of the test doesn't fail when TriggerProcessor gets unnecessary tracks out of Redis.
What are the "unnecessary tracks" in your test? Maybe we want to generate some tracks for task 1 here:
var tracks = builder()
.addTrack("EN", 1) // this
.addTrack("ES", 1) // this
.setTrigger("", 2)
.addTrack(11, "EN", 2)
.addTrack("ES", 2)
|
Previously, jrobble (Jeff Robble) wrote…
So to be clear, I'm not asking you to do anything here. |
There was a problem hiding this comment.
Reviewable status: 43 of 46 files reviewed, 7 unresolved discussions (waiting on @jrobble)
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/camel/TrackOutputHelper.java line 83 at r5 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Should we call the variable
needToCheckSuppressedTriggersand the methodneedToCheckTriggers()? The variable has something to do with track suppression because ofisSuppressed &&, while the method does not.
They are both related to track suppression. The first line of needToCheckSuppressedTriggers is checking if suppression is enabled. The lines directly surrounding the variable provide enough context and it is already a long variable name. The method is much further away, so more context is needed. Otherwise, someone looking at the method might not see what the method has to do with track suppression.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/camel/TrackOutputHelper.java line 223 at r5 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Do you do this because the only non-DETECTION algorithm is MARKUP?
I'm wondering if it would be better to iterate backwards until we find a DETECTION algorithm just in case we ever start supporting other non-DETECTION algorithms.
Minimally, leave a comment here explaining that the only non-DETECTION algorithm is MARKUP.
The methods in this class already have a lot parameters. To avoid adding even more, this method is called in a few places, so I think it is better to keep it simple. There are a lot of places throughout the code base that depend on there only being detection and markup algorithms. If that change is ever made, a developer would find all references to ActionType and update them accordingly.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/data/RedisImpl.java line 121 at r5 (raw file):
Previously, jrobble (Jeff Robble) wrote…
This isn't a track at this point. Consider calling it
trackListOpsinstead.
It has the interface of a list. There isn't a difference between tracks.index(0) and tracks.get(0).
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/segmenting/TriggerProcessor.java line 51 at r5 (raw file):
Previously, jrobble (Jeff Robble) wrote…
I don't think this needs to be public.
Done.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 206 at r2 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Middle refers to the median of the task indices.
Since the indices are 0, 1, 2, 3, and 4, I assume you're referring to index 2, which would mean the "no trigger" is the same as the ALL_MATCH trigger (both blank and null trigger property represent ALL_MATCH).
A line-ending comment could help here:
.setTrigger("", 2) // blank is the same as no triggerYour version of the test doesn't fail when TriggerProcessor gets unnecessary tracks out of Redis.
What are the "unnecessary tracks" in your test? Maybe we want to generate some tracks for task 1 here:
var tracks = builder() .addTrack("EN", 1) // this .addTrack("ES", 1) // this .setTrigger("", 2) .addTrack(11, "EN", 2) .addTrack("ES", 2)
The test name says "checking" and the test contains verify/never because it shouldn't even attempt to get the tracks. The exception generated explains why it is better not to include those tracks.
org.mockito.exceptions.misusing.UnnecessaryStubbingException:
Unnecessary stubbings detected.
Clean & maintainable test code requires zero unnecessary code.
Following stubbings are unnecessary (click to navigate to relevant line of code):
at org.mitre.mpf.wfm.segmenting.TestTriggerProcessor$TestCaseBuilder.getTriggeredTracks([TestTriggerProcessor.java:400]
Please remove unnecessary stubbings or use 'lenient' strictness. More info: javadoc for UnnecessaryStubbingException class.
I only used lenient() in certain cases where it was accessing local data from the passed in BatchJob. None of the stubbings for _mockInProgressJobs are lenient.
There was a problem hiding this comment.
Reviewed 2 of 3 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @brosenberg42)
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/camel/TrackOutputHelper.java line 83 at r5 (raw file):
Previously, brosenberg42 wrote…
They are both related to track suppression. The first line of
needToCheckSuppressedTriggersis checking if suppression is enabled. The lines directly surrounding the variable provide enough context and it is already a long variable name. The method is much further away, so more context is needed. Otherwise, someone looking at the method might not see what the method has to do with track suppression.
It makes more sense now while reading it the second time with your refactored logic.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/segmenting/TriggerProcessor.java line 124 at r7 (raw file):
Media media, DetectionContext context) { record TriggerEntry(int task, Predicate<Track> trigger) { }
Please call this taskIdx.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/segmenting/TestTriggerProcessor.java line 206 at r2 (raw file):
Previously, brosenberg42 wrote…
The test name says "checking" and the test contains verify/never because it shouldn't even attempt to get the tracks. The exception generated explains why it is better not to include those tracks.
org.mockito.exceptions.misusing.UnnecessaryStubbingException: Unnecessary stubbings detected. Clean & maintainable test code requires zero unnecessary code. Following stubbings are unnecessary (click to navigate to relevant line of code): at org.mitre.mpf.wfm.segmenting.TestTriggerProcessor$TestCaseBuilder.getTriggeredTracks([TestTriggerProcessor.java:400] Please remove unnecessary stubbings or use 'lenient' strictness. More info: javadoc for UnnecessaryStubbingException class.I only used
lenient()in certain cases where it was accessing local data from the passed inBatchJob. None of the stubbings for_mockInProgressJobsare lenient.
I understand that getTriggeredTracks() is used by many tests which have different flows so in some tests the stubs will be executed and others they won't.
In this particular case I proposed adding the tracks for the same reason they appear in other tests (basically, just fodder to make sure they don't make it to the end). It almost seems like we may be letting our desire to be strict get in the way of developing a more representative test.
However, I do understand that in this particular case verify/never should be sufficient without the extra tracks.
I think if boils down to the question, do we want to implement our stubs differently for the sake of adding these tracks? Hypothetically, we could implement/call a helper method like getTriggeredTracks() where we remove those stubs. That would enable us to stay strict. In reality, it would be cleaner just to make them lenient and prevent code duplication.
I'm just saying that overall it seems we're deciding not to add those tracks for the following reasons:
- Prevent making any changes to
getTriggeredTracks() - While they would make a more representative test, they are not absolutely required for what we are trying to test
I want to be clear that we're consciously making those choices. If so, I'm fine with that. Let me know if you think differently or I'm missing something. I'm trying to better understand our mindset regarding strictness.
There was a problem hiding this comment.
Reviewable status: 21 of 129 files reviewed, 2 unresolved discussions (waiting on @jrobble)
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/segmenting/TriggerProcessor.java line 124 at r7 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Please call this
taskIdx.
Done.
There was a problem hiding this comment.
Reviewed 85 of 107 files at r8, 22 of 22 files at r9, 3 of 3 files at r10, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @brosenberg42)
a discussion (no related file):
In ArtifactExtractionPolicy.java:
/** Default: Extract artifacts for tracks associated
with a "visual" detection type according to the
detection.artifact.extraction.policy.* settings. For example, this
would include faces and cars, but it would exclude speech,
motion and scene detection. */
VISUAL_TYPES_ONLY,
/** Default: Extract artifacts for tracks associated with any
* detection type according to the detection.artifact.extraction.policy.* settings. */
ALL_TYPES,"detection type" is used twice in the above comments. Change to "track type".
a discussion (no related file):
8e4237971957da59108bec54643c610eb66ad4cf48282e8c8c621ebbf" responded with a non-200 status code of 404. There are 1 attempts remaining and the next attempt will begin in 30000 ms.
There are 1 attempts remaining has an extra space after the 1.
a discussion (no related file):
Getting this:
2023-10-24 15:33:46,598 INFO [http-nio-8080-exec-7] o.m.m.w.d.InProgressBatchJobsService - [Job 56] Initializing batch job 56 which will run the "OCV TRITON YOLO OBJECT DETECTION (WITH MARKUP) PIPELINE" pipeline on the following media: "file:///opt/mpf/share/remote-media/dog-1080.jpg" (id=13)
2023-10-24 15:33:46,605 INFO [http-nio-8080-exec-7] o.m.m.w.d.InProgressBatchJobsService - [Job 56] Setting status of job 56 to IN_PROGRESS
2023-10-24 15:33:46,610 INFO [http-nio-8080-exec-7] o.m.m.w.b.i.JobRequestServiceImpl - [Job 56] Skipping job processing because compatible job found in TiesDb.
2023-10-24 15:33:46,613 INFO [Camel (camelContext) thread #7 - JmsConsumer[MPF.JOB_ROUTER]] o.m.m.w.d.InProgressBatchJobsService - [Job 56] Changing job 56's current task index from -1 to 0
2023-10-24 15:33:46,613 INFO [Camel (camelContext) thread #7 - JmsConsumer[MPF.JOB_ROUTER]] o.m.m.w.c.DefaultTaskSplitter - [Job 56] Task 1/2 - Operation: DETECTION - ActionType: DETECTION.
2023-10-24 15:33:46,615 INFO [Camel (camelContext) thread #7 - JmsConsumer[MPF.JOB_ROUTER]] o.m.m.w.c.DefaultTaskSplitter - [Job 56] DefaultTaskSplitter produced 1 work units with correlation id '56:16a9a502-aa55-4e3e-9925-c02c86f9a21f'.
2023-10-24 15:33:47,225 ERROR [Camel (camelContext) thread #46 - JmsConsumer[MPF.COMPLETED_DETECTIONS]] o.a.c.p.DefaultErrorHandler - Failed delivery for (MessageId: ID-23106830b23d-1698160439551-0-409 on ExchangeId: ID-23106830b23d-1698160439551-0-408). Exhausted after delivery attempt: 1 caught: java.lang.NumberFormatException: Cannot parse null string
Message History
---------------------------------------------------------------------------------------------------------------------------------------
RouteId ProcessorId Processor Elapsed (ms)
[Detection Response] [Detection Response] [jms://MPF.COMPLETED_DETECTIONS ] [ 10]
[Detection Response] [setExchangePattern] [setExchangePattern[InOnly] ] [ 0]
[Detection Response] [unmarshal1 ] [unmarshal[org.apache.camel.model.DataFormatDefinition@1eb0a4e3] ] [ 1]
[Detection Response] [process2 ] [ref:detectionResponseProcessor ] [ 3]
[Detection Response] [choice1 ] [when[{header{header(Unsolicited)} == true}]choice[] ] [ 0]
[Detection Response] [aggregate1 ] [aggregate[header(CorrelationId)] ] [ 0]
[Detection Response] [process3 ] [ref:trackMergingProcessor ] [ 1]
[Detection Response] [process4 ] [ref:MovingTrackLabelProcessor ] [ 0]
[Detection Response] [process5 ] [ref:detectionTransformationProcessor ] [ 3]
Stacktrace
---------------------------------------------------------------------------------------------------------------------------------------
java.lang.NumberFormatException: Cannot parse null string
at java.lang.Integer.parseInt(Integer.java:630) ~[?:?]
at java.lang.Integer.parseInt(Integer.java:786) ~[?:?]
at org.mitre.mpf.wfm.camel.operations.detection.transformation.DetectionTransformationProcessor.wfmProcess(DetectionTransformationProcessor.java:102) ~[classes/:?]
at org.mitre.mpf.wfm.camel.WfmProcessor.lambda$process$0(WfmProcessor.java:59) ~[classes/:?]
at org.mitre.mpf.mvc.util.MdcUtil.lambda$asSupplier$0(MdcUtil.java:120) ~[classes/:?]
at org.mitre.mpf.mvc.util.MdcUtil.job(MdcUtil.java:102) ~[classes/:?]
at org.mitre.mpf.mvc.util.MdcUtil.job(MdcUtil.java:114) ~[classes/:?]
at org.mitre.mpf.wfm.camel.WfmProcessor.process(WfmProcessor.java:59) ~[classes/:?]
at org.mitre.mpf.wfm.camel.WfmProcessor$$FastClassBySpringCGLIB$$dbb36fb4.invoke(<generated>) ~[classes/:?]
at org.springframework.cglib.proxy.MethodProxy.invoke(MethodProxy.java:218) ~[spring-core-5.3.24.jar:5.3.24]
For this job:
{
"jobProperties": {
"TIES_DB_URL" : "https://somehost"
},
"media": [
{
"mediaUri": "file:///opt/mpf/share/remote-media/dog-1080.jpg",
"metadata" : {
"MEDIA_HASH": "7783d66ccd942eacb982e7eeb645f5a446621d1e53745f42f894a64bf1a970e4",
"MIME_TYPE": "image/jpeg"
}
}
],
"pipelineName": "OCV TRITON YOLO OBJECT DETECTION (WITH MARKUP) PIPELINE"
}This is the line that causes the NPE:
int frameWidth = Integer.parseInt(media.getMetadata().get("FRAME_WIDTH"));
You stated that:
It's a merge issue from the camel refactor
BeginTaskProcessor doesn't copy the
JOB_COMPLETEheader
a discussion (no related file):
When a TRIGGER is specified for an action, check if a valid FEED_FORWARD_TYPE is also specified. If not, don't create the job. I think we should do this as part of the job pre-validation, similar to what we do when S3_RESULTS_BUCKET is specified but S3_ACCESS_KEY or S3_SECRET_KEY is missing.
trunk/mpf-system-tests/src/test/java/org/mitre/mpf/mst/TestSystemOnDiff.java line 68 at r10 (raw file):
Change:
Output object did not contain expected detection type:
to:
Output object did not contain expected track type:
There are multiple lines in this file like that. Please change them all.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/camel/JobCompleteProcessorImpl.java line 584 at r10 (raw file):
} } if (taskIndex == lastDetectionTaskIdx) {
We discussed handling a dynamic speech pipeline that doesn't end in keyword tagging. For example, let's say we did:
SPEAKER AND LANGUAGE DETECTION --> SPHINX STT (eng) --> WHISPER STT (spa)
If no spa tracks are generated, the final track count would be 0. This would not be desirable if there are eng tracks. It would be better if we counted both the eng and spa tracks.
More generally, I think the final track count should count all of the tracks from the final stage, as well as untriggered tracks from previous stages.
We talked about a situation where the first stage detects another language, like rus, for which we don't have a downstream STT stage. I think we want to count those tracks as well. If a user really doesn't care about those tracks, we would end up implementing a feature in the upstream stage to drop them, like the whitelists we have now.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/camel/TrackOutputHelper.java line 136 at r10 (raw file):
.map(a -> _aggregateJobPropertiesUtil.getValue(MpfConstants.TRIGGER, job, media, a)) .anyMatch(t -> t == null || t.isBlank()); // A task later in the pipeline does not have a trigger set. When a trigger
Good comment, but I think it needs to be slightly adjusted.
It says "does not have a trigger set" but is above !futureTaskMissingTrigger;, which would mean a trigger is set.
Should we move it above the boolean futureTaskMissingTrigger line and change it to say "Check if a task later in the pipeline does not have a trigger set" ?
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/camel/operations/detection/DetectionResponseProcessor.java line 101 at r10 (raw file):
if (totalResponses == 0) { String mediaLabel = getBasicMediaLabel(detectionResponse); log.warn("Response received, but no tracks were found for {}.", mediaLabel);
Let's also print out the action name since some actions may return tracks but others may not.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/camel/operations/detection/trackmerging/TrackMergingProcessor.java line 316 at r10 (raw file):
Change comment to:
This method assumes that we've already checked that both tracks have the same track type.
isEligibleForFixup() does not exist anymore.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/service/TaskMergingManager.java line 50 at r10 (raw file):
@Component public class TaskMergingManager {
Nice job breaking this logic out. Seems cleaner.
trunk/workflow-manager/src/main/resources/workflow-properties.json line 39 at r10 (raw file):
Change:
Those tracks will inherit the detection type and algorithm from the preceding task's tracks.
to:
Those tracks will inherit the track type and algorithm from the preceding task's tracks.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/camel/operations/detection/TestMovingTrackLabelProcessor.java line 279 at r10 (raw file):
0, //startOffsetTimeInclusive 1, //endOffsetTimeInclusive "",
Needs line ending comment.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/camel/operations/markup/TestMarkupSplitter.java line 66 at r10 (raw file):
5000, // startOffsetTime 10000, // endOffsetTime "",
Needs line ending comment.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/service/TestTaskMergingManager.java line 183 at r10 (raw file):
@Test public void testNonFeedForwardTaskMerging() {
Should we test task merging with feed forward?
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/service/TestTiesDbService.java line 214 at r10 (raw file):
.thenReturn("http://tiesdbForParent"); when(_mockAggregateJobPropertiesUtil.isOutputLastTaskOnly(_tiesDbParentMedia, _job)) .thenReturn(true);
Please leave a comment explaining how this will suppress the tracks from the first action. The other tasks in the pipeline are involved in a merge from the last task and are not suppressed. The first PARENT_SUPPRESSED task is the only one not involved in that chain.
There was a problem hiding this comment.
Reviewable status: 96 of 157 files reviewed, 13 unresolved discussions (waiting on @brosenberg42 and @jrobble)
a discussion (no related file):
Previously, jrobble (Jeff Robble) wrote…
In
ArtifactExtractionPolicy.java:/** Default: Extract artifacts for tracks associated with a "visual" detection type according to the detection.artifact.extraction.policy.* settings. For example, this would include faces and cars, but it would exclude speech, motion and scene detection. */ VISUAL_TYPES_ONLY, /** Default: Extract artifacts for tracks associated with any * detection type according to the detection.artifact.extraction.policy.* settings. */ ALL_TYPES,"detection type" is used twice in the above comments. Change to "track type".
Done.
a discussion (no related file):
Previously, jrobble (Jeff Robble) wrote…
8e4237971957da59108bec54643c610eb66ad4cf48282e8c8c621ebbf" responded with a non-200 status code of 404. There are 1 attempts remaining and the next attempt will begin in 30000 ms.
There are 1 attempts remaininghas an extra space after the1.
Done.
a discussion (no related file):
Previously, jrobble (Jeff Robble) wrote…
Getting this:
2023-10-24 15:33:46,598 INFO [http-nio-8080-exec-7] o.m.m.w.d.InProgressBatchJobsService - [Job 56] Initializing batch job 56 which will run the "OCV TRITON YOLO OBJECT DETECTION (WITH MARKUP) PIPELINE" pipeline on the following media: "file:///opt/mpf/share/remote-media/dog-1080.jpg" (id=13) 2023-10-24 15:33:46,605 INFO [http-nio-8080-exec-7] o.m.m.w.d.InProgressBatchJobsService - [Job 56] Setting status of job 56 to IN_PROGRESS 2023-10-24 15:33:46,610 INFO [http-nio-8080-exec-7] o.m.m.w.b.i.JobRequestServiceImpl - [Job 56] Skipping job processing because compatible job found in TiesDb. 2023-10-24 15:33:46,613 INFO [Camel (camelContext) thread #7 - JmsConsumer[MPF.JOB_ROUTER]] o.m.m.w.d.InProgressBatchJobsService - [Job 56] Changing job 56's current task index from -1 to 0 2023-10-24 15:33:46,613 INFO [Camel (camelContext) thread #7 - JmsConsumer[MPF.JOB_ROUTER]] o.m.m.w.c.DefaultTaskSplitter - [Job 56] Task 1/2 - Operation: DETECTION - ActionType: DETECTION. 2023-10-24 15:33:46,615 INFO [Camel (camelContext) thread #7 - JmsConsumer[MPF.JOB_ROUTER]] o.m.m.w.c.DefaultTaskSplitter - [Job 56] DefaultTaskSplitter produced 1 work units with correlation id '56:16a9a502-aa55-4e3e-9925-c02c86f9a21f'. 2023-10-24 15:33:47,225 ERROR [Camel (camelContext) thread #46 - JmsConsumer[MPF.COMPLETED_DETECTIONS]] o.a.c.p.DefaultErrorHandler - Failed delivery for (MessageId: ID-23106830b23d-1698160439551-0-409 on ExchangeId: ID-23106830b23d-1698160439551-0-408). Exhausted after delivery attempt: 1 caught: java.lang.NumberFormatException: Cannot parse null string Message History --------------------------------------------------------------------------------------------------------------------------------------- RouteId ProcessorId Processor Elapsed (ms) [Detection Response] [Detection Response] [jms://MPF.COMPLETED_DETECTIONS ] [ 10] [Detection Response] [setExchangePattern] [setExchangePattern[InOnly] ] [ 0] [Detection Response] [unmarshal1 ] [unmarshal[org.apache.camel.model.DataFormatDefinition@1eb0a4e3] ] [ 1] [Detection Response] [process2 ] [ref:detectionResponseProcessor ] [ 3] [Detection Response] [choice1 ] [when[{header{header(Unsolicited)} == true}]choice[] ] [ 0] [Detection Response] [aggregate1 ] [aggregate[header(CorrelationId)] ] [ 0] [Detection Response] [process3 ] [ref:trackMergingProcessor ] [ 1] [Detection Response] [process4 ] [ref:MovingTrackLabelProcessor ] [ 0] [Detection Response] [process5 ] [ref:detectionTransformationProcessor ] [ 3] Stacktrace --------------------------------------------------------------------------------------------------------------------------------------- java.lang.NumberFormatException: Cannot parse null string at java.lang.Integer.parseInt(Integer.java:630) ~[?:?] at java.lang.Integer.parseInt(Integer.java:786) ~[?:?] at org.mitre.mpf.wfm.camel.operations.detection.transformation.DetectionTransformationProcessor.wfmProcess(DetectionTransformationProcessor.java:102) ~[classes/:?] at org.mitre.mpf.wfm.camel.WfmProcessor.lambda$process$0(WfmProcessor.java:59) ~[classes/:?] at org.mitre.mpf.mvc.util.MdcUtil.lambda$asSupplier$0(MdcUtil.java:120) ~[classes/:?] at org.mitre.mpf.mvc.util.MdcUtil.job(MdcUtil.java:102) ~[classes/:?] at org.mitre.mpf.mvc.util.MdcUtil.job(MdcUtil.java:114) ~[classes/:?] at org.mitre.mpf.wfm.camel.WfmProcessor.process(WfmProcessor.java:59) ~[classes/:?] at org.mitre.mpf.wfm.camel.WfmProcessor$$FastClassBySpringCGLIB$$dbb36fb4.invoke(<generated>) ~[classes/:?] at org.springframework.cglib.proxy.MethodProxy.invoke(MethodProxy.java:218) ~[spring-core-5.3.24.jar:5.3.24]For this job:
{ "jobProperties": { "TIES_DB_URL" : "https://somehost" }, "media": [ { "mediaUri": "file:///opt/mpf/share/remote-media/dog-1080.jpg", "metadata" : { "MEDIA_HASH": "7783d66ccd942eacb982e7eeb645f5a446621d1e53745f42f894a64bf1a970e4", "MIME_TYPE": "image/jpeg" } } ], "pipelineName": "OCV TRITON YOLO OBJECT DETECTION (WITH MARKUP) PIPELINE" }This is the line that causes the NPE:
int frameWidth = Integer.parseInt(media.getMetadata().get("FRAME_WIDTH"));You stated that:
It's a merge issue from the camel refactor
BeginTaskProcessor doesn't copy the
JOB_COMPLETEheader
Done.
a discussion (no related file):
Previously, jrobble (Jeff Robble) wrote…
When a
TRIGGERis specified for an action, check if a validFEED_FORWARD_TYPEis also specified. If not, don't create the job. I think we should do this as part of the job pre-validation, similar to what we do whenS3_RESULTS_BUCKETis specified butS3_ACCESS_KEYorS3_SECRET_KEYis missing.
Done.
trunk/mpf-system-tests/src/test/java/org/mitre/mpf/mst/TestSystemOnDiff.java line 68 at r10 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Change:
Output object did not contain expected detection type:
to:
Output object did not contain expected track type:
There are multiple lines in this file like that. Please change them all.
Done.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/camel/JobCompleteProcessorImpl.java line 584 at r10 (raw file):
Previously, jrobble (Jeff Robble) wrote…
We discussed handling a dynamic speech pipeline that doesn't end in keyword tagging. For example, let's say we did:
SPEAKER AND LANGUAGE DETECTION --> SPHINX STT (eng) --> WHISPER STT (spa)If no spa tracks are generated, the final track count would be 0. This would not be desirable if there are eng tracks. It would be better if we counted both the eng and spa tracks.
More generally, I think the final track count should count all of the tracks from the final stage, as well as untriggered tracks from previous stages.
We talked about a situation where the first stage detects another language, like rus, for which we don't have a downstream STT stage. I think we want to count those tracks as well. If a user really doesn't care about those tracks, we would end up implementing a feature in the upstream stage to drop them, like the whitelists we have now.
Done.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/camel/TrackOutputHelper.java line 136 at r10 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Good comment, but I think it needs to be slightly adjusted.
It says "does not have a trigger set" but is above
!futureTaskMissingTrigger;, which would mean a trigger is set.Should we move it above the
boolean futureTaskMissingTriggerline and change it to say "Check if a task later in the pipeline does not have a trigger set" ?
Done.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/camel/operations/detection/DetectionResponseProcessor.java line 101 at r10 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Let's also print out the action name since some actions may return tracks but others may not.
getBasicMediaLabel already includes it.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/camel/operations/detection/trackmerging/TrackMergingProcessor.java line 316 at r10 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Change comment to:
This method assumes that we've already checked that both tracks have the same track type.
isEligibleForFixup()does not exist anymore.
Done.
trunk/workflow-manager/src/main/resources/workflow-properties.json line 39 at r10 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Change:
Those tracks will inherit the detection type and algorithm from the preceding task's tracks.
to:
Those tracks will inherit the track type and algorithm from the preceding task's tracks.
Done.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/camel/operations/detection/TestMovingTrackLabelProcessor.java line 279 at r10 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Needs line ending comment.
Done.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/camel/operations/markup/TestMarkupSplitter.java line 66 at r10 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Needs line ending comment.
Done.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/service/TestTiesDbService.java line 214 at r10 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Please leave a comment explaining how this will suppress the tracks from the first action. The other tasks in the pipeline are involved in a merge from the last task and are not suppressed. The first
PARENT_SUPPRESSEDtask is the only one not involved in that chain.
Done.
There was a problem hiding this comment.
Reviewed 61 of 61 files at r11, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @brosenberg42)
trunk/mpf-system-tests/src/test/java/org/mitre/mpf/mst/TestSystemOnDiff.java line 309 at r8 (raw file):
outputMedia.getTrackTypes().get(JsonActionOutputObject.TRACKS_MERGED_TYPE); assertEquals(1, mergedTracksOutput.size()); assertEquals("Tracks merged for task other than TESSERACT OCR",
I ran some examples. The only actions we're listing in TRACKS MERGED are those that are in the middle of a merge chain. We're no longer listing the action at the beginning of the chain.
I believe the reason is, as we discussed, that showing the same action in the TRACKS MERGED section and listing the actual tracks for that action would be confusing.
trunk/mpf-system-tests/src/test/resources/output/face/runDetectMarkupMultipleMediaTypes.json line 10 at r11 (raw file):
"details": [ { "source": "WORKFLOW_MANAGER",
Did you intend to remove this? "source" is still a field in JsonIssueDetails.java.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/camel/JobCompleteProcessorImpl.java line 584 at r10 (raw file):
Previously, brosenberg42 wrote…
Done.
I tested this with a dynamic speech pipeline with OUTPUT_LAST_TASK_ONLY=false and in the JSON it lists the downstream STT tracks and only the language tracks from the first stage that aren't triggered downstream. The track count only counts the number of tracks in the JSON, which is the downstream STT tracks and the untriggered upstream language track.
I believe this is the correct behavior based on what we discussed.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/camel/TrackOutputHelper.java line 159 at r11 (raw file):
private String getMergedAction(Track track, BatchJob job) { int actionIndex; if (track.getMergedTaskIndex() == track.getTaskIndex()) {
This first condition will only happen when task merging is disabled, right? I guess someone could try to set OUTPUT_MERGE_WITH_PREVIOUS_TASK=TRUE on the first task in a pipeline.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/camel/operations/detection/trackmerging/TrackMergingProcessor.java line 269 at r11 (raw file):
} /** Combines two tracks. This is a destructive method. The contents of track1 reflect the merged track. */
This comment is not accurate, so please remove it.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/service/TestTiesDbBeforeJobCheckService.java line 897 at r11 (raw file):
var action1 = JsonActionOutputObject.factory( "source1", "algo1", ImmutableSortedSet.of(track1));
There are a few instances of "source1" and "source2" in here you should change to "action1" and "action2".
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/service/TestTiesDbBeforeJobCheckService.java line 1096 at r11 (raw file):
List.of(detection)); var action = new JsonActionOutputObject("source", "algo");
Change "source" to "action".
There was a problem hiding this comment.
Reviewable status: 153 of 157 files reviewed, 5 unresolved discussions (waiting on @jrobble)
trunk/mpf-system-tests/src/test/resources/output/face/runDetectMarkupMultipleMediaTypes.json line 10 at r11 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Did you intend to remove this? "source" is still a field in
JsonIssueDetails.java.
Done.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/camel/TrackOutputHelper.java line 159 at r11 (raw file):
Previously, jrobble (Jeff Robble) wrote…
This first condition will only happen when task merging is disabled, right? I guess someone could try to set
OUTPUT_MERGE_WITH_PREVIOUS_TASK=TRUEon the first task in a pipeline.
That is correct.
trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/camel/operations/detection/trackmerging/TrackMergingProcessor.java line 269 at r11 (raw file):
Previously, jrobble (Jeff Robble) wrote…
This comment is not accurate, so please remove it.
Done.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/service/TestTiesDbBeforeJobCheckService.java line 897 at r11 (raw file):
Previously, jrobble (Jeff Robble) wrote…
There are a few instances of "source1" and "source2" in here you should change to "action1" and "action2".
Done.
trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/service/TestTiesDbBeforeJobCheckService.java line 1096 at r11 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Change "source" to "action".
Done.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r12, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @brosenberg42)
Issues:
Related PRs:
This change is