Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Layer][GPU][TF FE] test_segment_sum failed with get_shape was called… #28778

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pravin25
Copy link
Contributor

@pravin25 pravin25 commented Feb 3, 2025

Details:

  • Added dynamic shape support in GPU inference pipeline for SegmentSum ops
  • Updated tensorflow_tests/test_tf_SegmentSum.py to remove GPU skip
  • Ensured correct handling of dynamic shapes during GPU inference for SegmentSum This update resolves the GPU test failure and ensures proper support for dynamic shapes in SegmentSum ops
  • Added support for passing custom_eps through kwargs to allow dynamic epsilon values for comparison in tests.
    This improves flexibility in handling precision variations (FP32, FP16).
  • Following three tickets are interrelated.

Tickets:

@pravin25 pravin25 requested review from a team as code owners February 3, 2025 08:36
@github-actions github-actions bot added category: Core OpenVINO Core (aka ngraph) category: GPU OpenVINO GPU plugin category: TF FE OpenVINO TensorFlow FrontEnd labels Feb 3, 2025
@sys-openvino-ci sys-openvino-ci added the ExternalIntelPR External contributor from Intel label Feb 3, 2025
@rkazants
Copy link
Member

rkazants commented Feb 3, 2025

From TF FE side, looks good.

@rkazants
Copy link
Member

rkazants commented Feb 3, 2025

build_jenkins

@p-durandin
Copy link
Contributor

build_jenkins

Comment on lines 50 to 52
const size_t num_segments_idx = 2;
TensorsContainer const_data(&impl_param.get_stream());
if (memory_deps.count(num_segments_idx) > 0) {
const_data.emplace(3, memory_deps.at(num_segments_idx));
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I understand correctly that it uses dependency_idx=2 as num_segments input? It seems that dependency_idx=2 refers to segment_ids, doesn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, dependency_idx = 2 likely refers to segment IDs, and num_segments would be the number of segments.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pravin25, I believe that such replacement is not correct in all cases. If an "empty segment" is the last one (and thus not present in segment_ids), this approach may lead to incorrect segment number calculations
Here is example from the specs:

<layer ... type="EmbeddingSegmentsSum" ... >
    <input>
        <port id="0">     <!-- emb_table value is: [[-0.2, -0.6], [-0.1, -0.4], [-1.9, -1.8], [-1.,  1.5], [ 0.8, -0.7]] -->
            <dim>5</dim>
            <dim>2</dim>
        </port>
        <port id="1">     <!-- indices value is: [0, 2, 3, 4] -->
            <dim>4</dim>
        </port>
        <port id="2"/>    <!-- segment_ids value is: [0, 0, 2, 2] - second segment is empty -->
            <dim>4</dim>
        </port>
        <port id="3"/>    <!-- num_segments value is: 3 -->
        <port id="4"/>    <!-- default_index value is: 0 -->
        <port id="5"/>    <!-- per_sample_weigths value is: [0.5, 0.5, 0.5, 0.5] -->
            <dim>4</dim>
        </port>
    </input>
    <output>
        <port id="6">     <!-- output value is: [[-1.05, -1.2], [-0.2, -0.6], [-0.1, 0.4]] -->
            <dim>3</dim>
            <dim>2</dim>
        </port>
    </output>
</layer>

But what if segment_ids was configured like this?

        <port id="2"/>    <!-- segment_ids value is: [0, 0, 1, 1] - third segment is empty -->
            <dim>4</dim>
        </port>

Copy link
Contributor Author

@pravin25 pravin25 Feb 12, 2025

Choose a reason for hiding this comment

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

@sshlyapn , First I want to correct this example. It should be

<port id="2"/>    <!-- segment_ids value is: [0, 0, 0, 1, 1]  -  fourth segment is empty -->
      <dim>5</dim>
 </port>

Because data shape dimension and segment_id_shape have the same dimension.
If it is a mismatch, OpenVINO throws an exception.
Based on segment_id, the kernel calculates segment ops.
The second thing I take care of is if the blank segment_id comes.
Here is that core/shape_inference/includes/embedding_segments_sum_shape_inference.hpp:56-63

@p-durandin
Copy link
Contributor

@pravin25 please take a look on failed tests

@p-durandin
Copy link
Contributor

build_jenkins

@github-actions github-actions bot removed the category: Core OpenVINO Core (aka ngraph) label Feb 14, 2025
@p-durandin
Copy link
Contributor

build_jenkins

… on a descriptor

Fix GPU TensorFlow SegmentSum test failure
- Added dynamic shape support in GPU inference pipeline for SegmentSum ops
- Updated tensorflow_tests/test_tf_SegmentSum.py to remove GPU skip
- Ensured correct inputs for EmbededdingSegmentSum ops
- Ensured correct handling of dynamic shapes during GPU inference for SegmentSum
- Pass custom_eps via kwargs for flexible test tolerances.
- Removed calculate segments_num logic from embedding_segments_sum_shape_inference.hpp

Following both tickets are interrelated.
https://jira.devtools.intel.com/browse/CVS-105896
https://jira.devtools.intel.com/browse/CVS-152352
https://jira.devtools.intel.com/browse/CVS-156362
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: GPU OpenVINO GPU plugin category: TF FE OpenVINO TensorFlow FrontEnd ExternalIntelPR External contributor from Intel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants