Skip to content

Fix build issue with KafkaDataset#17418

Merged
gunan merged 10 commits intotensorflow:masterfrom
yongtang:17210-KafkaDataset
Mar 8, 2018
Merged

Fix build issue with KafkaDataset#17418
gunan merged 10 commits intotensorflow:masterfrom
yongtang:17210-KafkaDataset

Conversation

@yongtang
Copy link
Copy Markdown
Member

@yongtang yongtang commented Mar 4, 2018

This fix tries to address the issue raised in #17210 where error of NotFoundError: Op type not registered 'KafkaDataset' in binary. returned from kafka ops.

The issue was that the inclusion of kafka ops was removed due to the conflict merge from the other PR. This fix fixes the issue.

This fix fixes #17210.

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

yongtang added 4 commits March 4, 2018 22:15
This fix tries to address the issue raised in 17210 where
error of `NotFoundError: Op type not registered 'KafkaDataset' in binary.`
returned from kafka ops. The issue was that the inclusion of kafka ops
was removed due to the conflict merge from the other PR. This fix
fixes the issue.

This fix fixes 17210.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
due to the changes in some other places.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
deps = [":lookup"],
)

cc_library(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This new library shouldn't be necessary. The dataset.h header is now in tensorflow/core/framework and can be included via the //tensorflow/core:framework_headers_lib target.

You might want to check out the targets in //tensorflow/contrib/data/... that build dataset implementations as custom op libraries. This has changed in the last month or so, as we've refactored things to make separate compilation possible for dataset implementations.

yongtang added 2 commits March 6, 2018 21:11
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Copy link
Copy Markdown
Contributor

@mrry mrry left a comment

Choose a reason for hiding this comment

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

Neat! Thanks for making those changes. LGTM.

@mrry mrry added the kokoro:force-run Tests on submitted change label Mar 6, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 6, 2018
@yongtang
Copy link
Copy Markdown
Member Author

yongtang commented Mar 6, 2018

Thanks @mrry for the help 👍 . Some of the tests are failing. Seems to be some difference between clang and gcc (it runs fine on my local machine with gcc + ubuntu 16.04). I am looking into the issue now.

@yongtang yongtang force-pushed the 17210-KafkaDataset branch from eddcba0 to 3f19308 Compare March 6, 2018 23:18
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang yongtang force-pushed the 17210-KafkaDataset branch from 3f19308 to 7bd4881 Compare March 6, 2018 23:29
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang yongtang force-pushed the 17210-KafkaDataset branch 2 times, most recently from 6699684 to a3e4a55 Compare March 7, 2018 01:44
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang yongtang force-pushed the 17210-KafkaDataset branch from a3e4a55 to c1eaba4 Compare March 7, 2018 02:16
@yongtang
Copy link
Copy Markdown
Member Author

yongtang commented Mar 7, 2018

The reason of the previous test failure was that, includes in BUILD file adds include path with -isystem (vs. -I). That makes clang unhappy. The issue has been fixed by replacing includes with copt.

All tests passed now 🎉

@gunan gunan merged commit 37f6d22 into tensorflow:master Mar 8, 2018
@yongtang yongtang deleted the 17210-KafkaDataset branch March 8, 2018 16:45
StanislawAntol pushed a commit to StanislawAntol/tensorflow that referenced this pull request Mar 23, 2018
* Fix build issue with KafkaDataset

This fix tries to address the issue raised in 17210 where
error of `NotFoundError: Op type not registered 'KafkaDataset' in binary.`
returned from kafka ops. The issue was that the inclusion of kafka ops
was removed due to the conflict merge from the other PR. This fix
fixes the issue.

This fix fixes 17210.

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

* Change `import readers.Dataset` to `import dataset_ops.Dataset`,

due to the changes in some other places.

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

* Fix library dependency issues in bazel

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

* Add dependency to bazel rules

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

* Add license to lib and pip package

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

* Remove unneeded changes in bazel

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

* Address review feedback

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

* Fix sanity check

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

* Add zlib dependency and include path

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

* Add copts in bazel to address the discrepancy in clang and gcc

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NotFoundError: Op type not registered 'KafkaDataset' in binary.

5 participants