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.

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

update Defun doc to clarify that definitions are frozen at first .run call #6835

Merged
merged 2 commits into from Feb 12, 2017

Conversation

yaroslavvb
Copy link
Contributor

Fixes #6804

@tensorflow-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@yaroslavvb yaroslavvb changed the title update Defun doc update Defun doc to clarify that definitions are frozen at first .run call Jan 13, 2017
@drpngx
Copy link
Contributor

drpngx commented Jan 13, 2017

Jenkins, test this please.

Copy link
Contributor

@zffchen78 zffchen78 left a comment

Choose a reason for hiding this comment

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

Thank you for the update, Yaralsav.

@@ -748,6 +748,9 @@ def foo(x, y):
default graph and adds the definition of the function into the
default graph. Because the addition of the function into the graph
is deferred, the decorator can be used anywhere in the program.

Definitions of function is frozen for the session after first session.run
Copy link
Contributor

Choose a reason for hiding this comment

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

How about

Definitions of functions are frozen in a graph as soon as the graph is used to create a session.
Therefore, nodes using the function must be created in the graph before the corresponding session
is created.

Maybe then add an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That doesn't seem to match current behavior -- I can create a session, then create function node, and then issue first session.run and it works, still want me to change it to that?

@drpngx drpngx added the stat:awaiting response Status - Awaiting response from author label Jan 13, 2017
@yaroslavvb yaroslavvb added stat:awaiting tensorflower Status - Awaiting response from tensorflower and removed stat:awaiting response Status - Awaiting response from author labels Jan 14, 2017
@drpngx
Copy link
Contributor

drpngx commented Jan 24, 2017

We need to wait two weeks on that one, sorry about that.

@rmlarsen
Copy link
Member

@zffchen78 @drpngx Do you have additional comments? Can we merge this or are we still in the two week waiting period mentioned above?

@yaroslavvb
Copy link
Contributor Author

changed to the wording suggested by zf

@rmlarsen
Copy link
Member

Thanks @yaroslavvb. @zffchen78 can you take another look, please?

@drpngx
Copy link
Contributor

drpngx commented Feb 12, 2017

Jenkins, test this please.

@drpngx drpngx added awaiting testing (then merge) and removed stat:awaiting tensorflower Status - Awaiting response from tensorflower labels Feb 12, 2017
@drpngx drpngx merged commit 4fbf69e into tensorflow:master Feb 12, 2017
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.

None yet

6 participants