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.

Sign up for GitHub

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

Jump to bottom

Add StringLower/StringUpper op to support lower/upper() for string operations #25859

Merged
merged 10 commits into from May 3, 2019
Merged

Add StringLower/StringUpper op to support lower/upper() for string operations #25859

merged 10 commits into from May 3, 2019

Conversation

yongtang
Copy link
Member

This fix tries to address the issue raised in #25857 where there is no lower() or upper() for string operations yet.

This fix adds StringLower/StringUpper to the kernel to support this op.

The endpoints are exposed as strings.lower and strings.upper.

This fix fixes #25857.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

@yongtang yongtang mentioned this pull request Feb 19, 2019
Closed
@rthadur rthadur self-assigned this Feb 19, 2019
@rthadur rthadur added this to Assigned Reviewer in PR Queue via automation Feb 19, 2019
@ymodak ymodak added awaiting review Pull request awaiting review size:L CL Change Size: Large prtype:bugfix PR to fix a bug labels Feb 19, 2019
@penpornk penpornk added the API review API Review label Feb 22, 2019
PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Feb 26, 2019
alextp
alextp suggested changes Feb 26, 2019
View reviewed changes
Copy link
Contributor

@alextp alextp left a comment

Choose a reason for hiding this comment

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

Do we need to specify a unicode encoding somewhere here since we're manipulating characters?

@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Feb 27, 2019
@yongtang
Copy link
Member Author

Thanks @alextp. I take a look at TF 2.0 API: https://www.tensorflow.org/versions/r2.0/api_docs/python/tf/strings

It looks like unicode operations are separated with unicode_ prefixes. Wondering if it make sense to create similar unicode_upper and unicode_lower? Or we could also consolidate unicode operations into part of the existing strings operation?

@alextp
Copy link
Contributor

alextp commented Feb 27, 2019 via email

@yongtang
Copy link
Member Author

yongtang commented Mar 1, 2019

Thanks @alextp. I will update the PR to add the unicode option on top of lower/upper.

@yongtang
Add StringLower op to support lower() for string operations
f00ca97 
This fix tries to address the issue raised in 25857 where
there is no lower() for string operations yet.

This fix adds StringLower to the kernel to support this op.

This fix fixes 25857.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang
Expose strings.lower api endpoint based on StringLower op
a19e835 
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang
Add test case for strings.lower op
e0a90f8 
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang
Add StringUpper op to support upper() for string operations
5781625 
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang
Add test case for strings.upper
7cf89bd 
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang
Update API compatibility with
e2da5fd 
bazel-bin/tensorflow/tools/api/tests/api_compatibility_test
           --update_goldens True

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang
Add API defs with tensorflow/core/api_def/update_api_def.sh
5ebe920 
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang yongtang added the kokoro:force-run Tests on submitted change label Apr 3, 2019
@yongtang
Add unicode support of strings.lower and strings.upper
b102e32 
with additional encoding='' support, utf-8 is supported at the moment

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 3, 2019
@yongtang
Update api golden
92828cb 
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang
Copy link
Member Author

yongtang commented Apr 3, 2019

@alextp Thanks for the review and sorry for the delay. The PR has been updated to add an additional arg of encoding='' to allow unicode support. At the moment utf-8 is supported but additional encoding is possible if needed. Please take a look.

@rthadur rthadur requested a review from alextp April 3, 2019 21:15
alextp
alextp previously approved these changes Apr 17, 2019
View reviewed changes
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Apr 17, 2019
@alextp alextp added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process and removed API review API Review labels Apr 17, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 17, 2019
@TimZaman
Copy link
Contributor

Sweet

@penpornk penpornk added the kokoro:force-run Tests on submitted change label Apr 25, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 25, 2019
@penpornk
Copy link
Member

@alextp Thank you for reviewing! Sorry that I tagged API review early. I wasn't sure if it could be tagged at any time or just after the PR is approved so I kind of tagged it early. Will only tag after PR is approved next time.

@alextp
Copy link
Contributor

alextp commented Apr 25, 2019 via email

@penpornk
Copy link
Member

@alextp Oh, I meant you ended up having to review the whole PR instead because I tagged API review early. Sorry about that. :)

@penpornk
Copy link
Member

@yongtang Could you please take a look at test failures?
You might want to start with Ubuntu Python3's //tensorflow/python/kernel_tests:string_lower_op_test and //tensorflow/python/kernel_tests:string_upper_op_test

@yongtang
Fix python 3 test failures caused by unicode string.
fda502e 
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Apr 26, 2019
@yongtang yongtang added the kokoro:force-run Tests on submitted change label Apr 26, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 26, 2019
@yongtang
Copy link
Member Author

@penpornk Thanks for the review. The python 3 string error has been fixed and the PR has been updated. There are other test failures but it looks like they are unrelated. Please take a look and let me know if there are any issues.

@penpornk penpornk added the kokoro:force-run Tests on submitted change label Apr 26, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 26, 2019
@penpornk penpornk added the kokoro:force-run Tests on submitted change label Apr 26, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 26, 2019
penpornk
penpornk reviewed Apr 26, 2019
View reviewed changes
Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

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

The tests look fine now. Thanks again for your contribution! :)

@yongtang
Copy link
Member Author

@rthadur @penpornk Looks like copybara is failing:

feedback/copybara — Google internal checks FAILED for runs with \
     create time 2019-04-26T23:57:23.564146795Z,2019-04-26T23:55:48.976711363Z.

Wondering if additional update is needed or anything else I need to do?

@rthadur rthadur requested a review from penpornk April 29, 2019 18:22
penpornk
penpornk reviewed Apr 29, 2019
View reviewed changes
Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

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

I think it was just some flaky tests. The internal tests are being rerun. I'll let you know if we need more edits. Thank you for asking!

@tensorflow-copybara tensorflow-copybara merged commit fda502e into tensorflow:master May 3, 2019
PR Queue automation moved this from Reviewer Requested Changes to Merged May 3, 2019
@yongtang yongtang deleted the 25857-strings.lower branch May 3, 2019 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimZaman TimZaman TimZaman left review comments

@penpornk penpornk penpornk left review comments

@alextp alextp alextp left review comments

Assignees

@rthadur rthadur

Labels
cla: yes prtype:bugfix PR to fix a bug ready to pull PR ready for merge process size:L CL Change Size: Large
Projects
PR Queue
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add lower() to string operations
10 participants
@yongtang @alextp @TimZaman @penpornk @googlebot @kokoro-team @tensorflowbutler @tensorflow-copybara @ymodak @rthadur