Add support for feed-forward multi-track video requests. #1906

jrobble · 2025-05-14T01:58:51Z

Issues:

Support feeding foward all tracks to one sub-job (for Python video jobs only) #1907

Related PRs:

This change is

Add FEED_FORWARD_ALL_TRACKS property.

brosenberg42 · 2025-05-15T17:43:25Z

trunk/workflow-manager/src/main/resources/workflow-properties.json line 14 at r1 (raw file):

        "type": "BOOLEAN",
        "defaultValue": "false",
        "mediaTypes": ["VIDEO", "IMAGE", "AUDIO", "UNKNOWN"]

This should only be ["VIDEO"].

jrobble

Reviewable status: 0 of 21 files reviewed, 1 unresolved discussion (waiting on @brosenberg42 and @jrobble)

trunk/workflow-manager/src/main/resources/workflow-properties.json line 14 at r1 (raw file):

Previously, brosenberg42 wrote…

This should only be ["VIDEO"].

Done.

brosenberg42

@brosenberg42 reviewed 5 of 21 files at r1, 16 of 16 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @jrobble)

trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/service/TaskMergingManager.java line 87 at r2 (raw file):

            Message message, List<Track> feedForwardTracks) {
        // TODO: for now, assume all feed-forward tracks have the same history
        var feedForwardTrack = feedForwardTracks.get(0);

In a comment on DetectionRequest.java, I suggested changing the Optional<List<Track>> field to List<Track>. If we make that change, we might need to handle an empty list here.

trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/segmenting/VideoMediaSegmenter.java line 162 at r2 (raw file):

        }

        var AllVideoTracksRequestBuilder = AllVideoTracksRequest.newBuilder();

The A at the beginning of the variable name should be lower case.

trunk/detection/executor/cpp/batch/ProtobufRequestUtil.cpp line 118 at r2 (raw file):

                const auto& pb_ff_tracks = video_request.feed_forward_tracks();
                std::vector<MPFVideoTrack> ff_tracks;
                for (const auto& pb_ff_track : pb_ff_tracks) {

Please create a function named ConvertFeedForwardTrack and call it from here and from CreateVideoJob. It would convert the protobuf track to the C++ track similar to ConvertFeedForwardLocation

trunk/detection/executor/cpp/batch/CppComponentHandle.cpp line 63 at r2 (raw file):

    std::vector<MPFVideoTrack> CppComponentHandle::GetDetections(const MPFAllVideoTracksJob &job) {
        return component_->GetDetections(job);

We could throw an exception here instead of calling in to the component. It would allow us to remove the method from the C++ component interface.

trunk/detection/executor/cpp/batch/PythonComponentHandle.h line 60 at r2 (raw file):

        std::vector<MPFVideoTrack> GetDetections(const MPFVideoJob &job);

        std::vector<MPFVideoTrack> GetDetections(const MPFAllVideoTracksJob &job);

Please add a test to test_batch_executor.cpp for this method. There are unit tests for all of the other job types in that file.

trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/segmenting/DetectionRequest.java line 38 at r2 (raw file):

public record DetectionRequest(
        DetectionProtobuf.DetectionRequest protobuf,
        Optional<List<Track>> feedForwardTracks,

Normally, Optional is not used with collections because, in most cases, you can just use an empty collection. In this class, when there are no headers, we use Map.of(). We should handle feedForwardTracks similarly and use List.of() when there are no tracks.

trunk/detection/executor/cpp/batch/PythonComponentHandle.cpp line 573 at r2 (raw file):

                case VIDEO:
                    return component_.get_detections_from_video_method.has_value()
                           || component_.get_detections_from_all_video_tracks_method.has_value();

This is logically wrong. Supports() can return true even when calling the actual method will throw MPF_UNSUPPORTED_DATA_TYPE. One way to address the issue is to change the parameter to PythonComponentHandle::Supports and CppComponentHandle::Supports to be JobContext.

brosenberg42

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @jrobble)

trunk/detection/executor/cpp/batch/ProtobufRequestUtil.cpp line 140 at r2 (raw file):

                };
            }
            else {

Shouldn't this job type always have feed forward tracks? Is there a use case where it makes sense to create a MPFAllVideoTracksJob with no tracks?

jrobble · 2025-09-26T21:57:45Z

trunk/detection/executor/cpp/batch/PythonComponentHandle.cpp line 573 at r2 (raw file):

Previously, brosenberg42 wrote…

This is logically wrong. Supports() can return true even when calling the actual method will throw MPF_UNSUPPORTED_DATA_TYPE. One way to address the issue is to change the parameter to PythonComponentHandle::Supports and CppComponentHandle::Supports to be JobContext.

Supports() is used here in mpf_cli_job_runner.py:

class JobRunner(contextlib.AbstractContextManager):
    def __init__(self,
                 cmd_line_args: argparse.Namespace,
                 env_props: Mapping[str, str],
                 job_stdin: TextIO,
                 component,
                 descriptor):
        with contextlib.ExitStack() as exit_stack:
            self._output_dest = cmd_line_args.output
            exit_stack.push(self._output_dest)

            self._component_handle = component
            self._sdk_module = component.sdk_module

            self._mime_type = self._get_mime_type(
                cmd_line_args.media_type, cmd_line_args.media_path, cmd_line_args.media_metadata)

            self._media_type = self._get_media_type(
                cmd_line_args.media_type, self._mime_type)

            if not self._component_handle.supports(self._media_type):
                raise RuntimeError(
                    f'The component does not support {self._media_type.name.lower()} jobs.')

and here in detection executor main.cpp:

    while (true) {
        logger.Info("Waiting for next job.");
        auto job_context = job_receiver.GetJob();
        if (!component.Supports(job_context.job_type)) {
            job_receiver.ReportUnsupportedDataType(job_context);
            continue;
        }

I can see how to get the job context from the latter code, but not the former.

Once I have the job context what do I do with it?

jrobble

Reviewable status: 15 of 23 files reviewed, 8 unresolved discussions (waiting on @brosenberg42 and @jrobble)

trunk/detection/executor/cpp/batch/CppComponentHandle.cpp line 63 at r2 (raw file):

Previously, brosenberg42 wrote…

We could throw an exception here instead of calling in to the component. It would allow us to remove the method from the C++ component interface.

Done.

trunk/detection/executor/cpp/batch/ProtobufRequestUtil.cpp line 118 at r2 (raw file):

Previously, brosenberg42 wrote…

Please create a function named ConvertFeedForwardTrack and call it from here and from CreateVideoJob. It would convert the protobuf track to the C++ track similar to ConvertFeedForwardLocation

Done.

trunk/detection/executor/cpp/batch/ProtobufRequestUtil.cpp line 140 at r2 (raw file):

Previously, brosenberg42 wrote…

Shouldn't this job type always have feed forward tracks? Is there a use case where it makes sense to create a MPFAllVideoTracksJob with no tracks?

See this discussion: https://reviewable.io/reviews/openmpf/openmpf-python-component-sdk/91#-Oa0icuW7rcYdA2ZnFnn

trunk/detection/executor/cpp/batch/PythonComponentHandle.h line 60 at r2 (raw file):

Previously, brosenberg42 wrote…

Please add a test to test_batch_executor.cpp for this method. There are unit tests for all of the other job types in that file.

Done.

trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/segmenting/DetectionRequest.java line 38 at r2 (raw file):

Previously, brosenberg42 wrote…

Normally, Optional is not used with collections because, in most cases, you can just use an empty collection. In this class, when there are no headers, we use Map.of(). We should handle feedForwardTracks similarly and use List.of() when there are no tracks.

Done.

trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/segmenting/VideoMediaSegmenter.java line 162 at r2 (raw file):

Previously, brosenberg42 wrote…

The A at the beginning of the variable name should be lower case.

Done.

trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/service/TaskMergingManager.java line 87 at r2 (raw file):

Previously, brosenberg42 wrote…

In a comment on DetectionRequest.java, I suggested changing the Optional<List<Track>> field to List<Track>. If we make that change, we might need to handle an empty list here.

Done.

brosenberg42

@brosenberg42 reviewed 8 of 8 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jrobble)

trunk/detection/executor/cpp/batch/PythonComponentHandle.cpp line 573 at r2 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Supports() is used here in mpf_cli_job_runner.py:

class JobRunner(contextlib.AbstractContextManager):
    def __init__(self,
                 cmd_line_args: argparse.Namespace,
                 env_props: Mapping[str, str],
                 job_stdin: TextIO,
                 component,
                 descriptor):
        with contextlib.ExitStack() as exit_stack:
            self._output_dest = cmd_line_args.output
            exit_stack.push(self._output_dest)

            self._component_handle = component
            self._sdk_module = component.sdk_module

            self._mime_type = self._get_mime_type(
                cmd_line_args.media_type, cmd_line_args.media_path, cmd_line_args.media_metadata)

            self._media_type = self._get_media_type(
                cmd_line_args.media_type, self._mime_type)

            if not self._component_handle.supports(self._media_type):
                raise RuntimeError(
                    f'The component does not support {self._media_type.name.lower()} jobs.')

and here in detection executor main.cpp:

    while (true) {
        logger.Info("Waiting for next job.");
        auto job_context = job_receiver.GetJob();
        if (!component.Supports(job_context.job_type)) {
            job_receiver.ReportUnsupportedDataType(job_context);
            continue;
        }

I can see how to get the job context from the latter code, but not the former.

Once I have the job context what do I do with it?

CppComponentHandle is only used by the batch executor. It is not used in MpfCppSdkPythonBindings, so we don't need to worry about the cli runner.

jrobble · 2025-10-20T16:31:55Z

trunk/detection/executor/cpp/batch/PythonComponentHandle.cpp line 573 at r2 (raw file):

CppComponentHandle is only used by the batch executor

I'm confused. Both C++ and Python handles are used by mpf_cli_job_runner.py. From mpf_cli_executor_process.py:

    def _init_component(self, cmd_args) -> None:
        if self._component:
            return

        self._descriptor = self._load_descriptor(cmd_args.descriptor_file)
        lang = self._descriptor['sourceLanguage'].lower()
        if lang == 'c++':
            # Need to conditionally import because the C++ SDK won't be installed in Python
            # component images.
            import mpf_cpp_runner
            self._component = mpf_cpp_runner.CppComponentHandle(self._descriptor)
            self._exit_stack.enter_context(self._component)
        elif lang == 'python':
            # Need to conditionally import because the Python SDK won't be installed in C++
            # component images.
            import mpf_python_runner
            self._component = mpf_python_runner.PythonComponentHandle(self._descriptor)
        else:
            raise NotImplementedError(f'{lang} components are not supported.')

However, CppComponentHandle is also defined in CppComponentHandle.h. That is only used by the C++ detection executor.

CppComponentHandle [...] is not used in MpfCppSdkPythonBindings,

mpf_cpp_runner.py imports mpf_cpp_sdk, which has the bindings. That py file defines CppComponentHandle.

The bindings are used by the handle, not the other way around.

Overall, I'm lost. What are we trying to achieve?

JobContext is part of the C++ detection executor. It's never used by the CLI runner.

so we don't need to worry about the cli runner.

It sounds like you only care about the C++ and Python handles that are part of the C++ detection executor.

As I showed above in main.cpp, it seems easy enough to pass the context in to Supports here:

 if (!component.Supports(job_context.job_type)) {

I can get job_type from JobContext, but how does that help us?

brosenberg42

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jrobble)

trunk/detection/executor/cpp/batch/PythonComponentHandle.cpp line 573 at r2 (raw file):

Overall, I'm lost. What are we trying to achieve?

If you try to run an all tracks job on a component that only supports regular video jobs, Supports will return true. We didn't add a new data type for multi-track jobs, so we need more than just the data type to determine whether or not the job type is supported. mpf_cpp_runner.CppComponentHandle is a Python class. It is meant to be the CLI runner's version of the batch executor's CppComponentHandle.cpp.

This is how I thought you would implement Supports with the change:

bool Supports(const JobContext& job_context) const {
    if (std::holds_alternative<MPFVideoJob>(job_context.job)) {
        return component_.get_detections_from_video_method.has_value();
    }
    else if (std::holds_alternative<MPFAllVideoTracksJob>(job_context.job)) {
        return component_.get_detections_from_all_video_tracks_method.has_value();
    }
    else if (std::holds_alternative<MPFImageJob>(job_context.job)) {
        return component_.get_detections_from_image_method.has_value();
    }
    else if (std::holds_alternative<MPFAudioJob>(job_context.job)) {
        return component_.get_detections_from_audio_method.has_value();
    }
    else {
        return component_.get_detections_from_generic_method.has_value();
    }
}

jrobble · 2025-10-24T04:41:13Z

trunk/detection/executor/cpp/batch/PythonComponentHandle.cpp line 573 at r2 (raw file):

Previously, brosenberg42 wrote…

Overall, I'm lost. What are we trying to achieve?

If you try to run an all tracks job on a component that only supports regular video jobs, Supports will return true. We didn't add a new data type for multi-track jobs, so we need more than just the data type to determine whether or not the job type is supported. mpf_cpp_runner.CppComponentHandle is a Python class. It is meant to be the CLI runner's version of the batch executor's CppComponentHandle.cpp.

This is how I thought you would implement Supports with the change:
bool Supports(const JobContext& job_context) const {
    if (std::holds_alternative<MPFVideoJob>(job_context.job)) {
        return component_.get_detections_from_video_method.has_value();
    }
    else if (std::holds_alternative<MPFAllVideoTracksJob>(job_context.job)) {
        return component_.get_detections_from_all_video_tracks_method.has_value();
    }
    else if (std::holds_alternative<MPFImageJob>(job_context.job)) {
        return component_.get_detections_from_image_method.has_value();
    }
    else if (std::holds_alternative<MPFAudioJob>(job_context.job)) {
        return component_.get_detections_from_audio_method.has_value();
    }
    else {
        return component_.get_detections_from_generic_method.has_value();
    }
}

I implemented this and pushed the changes. There is an issue with this new code:

while (true) {
        logger.Info("Waiting for next job.");
        auto job_context = job_receiver.GetJob();
        if (!component.Supports(job_context)) {
            job_receiver.ReportUnsupportedDataType(job_context);
            continue;
        }

We can no longer assume that Supports() fails due to a data type issue, so we can't call ReportUnsupportedDataType(). I think we need to generate an exception inside of Supports() with the error message. If that's the case, should we rename the call to something like "validates"?

More generally, in the future, I don't see how it's possible to determine if C++ components support the all tracks method since the C++ components have to completely implement the API. We can't check if a method is missing from the implementation with *_method.has_value().

Is that a problem for C++ implementations or is the default behavior of generating this exception acceptable?

throw MPFDetectionException(MPFDetectionError::MPF_UNSUPPORTED_DATA_TYPE);

Looking at that, again it assumes an unsupported data type when it may just be that the all tracks method isn't implemented. Seems like we would need a more generic NOT_IMPLEMENTED exception.

brosenberg42

@brosenberg42 reviewed 6 of 6 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jrobble)

trunk/detection/executor/cpp/batch/PythonComponentHandle.cpp line 573 at r2 (raw file):

Previously, jrobble (Jeff Robble) wrote…

I implemented this and pushed the changes. There is an issue with this new code:
while (true) {
        logger.Info("Waiting for next job.");
        auto job_context = job_receiver.GetJob();
        if (!component.Supports(job_context)) {
            job_receiver.ReportUnsupportedDataType(job_context);
            continue;
        }
We can no longer assume that Supports() fails due to a data type issue, so we can't call ReportUnsupportedDataType(). I think we need to generate an exception inside of Supports() with the error message. If that's the case, should we rename the call to something like "validates"?

More generally, in the future, I don't see how it's possible to determine if C++ components support the all tracks method since the C++ components have to completely implement the API. We can't check if a method is missing from the implementation with *_method.has_value().

Is that a problem for C++ implementations or is the default behavior of generating this exception acceptable?
throw MPFDetectionException(MPFDetectionError::MPF_UNSUPPORTED_DATA_TYPE);
Looking at that, again it assumes an unsupported data type when it may just be that the all tracks method isn't implemented. Seems like we would need a more generic NOT_IMPLEMENTED exception.

I was considering the case to be an unsupported job type. I see your point about it not really being an unsupported data type. In the code you posted, you can see we handle unsupported data type differently than other errors. I originally thought we should handle the issue in the same way. I see that the error messages are going to be confusing. I think you can just revert that change, sorry for the confusion.

As far as future changes go, I think we should remove Supports and rely on GetDetections throwing MPFDetectionException(MPFDetectionError::MPF_UNSUPPORTED_DATA_TYPE) to get the existing behavior. It can use a different error code if the problem is that it does not support multi-track jobs. To do that, we would also need to refactor the component executor and update the C++ components

This reverts commit 28bf135.

jrobble added 3 commits May 13, 2025 16:04

Add support for feed-forward multi-track video requests.

ffb6e98

Update component executor for C++ and Python.

69f9825

Enable protobuf response.

3413018

Add FEED_FORWARD_ALL_TRACKS property.

jrobble changed the title ~~Feat/ff all tracks~~ Add support for feed-forward multi-track video requests. May 15, 2025

jrobble self-assigned this May 15, 2025

jrobble added this to OpenMPF: Development May 15, 2025

github-project-automation bot moved this to To do in OpenMPF: Development May 15, 2025

jrobble added 4 commits May 15, 2025 15:06

Fix bug when there are no tracks.

65497a5

Refactor "multi track" to "all tracks".

1bed982

Fix bug where all video tracks method wasn't being checked.

b4ccd0e

Merge branch 'develop' into feat/ff-all-tracks

99dc349

jrobble commented Sep 18, 2025

View reviewed changes

Merge branch 'develop' into feat/ff-all-tracks

7865984

jrobble mentioned this pull request Sep 19, 2025

Document FEED_FORWARD_ALL_TRACKS behavior. openmpf/openmpf.github.io#203

Open

jrobble requested a review from brosenberg42 September 19, 2025 21:38

brosenberg42 requested changes Sep 25, 2025

View reviewed changes

jrobble added 3 commits September 26, 2025 15:31

Refactor to ConvertFeedForwardTrack.

c12d1b1

Update MpfCppSdkPythonBindings.

c42b6d5

Don't use optional for feed-forward track collection.

02ff337

jrobble added 3 commits September 26, 2025 22:13

Update test_batch_executor.cpp.

943e5a7

Fix test.

ba12b7c

Update test.

78ea168

jrobble commented Sep 27, 2025

View reviewed changes

Fix tests.

6ece81b

brosenberg42 requested changes Sep 30, 2025

View reviewed changes

Merge branch 'develop' into feat/ff-all-tracks

dc7c5f5

brosenberg42 requested changes Oct 22, 2025

View reviewed changes

Update Supports() to use JobContext.

28bf135

brosenberg42 approved these changes Oct 24, 2025

View reviewed changes

Revert "Update Supports() to use JobContext."

7f7e76a

This reverts commit 28bf135.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add support for feed-forward multi-track video requests. #1906

Add support for feed-forward multi-track video requests. #1906

Uh oh!

jrobble commented May 14, 2025 •

edited

Loading

Uh oh!

brosenberg42 commented May 15, 2025

Uh oh!

jrobble left a comment

Uh oh!

brosenberg42 left a comment

Uh oh!

brosenberg42 left a comment

Uh oh!

jrobble commented Sep 26, 2025

Uh oh!

jrobble left a comment

Uh oh!

brosenberg42 left a comment

Uh oh!

jrobble commented Oct 20, 2025

Uh oh!

brosenberg42 left a comment

Uh oh!

jrobble commented Oct 24, 2025

Uh oh!

brosenberg42 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Add support for feed-forward multi-track video requests. #1906

Are you sure you want to change the base?

Add support for feed-forward multi-track video requests. #1906

Uh oh!

Conversation

jrobble commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brosenberg42 commented May 15, 2025

Uh oh!

jrobble left a comment

Choose a reason for hiding this comment

Uh oh!

brosenberg42 left a comment

Choose a reason for hiding this comment

Uh oh!

brosenberg42 left a comment

Choose a reason for hiding this comment

Uh oh!

jrobble commented Sep 26, 2025

Uh oh!

jrobble left a comment

Choose a reason for hiding this comment

Uh oh!

brosenberg42 left a comment

Choose a reason for hiding this comment

Uh oh!

jrobble commented Oct 20, 2025

Uh oh!

brosenberg42 left a comment

Choose a reason for hiding this comment

Uh oh!

jrobble commented Oct 24, 2025

Uh oh!

brosenberg42 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jrobble commented May 14, 2025 •

edited

Loading