Skip to content

[CMake] Remove invalid python modules#15603

Merged
drpngx merged 3 commits into
tensorflow:masterfrom
Androbin:invalid
Dec 28, 2017
Merged

[CMake] Remove invalid python modules#15603
drpngx merged 3 commits into
tensorflow:masterfrom
Androbin:invalid

Conversation

@Androbin

@Androbin Androbin commented Dec 23, 2017

Copy link
Copy Markdown
Contributor

Split from #15368
@mrry Friendly ping.

@tensorflow-jenkins

tensorflow-jenkins commented Dec 23, 2017

Copy link
Copy Markdown
Collaborator

Can one of the admins verify this patch?

@drpngx drpngx requested a review from mrry December 25, 2017 02:25
@drpngx drpngx added awaiting review Pull request awaiting review kokoro:run labels Dec 25, 2017
@drpngx

drpngx commented Dec 26, 2017

Copy link
Copy Markdown
Contributor

Jenkins, test this please.

@Androbin

Androbin commented Dec 27, 2017

Copy link
Copy Markdown
Contributor Author

The last CI build failed due to newly included tensorflow/contrib/lite/python importing tensorflow/contrib/lite/toco/python. @drpngx Please re-run the tests.

@drpngx

drpngx commented Dec 27, 2017

Copy link
Copy Markdown
Contributor

Jenkins, test this please.

@drpngx drpngx added the kokoro:force-run Tests on submitted change label Dec 27, 2017
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 27, 2017
@Androbin Androbin mentioned this pull request Dec 27, 2017
Merged
@drpngx drpngx added awaiting testing (then merge) and removed awaiting review Pull request awaiting review labels Dec 27, 2017
@Androbin

Androbin commented Dec 27, 2017

Copy link
Copy Markdown
Contributor Author

How is this import supposed to be resolved?
ImportError: No module named 'tensorflow.contrib.lite.toco.python.tensorflow_wrap_toco'

@drpngx drpngx added the kokoro:force-run Tests on submitted change label Dec 27, 2017
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 27, 2017
@drpngx

drpngx commented Dec 27, 2017

Copy link
Copy Markdown
Contributor

You need a dependency to //tensorflow/contrib/lite/toco/python:tensorflow_wrap_toco. I suggest not adding them just yet, and adding them in a separate PR.

@drpngx drpngx added stat:awaiting response Status - Awaiting response from author and removed awaiting testing (then merge) labels Dec 27, 2017
@drpngx

drpngx commented Dec 27, 2017

Copy link
Copy Markdown
Contributor

(The _wrap library, as its name implies, is a SWIG wrapped library. It links against protobuf, which gets statically linked, so adding it is probably unsafe to add it.)

@Androbin

Androbin commented Dec 27, 2017

Copy link
Copy Markdown
Contributor Author

Aren't these two lines supposed to include python:tensorflow_wrap_toco?
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/contrib/BUILD#L60
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/contrib/lite/python/BUILD#L18

@drpngx

drpngx commented Dec 27, 2017

Copy link
Copy Markdown
Contributor

Yes, but loading that library runtime is inherently unsafe, particularly on MacOS. I'd do that in another PR.

@Androbin

Androbin commented Dec 27, 2017

Copy link
Copy Markdown
Contributor Author

Okay, I have commented them out and left a TODO.

@drpngx drpngx removed the stat:awaiting response Status - Awaiting response from author label Dec 27, 2017
@drpngx

drpngx commented Dec 27, 2017

Copy link
Copy Markdown
Contributor

Jenkins, test this please.

Thanks!

@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 27, 2017
@drpngx

drpngx commented Dec 28, 2017

Copy link
Copy Markdown
Contributor

The dreaded ERROR: Error fetching remote repo 'origin' again.

Jenkins, test this please.

@drpngx drpngx merged commit 8e64ade into tensorflow:master Dec 28, 2017
@Androbin Androbin deleted the invalid branch December 28, 2017 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mrry mrry mrry approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Androbin @tensorflow-jenkins @drpngx @mrry @googlebot @kokoro-team