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

[PluggableDevice] Kernel C API enhancement for retrieving attributes #44017

Merged
merged 11 commits into from Dec 9, 2020
Merged

[PluggableDevice] Kernel C API enhancement for retrieving attributes #44017

merged 11 commits into from Dec 9, 2020

Conversation

jzhoulon
Copy link
Contributor

Adding more attribution C API for pluggable device kernels implementation.

@google-ml-butler google-ml-butler bot added the size:L CL Change Size: Large label Oct 14, 2020
@google-cla google-cla bot added the cla: yes label Oct 14, 2020
@jzhoulon
Copy link
Contributor Author

@annarev @penpornk @yisitu We have added some kernel C API for retrieving more op attributions. Can you help to review it? thanks.

@gbaned gbaned self-assigned this Oct 14, 2020
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Oct 14, 2020
@gbaned gbaned requested a review from annarev October 14, 2020 16:12
@gbaned gbaned added the awaiting review Pull request awaiting review label Oct 16, 2020
annarev
annarev reviewed Oct 18, 2020
View reviewed changes
tensorflow/c/kernels.cc Outdated Show resolved Hide resolved
tensorflow/c/kernels.cc Show resolved Hide resolved
tensorflow/c/kernels_test.cc Outdated Show resolved Hide resolved
tensorflow/c/kernels_test.cc Outdated Show resolved Hide resolved
@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Oct 23, 2020
@gbaned gbaned requested a review from annarev October 27, 2020 16:55
@gbaned gbaned added the awaiting review Pull request awaiting review label Oct 27, 2020
@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Oct 31, 2020
@gbaned gbaned requested review from annarev and removed request for annarev November 2, 2020 11:05
annarev
annarev reviewed Nov 3, 2020
View reviewed changes
tensorflow/c/kernels.cc Outdated Show resolved Hide resolved
tensorflow/c/kernels.cc Outdated Show resolved Hide resolved
tensorflow/c/kernels.h Outdated Show resolved Hide resolved
tensorflow/c/kernels.cc Show resolved Hide resolved
tensorflow/c/kernels.h Outdated Show resolved Hide resolved
tensorflow/c/kernels.h Outdated Show resolved Hide resolved
tensorflow/c/kernels.h Outdated Show resolved Hide resolved
PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Nov 5, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Nov 5, 2020
@annarev annarev added the API review API Review label Nov 5, 2020
@annarev
Copy link
Contributor

annarev commented Nov 5, 2020

Looks good to me. I added API review label so that API owners can take a look as well

@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Nov 5, 2020
@penpornk penpornk mentioned this pull request Nov 5, 2020
Merged
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Nov 5, 2020
@jzhoulon
Copy link
Contributor Author

@jzhoulon Can you please resolve conflicts? Thanks!

@gbaned I have resolved conflicts.

@gbaned gbaned requested a review from annarev November 26, 2020 03:21
guptapriya
guptapriya reviewed Dec 2, 2020
View reviewed changes
Copy link
Contributor

@guptapriya guptapriya left a comment

Choose a reason for hiding this comment

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

(for tf-api-owners)

tensorflow/c/kernels.h
//
// If the attribute could not be found or could not be interpreted as
// bool, *status is populated with an error.
TF_CAPI_EXPORT extern void TF_OpKernelConstruction_GetAttrString(
Copy link
Contributor

Choose a reason for hiding this comment

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

So should the type of val be char* val instead?

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Dec 5, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 5, 2020
guptapriya
guptapriya approved these changes Dec 7, 2020
View reviewed changes
Copy link
Contributor

@guptapriya guptapriya left a comment

Choose a reason for hiding this comment

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

for @tensorflow/api-owners

@google-ml-butler google-ml-butler bot added the kokoro:force-run Tests on submitted change label Dec 7, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 7, 2020
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Dec 8, 2020
annarev
annarev reviewed Dec 8, 2020
View reviewed changes
tensorflow/c/kernels_test.cc Outdated

// Helper macros for the TF_OpKernelConstruction_GetAttr* tests.
#define EXPECT_TF_SIZE(attr_name, expected_list_size, expected_total_size) \
\
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line. It just contains \

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Dec 8, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 8, 2020
@gbaned gbaned removed the ready to pull PR ready for merge process label Dec 8, 2020
@gbaned gbaned requested a review from annarev December 8, 2020 13:46
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Dec 8, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 8, 2020
@copybara-service copybara-service bot merged commit 798304c into tensorflow:master Dec 9, 2020
PR Queue automation moved this from Approved by Reviewer to Merged Dec 9, 2020
copybara-service bot pushed a commit that referenced this pull request Dec 10, 2020
@mihaimaruseac @tensorflower-gardener
Revert PR #44017: [PluggableDevice] Kernel C API enhancement for retr…
03e4d9f 
…ieving attributes

PiperOrigin-RevId: 346844467
Change-Id: I9d2121c3ea2402df51879852d0bade21577b3d9b
copybara-service bot pushed a commit that referenced this pull request Dec 11, 2020
@tensorflower-gardener
Merge pull request #44017 from Intel-tensorflow:pluggable_device_kern…
e9868ac 
…el_c_api

PiperOrigin-RevId: 347042144
Change-Id: I05c7587c7a491199f8ba24109eaf4f6d1bc83c72
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guptapriya guptapriya guptapriya approved these changes

@annarev annarev annarev approved these changes

@fchollet fchollet Awaiting requested review from fchollet

Assignees

@gbaned gbaned

Labels
API review API Review cla: yes 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.

None yet
7 participants
@jzhoulon @annarev @gbaned @fchollet @guptapriya @kokoro-team @tensorflowbutler