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

Add is_closed() method to Queue #10175

Merged
merged 14 commits into from Jun 28, 2017

Conversation

Projects
None yet
10 participants
@lattas
Copy link
Contributor

lattas commented May 24, 2017

to show if a Queue of any type is closed or not, mainly for debugging purposes. Fixes #7355.

@tensorflow-jenkins

This comment has been minimized.

Copy link
Collaborator

tensorflow-jenkins commented May 24, 2017

Can one of the admins verify this patch?

@googlebot googlebot added the cla: yes label May 24, 2017

@maciekcc maciekcc requested a review from keveman May 26, 2017

@maciekcc

This comment has been minimized.

Copy link
Contributor

maciekcc commented May 26, 2017

Jenkins test this please.

@lattas

This comment has been minimized.

Copy link
Contributor

lattas commented May 31, 2017

@girving, you may also be interested to take a look since you discussed this on #7355

@@ -77,6 +77,9 @@ class QueueInterface : public ResourceBase {
virtual void Close(OpKernelContext* ctx, bool cancel_pending_enqueues,
DoneCallback callback) = 0;

// Returns true if a given queue is closed and false if it is open.
virtual bool closed() = 0;

This comment has been minimized.

@girving

girving May 31, 2017

Contributor

Should probably be const.

This comment has been minimized.

@lattas

lattas Jun 1, 2017

Contributor

done.

//
// The op has one input, which is the handle of the appropriate Queue;
// and one output, which is a scalar boolean value that shows whether
// the queue is closed.

This comment has been minimized.

@girving

girving May 31, 2017

Contributor

Remove this comment: the REGISTER_OP documentation should cover this. This kind of comment is in danger of going out of date.

This comment has been minimized.

@lattas

lattas Jun 1, 2017

Contributor

done.

lattas added some commits Jun 1, 2017

@lattas lattas force-pushed the lattas:queuebase-is-closed branch from 2573f5a to c80d00e Jun 1, 2017

@@ -78,7 +78,7 @@ class QueueInterface : public ResourceBase {
DoneCallback callback) = 0;

// Returns true if a given queue is closed and false if it is open.
virtual bool closed() = 0;
virtual const bool closed() = 0;

This comment has been minimized.

@girving

girving Jun 1, 2017

Contributor

Should be

virtual bool closed() const = 0;

Making return values const doesn't do anything in C++.

This comment has been minimized.

@lattas

lattas Jun 1, 2017

Contributor

done.

@girving

This comment has been minimized.

Copy link
Contributor

girving commented Jun 1, 2017

Jenkins, test this please.

@girving

This comment has been minimized.

Copy link
Contributor

girving commented Jun 1, 2017

@cwhipkey Looks like the tests failed for tool reasons. Should I ping you or someone else?

@gunan

This comment has been minimized.

Copy link
Member

gunan commented Jun 1, 2017

Both failures seem to be issues on infra side.
I took the bad machines offline. now should be better.

Jenkins, test this please.

@girving

girving approved these changes Jun 1, 2017

@girving

This comment has been minimized.

Copy link
Contributor

girving commented Jun 1, 2017

@martinwicke Who merges these days?

@martinwicke

This comment has been minimized.

Copy link
Member

martinwicke commented Jun 1, 2017

This week, @andrewharp.

@martinwicke

This comment has been minimized.

Copy link
Member

martinwicke commented Jun 1, 2017

Should get API review.

@@ -77,6 +77,9 @@ class QueueInterface : public ResourceBase {
virtual void Close(OpKernelContext* ctx, bool cancel_pending_enqueues,
DoneCallback callback) = 0;

// Returns true if a given queue is closed and false if it is open.
virtual bool closed() const = 0;

This comment has been minimized.

@josh11b

josh11b Jun 12, 2017

Member

For consistency, this should be "is_closed", not "closed"

This comment has been minimized.

@lattas

lattas Jun 13, 2017

Contributor

done.

@josh11b

This comment has been minimized.

Copy link
Member

josh11b commented Jun 12, 2017

This is fine for API review assuming that one change is made.

lattas added some commits Jun 13, 2017

@martinwicke

This comment has been minimized.

Copy link
Member

martinwicke commented Jun 14, 2017

Jenkins, test this please.

@keveman keveman removed their request for review Jun 14, 2017

@gunan

This comment has been minimized.

Copy link
Member

gunan commented Jun 14, 2017

Is it possible the PR authors local version is out of sync with the master?
Maybe a pull is needed first?

lattas added some commits Jun 14, 2017

@lattas

This comment has been minimized.

Copy link
Contributor

lattas commented Jun 14, 2017

@gunan Done.

@gunan

This comment has been minimized.

Copy link
Member

gunan commented Jun 14, 2017

Jenkins, test this please.

@martinwicke

This comment has been minimized.

Copy link
Member

martinwicke commented Jun 14, 2017

@gunan, no luck.

@lattas

This comment has been minimized.

Copy link
Contributor

lattas commented Jun 14, 2017

If I have understood the api-compatibility-test correctly, it will fail at any change on the API, except when it is told to update the goldens. Following the instructions in its README, I ran the command locally and that should allow only the next api-compatibility-test to succeed with API changes, by creating a temporary text file in the same folder. My question is, is the CI aware of this?

Also note that I added is_closed to Queue_Base goldens myself. It was not updated automatically. Any thoughts?

@martinwicke

This comment has been minimized.

Copy link
Member

martinwicke commented Jun 14, 2017

@lattas

This comment has been minimized.

Copy link
Contributor

lattas commented Jun 14, 2017

I updated (hardcodded) myself the pbtxt file for QueueBase only. Running

    $ bazel build tensorflow/tools/api/tests:api_compatibility_test
    $ bazel-bin/tensorflow/tools/api/tests/api_compatibility_test \
          --update_goldens True

did not update them. Should I update it for all Queue Types or all should be updated automatically?

@martinwicke

This comment has been minimized.

Copy link
Member

martinwicke commented Jun 14, 2017

@lattas

This comment has been minimized.

Copy link
Contributor

lattas commented Jun 14, 2017

Thank you for the feedback. I will look into it and come up with an answer shortly.

@gunan

This comment has been minimized.

Copy link
Member

gunan commented Jun 16, 2017

I think The problem is still related to syncing.
I checked out your branch, and the test is failing the same way for me.
I think what you needed to do was, first fetch and merge all changes from tensorflow/tensorflow:master. You did this. But you needed to rerun api compatibility test and update goldens again. I can confirm that, when I run api compatibility test on your branch, I see this failure:

ERROR:tensorflow:Issue 1	: Change detected in python object: tensorflow.PaddingFIFOQueue.
ERROR:tensorflow:Issue 2	: Change detected in python object: tensorflow.PriorityQueue.
ERROR:tensorflow:Issue 3	: Change detected in python object: tensorflow.RandomShuffleQueue.
ERROR:tensorflow:Issue 4	: Change detected in python object: tensorflow.QueueBase.
ERROR:tensorflow:Issue 5	: Change detected in python object: tensorflow.FIFOQueue.

Then I ran update goldens script. Which made the following modifications:

	modified:   tensorflow/tools/api/golden/tensorflow.-f-i-f-o-queue.pbtxt
	modified:   tensorflow/tools/api/golden/tensorflow.-padding-f-i-f-o-queue.pbtxt
	modified:   tensorflow/tools/api/golden/tensorflow.-priority-queue.pbtxt
	modified:   tensorflow/tools/api/golden/tensorflow.-queue-base.pbtxt
	modified:   tensorflow/tools/api/golden/tensorflow.-random-shuffle-queue.pbtxt

Then rerunning api compatibility test returns success.

Ran 2 tests in 1.080s

OK
@drpngx

This comment has been minimized.

Copy link
Member

drpngx commented Jun 26, 2017

@a-lattas any luck with that?

@lattas

This comment has been minimized.

Copy link
Contributor

lattas commented Jun 26, 2017

I apologise for the delay, I am away from home for a few days.

I can see on @gunan 's post that the api-compatibility-test is failing and running it with update-goldens let's it progress.

However, when running

    $ bazel build tensorflow/tools/api/tests:api_compatibility_test

I get something like

Ran 2 tests in 0.080s (skipped 1)
OK

with no failure message and

    $ bazel-bin/tensorflow/tools/api/tests/api_compatibility_test \
          --update_goldens True

does not make any chaninges to the goldens.

I have not found the reason why. skipped 1 could mean that it is skipping the test? Also, I am not using Docker, as it is not described as necessary. Any ideas?

@gunan

This comment has been minimized.

Copy link
Member

gunan commented Jun 26, 2017

It is certainly possible.
Api compatibility test runs on Linux and python 2 only, as different versions of dependencies can trigger failures in the api object names. Is it possible you are on a macbook, or using python3?

@lattas

This comment has been minimized.

Copy link
Contributor

lattas commented Jun 26, 2017

I suppose this is the issue then. I am using Ubuntu, but with python 3.

I will run the test soon with python 2 and inform you about its result.

Thank you.

@martinwicke martinwicke removed the API review label Jun 26, 2017

@lattas

This comment has been minimized.

Copy link
Contributor

lattas commented Jun 27, 2017

@gunan Thanks!
I did with python2 . Everything worked as expected and goldens got automatically updated. It should be ready for testing.

@girving

This comment has been minimized.

Copy link
Contributor

girving commented Jun 27, 2017

Jenkins, test this please.

@girving

This comment has been minimized.

Copy link
Contributor

girving commented Jun 27, 2017

The only failure looks unrelated. @drpngx: Ready for merge!

@drpngx drpngx merged commit a4f18c3 into tensorflow:master Jun 28, 2017

9 of 11 checks passed

MacOS CPU Tests FAILURE
Details
ci.tensorflow.org FAILURE
Details
Android Demo App SUCCESS
Details
Linux CPU Tests SUCCESS
Details
Linux CPU Tests (Python 3) SUCCESS
Details
Linux CPU Tests Makefile SUCCESS
Details
Linux GPU SUCCESS
Details
Linux XLA SUCCESS
Details
Sanity Checks SUCCESS
Details
Windows Cmake Tests SUCCESS
Details
cla/google All necessary CLAs are signed

@lattas lattas deleted the lattas:queuebase-is-closed branch Jun 28, 2017

allenlavoie pushed a commit to allenlavoie/tensorflow that referenced this pull request Jul 15, 2017

Add is_closed() method to Queue (tensorflow#10175)
* add QueueBase.is_closed(). Fixes tensorflow#7355

* Make closed() const

* Remove unecessary documentation

* Make closed() const pure virtual method

* Rename closed to is_closed

* Change all Queue to queue in documentation

* Update API to include Queue_Base.is_closed

* Recall change on goldens

* Update API goldens for Queuebase is_closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment