SAHI in `workflows` #574

PawelPeczek-Roboflow · 2024-08-13T11:45:36Z

Description

In this PR we are adding SAHI to workflows in the following way:

there is a block with image slicer (private implementation copied from supervision to avoid fragile integration on private interface)
there is image stitch block that is capable of merging detections yielded from crops into single detections objects

😄 PROS of the approach:

we have single schema of SAHI for multiple detections / segmentation models which is superior over Add 2x Inference Slicer blocks #456
we can use stitch not only for SAHI technique (shown in tests)

😢 CONS of the approach:

we do not fully re-use supervision implementation
we have three blocks instead of one per model - which is harder to initialise and UI would need to start displaying group of blocks as one - please note that @EmilyGavrilenko @hansent

@LinasKo could u please take a look at the implementation to verify that usage of supervision components is as intended

NUANCES (please acknowledge carefully)

stitch block requires reference_image - which is the image used to create crops, which will create the following schema:

dimensionality 1: image -> slicer \ --------------- image -----> stitch -> merged predictions
dimensionality 2:                  \-> model -> predictions            /

Initially I wanted to avoid this additional parameter reference_image - but we need it if we wanted to maintain notion of parent_id in sv.Detections - which is used by such blocks as Detections Consensus and can ensure that whenever
we are blending is coming from the same image - which is helpful to prevent bugs. I tried to remove the problem but that lead me to lineage property being added into Execution Engine entity causing partially breaking change to EE which I wanted to avoid (probably impact would be low, as there are barely no custom plugins created), but stopped the work in this direction as this is labour intensive and potentially not needed if you like the approach. If you don't we need to:

add lineage property (see commits in the PR which were reverted)
fix Prediction-level metadata in sv.Detections supervision#1226

In my opinion this is discussion about tradeoffs:

the solution that we have now relies on proper metadata to be passed along with proper inter-dimension-level connections between steps
the alternative solution relies on adding more and more payload into data flowing through workflows
both approaches have pros and cons - I would like to know your opinions on that matter

IMPORTANT
SAHI is not always good approach - please try to plt.imshow() visualisations in integration tests running on cars images to see how improper use is going to lead to terrible results - we should mindfully guide users towards the feature, otherwise people will not like that.

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
This change requires a documentation update

How has this change been tested, please provide a testcase or example of how you tested the change?

CI green 🟢
new tests

Any specific deployment considerations

For example, documentation changes, usability, usage/costs, secrets, etc.

Docs

Docs updated? What were the changes:

…about detections metadata

…rvation of all required properties

…stead

…ts offstes

PawelPeczek-Roboflow · 2024-08-13T12:18:29Z

We also need to decide on reasonable defaults for slice size. I bet that 640 may be better than 320, but this is only gut feeling based on assumption that size of input for model is more likely to be 640 than 320.

hansent · 2024-08-13T15:21:09Z

we have three blocks instead of one per model - which is harder to initialise and UI would need to start displaying group of blocks as one - please note that @EmilyGavrilenko
@thomas

We need this UI change to merge this, or you mean in general this is a pattern we should move towards to make this easier / better UX? Not a trivial UI change in terms of graph/viz editing state I think.

Could we have the individual blocks in addition to a block that handles all of it for ease of use / most common scenario?

PawelPeczek-Roboflow · 2024-08-13T15:26:01Z

we have three blocks instead of one per model - which is harder to initialise and UI would need to start displaying group of blocks as one - please note that @EmilyGavrilenko
@thomas

We need this UI change to merge this, or you mean in general this is a pattern we should move towards to make this easier / better UX? Not a trivial UI change in terms of graph/viz editing state I think.

I meant changes in UI in the future, but I bet now it will be confusing if we added the blocks from this PR - I bet noone would know how to use it.

Could we have the individual blocks in addition to a block that handles all of it for ease of use / most common scenario?
well, we could, but I really would like to avoid that - as we will be in need to maintain that over time

EmilyGavrilenko · 2024-08-13T16:40:49Z

we have three blocks instead of one per model - which is harder to initialize and UI would need to start displaying group of blocks as one - please note that @EmilyGavrilenko
@thomas

We need this UI change to merge this, or you mean in general this is a pattern we should move towards to make this easier / better UX? Not a trivial UI change in terms of graph/viz editing state I think.

I meant changes in UI in the future, but I bet now it will be confusing if we added the blocks from this PR - I bet noone would know how to use it.

Could we have the individual blocks in addition to a block that handles all of it for ease of use / most common scenario?
well, we could, but I really would like to avoid that - as we will be in need to maintain that over time

Agree that having 3 blocks will be very confusing from a UI perspective. Couple questions/thoughts:

are there uses for the DetectionsStitch and ImageSlicer blocks outside of SAHI? Should we allow users to add them individually, or only as a SAHI group
in terms of UX, we should have a SAHI block, which adds all 3 blocks to the UI for configuration as a group. However, there's the added issue that we don't have a Model block, and it's currently very difficult to swap one model type out for another.
- For the ideal user experience, we add a dummy Model block which only allows you to add a supported model type, and then a grouped block type which adds a group of blocks and if one is deleted all of them are.

However, the ideal UX is a much bigger lift, we can start providing value to users with the individual blocks + a blog post to explain how to configure them

PawelPeczek-Roboflow · 2024-08-14T07:34:47Z

Re: @EmilyGavrilenko comment:

There is usage outside SAHI - illustrated here
I agree with direction of UI and the plan for onboarding individual blocks. I much prefer having blocks doing single things that can be nicely composable, rather than multiple variations of groupped blocks in backend when simplification can be introduced in UI

PawelPeczek-Roboflow · 2024-08-14T16:09:57Z

Example where SAHI does not work

hansent · 2024-08-15T12:06:29Z

inference/core/workflows/core_steps/fusion/detections_stitch/v1.py

+                resolution_wh=resolution_wh,
+            )
+            re_aligned_predictions.append(re_aligned_detections)
+        overlap_filter = choose_overlap_filter_strategy(


should we be merging / detecting duplicates each time we add boxes rather than once at the end? Maybe that doesn't make a difference?

we should do it at the end, otherwise we are running unneeded computations I am afraid, besides - this is the implementation of sv.InferenceSlicer that I hope will become part of public interface later on, such that we could replace our duplicate code with sv

hansent · 2024-08-15T12:10:52Z

inference/core/workflows/core_steps/fusion/detections_stitch/v1.py

+    if SCALING_RELATIVE_TO_PARENT_KEY in detections.data:
+        scale = detections[SCALING_RELATIVE_TO_PARENT_KEY][0]
+        if abs(scale - 1.0) > 1e-4:
+            raise ValueError(


what s an example of this happening?

if you bring operation of bboxes scaling in between model and stitch, which you should not be doing, but you can through UQL

hansent · 2024-08-15T13:01:44Z

inference/core/workflows/core_steps/transformations/image_slicer/v1.py

+    @classmethod
+    def describe_outputs(cls) -> List[OutputDefinition]:
+        return [
+            OutputDefinition(name="crops", kind=[BATCH_OF_IMAGES_KIND]),


I think output should be named slices instead of crops given other naming on block and properties

hansent · 2024-08-15T13:07:35Z

tests/workflows/integration_tests/execution/test_workflow_with_stitch_for_dynamic_crop.py

+    result = execution_engine.run(
+        runtime_parameters={
+            "image": [license_plate_image],
+            "overlap_filtering_strategy": "none",


Suggested change

"overlap_filtering_strategy": "none",

not used in workflow

hansent · 2024-08-15T13:11:44Z

tests/workflows/unit_tests/core_steps/fusion/test_detections_stitch.py

np tests for actual block functionality. I think its OK. since its basically tested in test_workflows_with_sahi.py but maybe add a comment to point to that file in case someone comes in here to understand how this block should behave

hansent

Let's rename slider output to slices instead of crops but otherwise looks great!

PawelPeczek-Roboflow added 8 commits August 12, 2024 14:56

Add first implementation of SAHI

2668137

Modify implementation such that it requires less selectors and cares …

e81bb1b

…about detections metadata

Add first tests

1c273fb

Add changes to properly test execution of new blocks, including prese…

f37b52e

…rvation of all required properties

Test changes regarding image lineage

5e230ce

Get rid of lineage and use explicit passing of high-level metadata in…

b990bb4

…stead

Add tests for SAHI at instance segmentation

5bd8eaa

Extend schema endpoint to return also definitions of inputs and outpu…

7bb4848

…ts offstes

PawelPeczek-Roboflow requested review from grzegorz-roboflow, yeldarby, probicheaux and hansent as code owners August 13, 2024 11:45

PawelPeczek-Roboflow requested review from EmilyGavrilenko and LinasKo August 13, 2024 11:45

PawelPeczek-Roboflow mentioned this pull request Aug 13, 2024

Add 2x Inference Slicer blocks #456

Closed

2 tasks

Merge branch 'main' into feature/inference_slicer_new

bdf5960

Merge branch 'main' into feature/inference_slicer_new

94c66e9

hansent reviewed Aug 15, 2024

View reviewed changes

hansent requested changes Aug 15, 2024

View reviewed changes

Merge branch 'main' into feature/inference_slicer_new

f3ef9c0

PawelPeczek-Roboflow added 2 commits August 15, 2024 15:31

Apply CR suggestions

ad3a0b2

Fix unit tests

5770291

PawelPeczek-Roboflow requested a review from hansent August 15, 2024 13:43

hansent approved these changes Aug 15, 2024

View reviewed changes

PawelPeczek-Roboflow merged commit f422a3e into main Aug 15, 2024
58 checks passed

PawelPeczek-Roboflow deleted the feature/inference_slicer_new branch August 15, 2024 14:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SAHI in `workflows` #574

SAHI in `workflows` #574

PawelPeczek-Roboflow commented Aug 13, 2024

PawelPeczek-Roboflow commented Aug 13, 2024

hansent commented Aug 13, 2024

PawelPeczek-Roboflow commented Aug 13, 2024

EmilyGavrilenko commented Aug 13, 2024

PawelPeczek-Roboflow commented Aug 14, 2024

PawelPeczek-Roboflow commented Aug 14, 2024

hansent Aug 15, 2024

PawelPeczek-Roboflow Aug 15, 2024

hansent Aug 15, 2024

PawelPeczek-Roboflow Aug 15, 2024

hansent Aug 15, 2024

PawelPeczek-Roboflow Aug 15, 2024

hansent Aug 15, 2024

PawelPeczek-Roboflow Aug 15, 2024

hansent Aug 15, 2024 •

edited

Loading

hansent left a comment

SAHI in workflows #574

SAHI in workflows #574

Conversation

PawelPeczek-Roboflow commented Aug 13, 2024

Description

NUANCES (please acknowledge carefully)

Type of change

How has this change been tested, please provide a testcase or example of how you tested the change?

Any specific deployment considerations

Docs

PawelPeczek-Roboflow commented Aug 13, 2024

hansent commented Aug 13, 2024

PawelPeczek-Roboflow commented Aug 13, 2024

EmilyGavrilenko commented Aug 13, 2024

PawelPeczek-Roboflow commented Aug 14, 2024

PawelPeczek-Roboflow commented Aug 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hansent Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

hansent left a comment

Choose a reason for hiding this comment

SAHI in `workflows` #574

SAHI in `workflows` #574

hansent Aug 15, 2024 •

edited

Loading