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

Implement forward_all function that performs forwarding on mulptiple variables at once #243

Merged
merged 10 commits into from Jan 29, 2019

Conversation

takuseno
Copy link
Contributor

@takuseno takuseno commented Sep 11, 2018

Hi, @TE-TakuyaNarihira san.

I added new function forward_all.

Formerly, forwarding multiple outputs which shared hidden layers (just like policy and value of actor-critic) requires multiple forwardings in static graph. See the example below.

x = nn.Variable((3, 3))
h = PF.affine(x, 3, name='hidden')
y1 = PF.affine(h, 3, name='y1')
y2 = PF.affine(h, 3, name='y2')

y1.forward()
y2.forward() # hidden layer was computed twice!!

This is critical at huge architectures.

Thus, I added forward_all function.

x = nn.Variable((3, 3))
h = PF.affine(x, 3, name='hidden')
y1 = PF.affine(h, 3, name='y1')
y2 = PF.affine(h, 3, name='y2')

# compute h only once!
nn.forward_all([y1, y2])

This function performs forwarding with shared fclosed, which prevents shared layers from being revisited.

What do you think of this?

Copy link
Member

@AkioHayakawa-sony AkioHayakawa-sony 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 your PR! We think you made a good point.
While your code looks good for the most part, we have two suggestions that may further improve it.

First, GIL should be released when forward_all is running.
It is not desirable that other python threads are paused while forward_all is running.
So, could you make it callable without GIL as we suggested?

Second, could you write a test code for forward_all function?
We would like to make sure that forward_all works correctly, especially for the networks with more complicated architectures, such as skip, branching and merging connections.
It may be helpful to reference the test code for normal forward function at https://github.com/sony/nnabla/blob/master/python/test/test_graph.py

We thank you again for your contribution.

python/src/nnabla/_computation_graph.pxd Outdated Show resolved Hide resolved
python/src/nnabla/_computation_graph.pyx Outdated Show resolved Hide resolved
@takuseno
Copy link
Contributor Author

takuseno commented Oct 3, 2018

@TE-AkioHayakawa Hi, Hayakawa san, it's been a week!
Thank you for reviewing my PR! I will apply all of your suggestions in a week!

@takuseno
Copy link
Contributor Author

@TE-AkioHayakawa Hi, Hayakawa san!
I've implemented the unit test! All tests have passed on my machine!

@AkioHayakawa-sony
Copy link
Member

Sorry for late reply.
I will review your code in about a week.

Thank you for your commitment!

@AkioHayakawa-sony
Copy link
Member

Test failed in python 2.7.15 environment. (test_graph_clear_buffer, test_graph_rewire).

@takuseno
Copy link
Contributor Author

Thank you for reviewing my code! I'll check unit tests with Python 2.

@AkioHayakawa-sony
Copy link
Member

Sorry, I might be wrong. This problem is not related to python 2.

I found that the result gradients are not correct when CpuArray is used (not CpuCachedArray) and backward() is called with clear_buffer=True.

@takuseno
Copy link
Contributor Author

I think I got your point. Unlike F.sink, doing backward on several roots with clear_buffer=True is difficult as the first backward clears variables that the other backward actually needs. This might force users to understand this behaviour.

@AkioHayakawa-sony
Copy link
Member

CI test has passed.

@AkioHayakawa-sony
Copy link
Member

@takuseno I will merge your pull request as soon as possible.
Thank you again for all your contributions!!

@AkioHayakawa-sony AkioHayakawa-sony merged commit d7b032b into sony:master Jan 29, 2019
@takuseno
Copy link
Contributor Author

@TE-AkioHayakawa Thank you!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants