Skip to content

[luci] Handle dynamic dimensions in Squeeze shape inference#14857

Merged
seanshpark merged 2 commits intoSamsung:masterfrom
mbencer:mbencer/HandleDynDimInSquueze
Mar 27, 2025
Merged

[luci] Handle dynamic dimensions in Squeeze shape inference#14857
seanshpark merged 2 commits intoSamsung:masterfrom
mbencer:mbencer/HandleDynDimInSquueze

Conversation

@mbencer
Copy link
Contributor

@mbencer mbencer commented Mar 19, 2025

This commit skip verification of squeezed dimension if such dimension is dynamic.

ONE-DCO-1.0-Signed-off-by: Mateusz Bencer m.bencer@partner.samsung.com

Draft: #14882
Issue: #14791

This commit skip verification of squeezed dimension if such dimension is dynamic.

ONE-DCO-1.0-Signed-off-by: Mateusz Bencer <m.bencer@partner.samsung.com>
@seanshpark
Copy link
Contributor

Plz follow #14844 (comment) for this too

@mbencer
Copy link
Contributor Author

mbencer commented Mar 20, 2025

Plz follow #14844 (comment) for this too

I'm trying with TF recipe like:

operand {
  name: "ifm"
  type: FLOAT32
  shape { dim: 3 dim: 4 dim: 5 dim: 1}
  shape_signature { dim: 3 dim: 4 dim: 5 dim: -1 }
}
operand {
  name: "ofm"
  type: FLOAT32
  shape { dim: 3 dim: 4 dim: 5 }
}
operation {
  type: "Squeeze"
  squeeze_options { squeeze_dim: 3 }
  input: "ifm"
  output: "ofm"
}
input: "ifm"
output: "ofm"

But it fails on asserts in luci-interpreter Squeeze kernel because of dynamic dimension in input shape. It looks like only shape_signature is propagated there.

@seanshpark
Copy link
Contributor

I'm trying with TF recipe like:

this is tflite recipe, not TF

... on asserts in luci-interpreter Squeeze kernel ...

you may have to fix luci-interpreter too, as I know you are to support dynamic shapes.

jinevening
jinevening previously approved these changes Mar 21, 2025
Copy link
Collaborator

@jinevening jinevening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code itself looks good to me.

@mbencer
Copy link
Contributor Author

mbencer commented Mar 21, 2025

@seanshpark

this is tflite recipe, not TF

Right ;)

you may have to fix luci-interpreter too, as I know you are to support dynamic shapes.

I see but it looks like a bigger task. In the current state I'm specializing dynamic shape model to the static version via circle-resizer but still probably will be needed in the future. I'll think about it ;)

In the current moment I'm proposing introduce the Squeeze test based on tflite recipe in steps (the order is important):
#14864
#14865
#14866

But this PR should be merged at first.

@seanshpark
Copy link
Contributor

as you've posted multiple PRs related to this PR, please add a link to the full draft PR.
I'd like to check all.

@mbencer
Copy link
Contributor Author

mbencer commented Mar 24, 2025

as you've posted multiple PRs related to this PR, please add a link to the full draft PR. I'd like to check all.

sure - Draft: #14882

@seanshpark seanshpark self-requested a review March 24, 2025 09:23
@mbencer
Copy link
Contributor Author

mbencer commented Mar 27, 2025

@seanshpark In my opinion we should merge the PRs in order:

@seanshpark
Copy link
Contributor

we should merge the PR

I'd like to but you didn't apply the changes from the draft.

@mbencer
Copy link
Contributor Author

mbencer commented Mar 27, 2025

we should merge the PR

I'd like to but you didn't apply the changes from the draft.

ohh, I see, I've missed #14882 (comment)

@mbencer mbencer requested a review from jinevening March 27, 2025 08:41
Copy link
Collaborator

@jinevening jinevening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@seanshpark seanshpark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@seanshpark seanshpark merged commit 9350461 into Samsung:master Mar 27, 2025
8 checks passed
@mbencer mbencer deleted the mbencer/HandleDynDimInSquueze branch May 2, 2025 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants