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

[BugFix][MSC] split name_string with index by colon from the right #17000

Merged
merged 2 commits into from
May 29, 2024

Conversation

psunn
Copy link
Contributor

@psunn psunn commented May 15, 2024

Fixes a naming mismatch in MSCGraph where tensor_name could formatted as 'string:index:index',and the corresponding node.name is 'string:index'. Splitting tensor_name from the right aligns it correctly.

For example, the TFLite default input name 'serving_default_input:0' becomes 'serving_default_input:0:0' in MSCGraph, while node.name remains 'serving_default_input:0'.

Fixes a naming mismatch in MSCGraph where tensor_name could formatted as
'string:index:index',and the corresponding node.name is 'string:index'.
Splitting tensor_name from the right aligns it correctly.

For example, the TFLite default input name 'serving_default_input:0'
becomes 'serving_default_input:0:0' in MSCGraph, while node.name remains
'serving_default_input:0'.
Copy link
Contributor

@lhutton1 lhutton1 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @psunn, would it be possible to add a test case?

@Hzfengsy
Copy link
Member

cc @Archermmt

@psunn
Copy link
Contributor Author

psunn commented May 16, 2024

Thanks for the fix @psunn, would it be possible to add a test case?

@lhutton1 Thanks for the review. I've added a test case.

@psunn
Copy link
Contributor Author

psunn commented May 22, 2024

Hi @Archermmt, could you help to review this PR? thanks in advance.

@Archermmt
Copy link
Contributor

Seem ok for me. The only change in this PR is the split direction, which has no influence on current test. I think the main reason is that: in some corner cases like TFLite model, the node name looks like "NODE:IDX", which is normally the tensor name format.

@psunn
Copy link
Contributor Author

psunn commented May 28, 2024

@Archermmt thanks for review.
Yes, the change in this PR has no effect on existing tests. An additional test case has been added to cover node name_string with an index by colon.

I force-pushed only to address linting issues.

@lhutton1 @Archermmt @Hzfengsy
Would it be okay to merge it? thanks.

@lhutton1 lhutton1 merged commit 7afac14 into apache:main May 29, 2024
19 checks passed
@lhutton1
Copy link
Contributor

Thanks @psunn @Archermmt @Hzfengsy!

@psunn psunn deleted the fix_msc_split_str branch May 29, 2024 09:35
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.

None yet

4 participants