Add OrToolsSubjectComponent

[brosenberg42]

Issues:

• Add initial subject tracking implementation openmpf#1832

Related PRs:

• Feat/subject tracking openmpf#1833
• Add OrToolsSubjectComponent #378
• Initial subject tracking implementation openmpf-python-component-sdk#84
• Feat/subject tracking openmpf-docker#219
• Add protobuf build commands openmpf.github.io#191

---

This change is 

[jrobble]

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @brosenberg42)

---

python/OrToolsSubjectComponent/Dockerfile line 33 at r1 (raw file):

RUN pip install ortools

RUN --mount=target=.,readwrite install-component.sh

Eventually we'll need to add unit tests to this.

---

python/OrToolsSubjectComponent/Dockerfile line 39 at r1 (raw file):

COPY --from=build_component $COMPONENT_VIRTUALENV $COMPONENT_VIRTUALENV

COPY --from=build_component $PLUGINS_DIR/OrToolsSubjectComponent $PLUGINS_DIR/OrToolsSubjectComponent

Please add labels like these:

LABEL org.label-schema.license="Apache 2.0" \
      org.label-schema.name="OpenMPF Azure Read Text Detection" \
      org.label-schema.schema-version="1.0" \
      org.label-schema.url="https://openmpf.github.io" \
      org.label-schema.vcs-url="https://github.com/openmpf/openmpf-components" \
      org.label-schema.vendor="MITRE"

I know this is a first cut so things like this are missing. Before we land this, please add a README.md, LICENSE, and NOTICE.

---

python/OrToolsSubjectComponent/pyproject.toml line 27 at r1 (raw file):

#############################################################################

[build-system]

Do we not need setup.cfg anymore?

---

python/OrToolsSubjectComponent/or_tools_component/__init__.py line 1 at r1 (raw file):

#############################################################################

For detection components the __init__.py uses from. For example:

from .acs_read_detection_component import AcsReadDetectionComponent

Looking around on forums, the consensus seems to be that in most cases __init_py should have little in it, unless you have package-level instantiation code, or are writing a library that's so small/basic that you don't need to break it out into modules.

What's your motivation? I'm assuming it's the latter.

If I remember correctly, I think you mentioned that it's basically unnecessary to have an __init__.py that just imports one thing.

Making a note here for myself that a similar change was made to__init.py__ in the example PythonSubjectComponent and subject/api/mpf_subject_api/__init__.py.

If this is the direction we're going, should we make these changes to the detection components, or is it such a minor / optional thing it's unnecessary?

---

python/OrToolsSubjectComponent/or_tools_component/__init__.py line 70 at r1 (raw file):

        relationships = get_relationship_dict(assignments, non_subject_entities, media_id)
        num_entities = sum(len(es) for es in all_entities.values())

What does es stand for?

---

python/OrToolsSubjectComponent/or_tools_component/__init__.py line 71 at r1 (raw file):
Let's log:

Finished running subject tracking job. Found X subjects and Y non-subject entities.

---

python/OrToolsSubjectComponent/or_tools_component/__init__.py line 178 at r1 (raw file):

class Assignment:
    @classmethod
    def assign_tracks(

I'm assuming this method takes the most amount of time. Please add a log line stating how much time it took to run. Add other timing log lines that you think might be helpful.

---

python/OrToolsSubjectComponent/or_tools_component/__init__.py line 196 at r1 (raw file):

                    assignment_var = AssignmentVar(model_var, ft, pt, score)
                    face_to_person_assignments.append(assignment_var)
                    model_vars_grouped_by_person[id(pt)].append(model_var)

Interesting use of memory address for the unique id.

---

python/OrToolsSubjectComponent/or_tools_component/__init__.py line 202 at r1 (raw file):

                yield TrackAssignment.face_only(ft)

        for person_face_assignment_group in model_vars_grouped_by_person.values():

Add a comment here. For example:

"Ensure that each person track is only assigned to one face track."

---

python/OrToolsSubjectComponent/or_tools_component/__init__.py line 220 at r1 (raw file):

    @staticmethod
    def _add_overlap_constraints(

Please add a method comment explaining what this does. For example:

"Ensure that only person tracks with no frames in common are assigned to the same face track."

---

python/OrToolsSubjectComponent/or_tools_component/__init__.py line 392 at r1 (raw file):

# TODO include in output
def get_subject_to_subject_relationships(
            assignments: Sequence[TrackAssignment], media_id: mpf_sub.MediaId):

Missing -> type annotation for the return value.

---

python/OrToolsSubjectComponent/or_tools_component/config.json line 10 at r1 (raw file):

    "entity_mappings": [
        {
            "entity": "vehicle",

I think this should be entity_type. That's what you call it in the code.

[brosenberg42]

Reviewable status: 4 of 6 files reviewed, 11 unresolved discussions (waiting on @jrobble)

---

python/OrToolsSubjectComponent/pyproject.toml line 27 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Do we not need setup.cfg anymore?

No, not unless we are doing something complicated with setuptools.

---

python/OrToolsSubjectComponent/or_tools_component/__init__.py line 1 at r1 (raw file):

It has been a while since our Python SDK was initially released and things change. For a while __init__.py was just used as a marker file. It is more common now to include code in __init__.py.

are writing a library that's so small/basic that you don't need to break it out into modules.

This approach doesn't prevent you from using multiple Python files or using sub-packages. You can both have code in __init__.py and other Python files.

If this is the direction we're going, should we make these changes to the detection components, or is it such a minor / optional thing it's unnecessary?

There is no need to go back and update the other components. It is very minor. It also should be the component developer that decides how best to break up the code. Sometimes one file is sufficient, sometimes it isn't. It would be like saying to be consistent all Python components must use exactly 2 .py files

---

python/OrToolsSubjectComponent/or_tools_component/__init__.py line 70 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

What does es stand for?

When you use lambdas or other constructs where the scope of a variable is a single line, it is common to use single letter variable names and to pluralize them with s. e is for entity, but the entities are in a nested collection. es is used to make it clear that it is a collection of entities, not a single entity.

---

python/OrToolsSubjectComponent/or_tools_component/__init__.py line 71 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Let's log:

Finished running subject tracking job. Found X subjects and Y non-subject entities.

Done.

---

python/OrToolsSubjectComponent/or_tools_component/__init__.py line 178 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

I'm assuming this method takes the most amount of time. Please add a log line stating how much time it took to run. Add other timing log lines that you think might be helpful.

I don't think we should be including code for timing things like that. The way you address things that are taking too long is to use a profiler. Following your instructions here is just going to give people an inaccurate idea of what is actually going on. Your assumption may be inaccurate, but could be verified by using a profiler. Adding timing information to just that one method will not allow you to verify your assumption.

I can add log lines stating that we are assigning tracks, but it wouldn't include timing information. An interested party would need to subtract the timestamps.

---

python/OrToolsSubjectComponent/or_tools_component/__init__.py line 202 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Add a comment here. For example:

"Ensure that each person track is only assigned to one face track."

Done.

---

python/OrToolsSubjectComponent/or_tools_component/__init__.py line 220 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Please add a method comment explaining what this does. For example:

"Ensure that only person tracks with no frames in common are assigned to the same face track."

Done.

---

python/OrToolsSubjectComponent/or_tools_component/__init__.py line 392 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Missing -> type annotation for the return value.

Done.

---

python/OrToolsSubjectComponent/or_tools_component/config.json line 10 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

I think this should be entity_type. That's what you call it in the code.

Done.

[brosenberg42]

Reviewable status: 2 of 9 files reviewed, 11 unresolved discussions (waiting on @jrobble)

---

python/OrToolsSubjectComponent/Dockerfile line 39 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Please add labels like these:

LABEL org.label-schema.license="Apache 2.0" \
      org.label-schema.name="OpenMPF Azure Read Text Detection" \
      org.label-schema.schema-version="1.0" \
      org.label-schema.url="https://openmpf.github.io" \
      org.label-schema.vcs-url="https://github.com/openmpf/openmpf-components" \
      org.label-schema.vendor="MITRE"

I know this is a first cut so things like this are missing. Before we land this, please add a README.md, LICENSE, and NOTICE.

Done.

[jrobble]

Reviewed 2 of 2 files at r3, 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @brosenberg42)

---

python/OrToolsSubjectComponent/or_tools_component/__init__.py line 178 at r1 (raw file):

Previously, brosenberg42 wrote…

I don't think we should be including code for timing things like that. The way you address things that are taking too long is to use a profiler. Following your instructions here is just going to give people an inaccurate idea of what is actually going on. Your assumption may be inaccurate, but could be verified by using a profiler. Adding timing information to just that one method will not allow you to verify your assumption.

I can add log lines stating that we are assigning tracks, but it wouldn't include timing information. An interested party would need to subtract the timestamps.

For now we agreed that there may be a better way to get timing info. There are some things to consider:

• Getting enough info to triage an issue, like knowing if it's related to the env., and knowing what the overall behavior is that's causing the issue.
• Getting enough info to technically debug an issue (profile).
• Getting stats for comparison, like frames/sec, faces/sec, entities/sec.
• Enabling someone not familiar with the code to get stats / enough log info to understand expected performance, and easily compare performance when things change (like libs) and running on different hardware (GPUs).

[brosenberg42]

Reviewable status: 8 of 9 files reviewed, 2 unresolved discussions (waiting on @jrobble)

---

python/OrToolsSubjectComponent/Dockerfile line 33 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Eventually we'll need to add unit tests to this.

Added to openmpf/openmpf#1866

---

python/OrToolsSubjectComponent/or_tools_component/__init__.py line 178 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

For now we agreed that there may be a better way to get timing info. There are some things to consider:

• Getting enough info to triage an issue, like knowing if it's related to the env., and knowing what the overall behavior is that's causing the issue.
• Getting enough info to technically debug an issue (profile).
• Getting stats for comparison, like frames/sec, faces/sec, entities/sec.
• Enabling someone not familiar with the code to get stats / enough log info to understand expected performance, and easily compare performance when things change (like libs) and running on different hardware (GPUs).

Done.

[jrobble]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @brosenberg42)

---

python/OrToolsSubjectComponent/or_tools_component/__init__.py line 434 at r6 (raw file):

        entity_track: NonSubjectEntity) -> List[FrameIndex]:
    return sorted(
        fi for fi, subj_rects, det_rect in get_items_with_common_keys(

Why is this det_rect and not det_rects?

[jrobble]

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

---

python/OrToolsSubjectComponent/or_tools_component/__init__.py line 385 at r6 (raw file):

@timing.time_func('Finding relationships')

Great job with the timer and user of decorators.

[brosenberg42]

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

---

python/OrToolsSubjectComponent/or_tools_component/__init__.py line 434 at r6 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Why is this det_rect and not det_rects?

subj_rects is a List[Rect[int]], but det_rect is a Rect[int]. This difference is a bit more obvious if you look at the call to any_intersection on line 436 below. det_rect needs to put in to a tuple because it is not a collection.

[jrobble]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @brosenberg42)

---

python/OrToolsSubjectComponent/or_tools_component/__init__.py line 434 at r6 (raw file):

Previously, brosenberg42 wrote…

subj_rects is a List[Rect[int]], but det_rect is a Rect[int]. This difference is a bit more obvious if you look at the call to any_intersection on line 436 below. det_rect needs to put in to a tuple because it is not a collection.

Looking closer, I get it. A subject's detection rects is a map of frame ids to rects that spans across all of the tracks assoc. with that subject. An entity track (NonSubjectEntity) is just a single track, so there will be one rect at most assoc. with each frame id.