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

[luci] Remove Add_STR tests #13751

Merged
merged 1 commit into from
Aug 26, 2024
Merged

Conversation

jinevening
Copy link
Contributor

@jinevening jinevening commented Aug 26, 2024

This removes invalid Add_STR tests.

ONE-DCO-1.0-Signed-off-by: Hyukjin Jeong [email protected]


Related to: https://github.com/Samsung/ONE/pull/13750/files#r1730675966
Draft PR: #13750

This removes invalid Add_STR tests.

ONE-DCO-1.0-Signed-off-by: Hyukjin Jeong <[email protected]>
@seanshpark
Copy link
Contributor

As the comment says

# This is test for import/export of STRING tensortype
# interpreter or runtime may fail as Add won't support this
``
this is just for STRING tensortype, happen to be using `Add` Op...
we should not remove this.

what test is failing?

@jinevening
Copy link
Contributor Author

PTAL https://github.com/Samsung/ONE/pull/13750/files#r1730675966. STRING inputs are not valid for ADD. Those recipes should have CUSTOM operators.

@seanshpark
Copy link
Contributor

... Those recipes should have CUSTOM operators.

I've read the comment but I don't know which test fails.
Asking again, which test fail?

@jinevening
Copy link
Contributor Author

what test is failing?

Circle files for Add_STR are not created under common-artifacts after ADD's shape inference logic is updated.

@jinevening
Copy link
Contributor Author

It seems that circle2circle is a problem. Add_STR_000.opt.circle is not created.

@seanshpark
Copy link
Contributor

under common-artifacts after ADD's shape inference logic is updated.

can you add to to exclude.lst ?

@seanshpark
Copy link
Contributor

tcgenerate(Add_STR_000) # STRING is not supported
tcgenerate(Add_STR_001) # STRING is not supported

um.. already added...

@jinevening
Copy link
Contributor Author

I think tcgenerate is not enough. May need to add optimize(Add_STR_000) and optimize(Add_STR_001).

@seanshpark
Copy link
Contributor

um... sorry for wrong questions. I was confused with the draft and changes of this PR.

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
Copy link
Contributor

Anyway recipe itself should not be removed for tflchef.

@jinevening
Copy link
Contributor Author

Anyway recipe itself should not be removed for tflchef.

I see. With #13754, it will be possible to leave the recipes. But, those recipes generate invalid tflite, so I think it would be better to remove them someday.

@jinevening jinevening merged commit 96b5c2e into Samsung:master Aug 26, 2024
10 checks passed
@jinevening jinevening deleted the luci/remove_add_str branch August 26, 2024 06:50
@seanshpark
Copy link
Contributor

so I think it would be better to remove them someday.

Yes, they are invalid files but they exist for test creating STRING tensors, nothing more.

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.

2 participants