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

[C++ Frontend] Kaiming Initialization #14718

Closed
wants to merge 20 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@JoshVarty
Copy link
Contributor

JoshVarty commented Dec 3, 2018

/cc @goldsborough

Working on #14582

The corresponding python implementations are at: pytorch/torch/nn/init.py

Here is my initial implementation of Kaiming Initialization. I have not been able to figure out how to successfully run tests locally so I haven't added any yet.

A couple questions:

  • Are the enums defined in the right place? I copied their names from Python, but do you prefer different naming conventions for C++?
  • To run tests locally do I use python setup.py test? Can I run just a subset of the tests somehow?
  • Should I add my tests at test/cpp/api/misc.cpp?

@JoshVarty JoshVarty requested review from ebetica and goldsborough as code owners Dec 3, 2018

@goldsborough
Copy link
Contributor

goldsborough left a comment

When you build PyTorch from source (see CONTRIBUTING.md), there will be a test_api binary in the build folder (use find test_api if you can't find it). You can filter tests by using `test_api --gtest_filter="TestSuite.TestCase" -- see https://github.com/abseil/googletest/blob/master/googletest/docs/primer.md. You'd want to add your test here

Show resolved Hide resolved torch/csrc/api/include/torch/nn/init.h Outdated
Show resolved Hide resolved torch/csrc/api/include/torch/nn/init.h Outdated
Show resolved Hide resolved torch/csrc/api/include/torch/nn/init.h Outdated
Show resolved Hide resolved torch/csrc/api/src/nn/init.cpp Outdated
Show resolved Hide resolved torch/csrc/api/src/nn/init.cpp
Show resolved Hide resolved torch/csrc/api/src/nn/init.cpp Outdated
Show resolved Hide resolved torch/csrc/api/src/nn/init.cpp Outdated
Show resolved Hide resolved torch/csrc/api/src/nn/init.cpp Outdated

@goldsborough goldsborough changed the title WIP: Kaiming Initialization for C++ API [WIP][C++ Frontend] Kaiming Initialization Dec 4, 2018

@goldsborough goldsborough added the cpp label Dec 4, 2018

@ezyang

This comment has been minimized.

Copy link
Contributor

ezyang commented Dec 6, 2018

Hi @goldsborough, any update on this PR?

@JoshVarty

This comment has been minimized.

Copy link
Contributor Author

JoshVarty commented Dec 6, 2018

Sorry for the delay, I've made the changes but haven't quite figured out how to run the tests. I'll try to get them running ASAP. I'd be interested in adding tests to this PR.

@goldsborough

This comment has been minimized.

Copy link
Contributor

goldsborough commented Dec 6, 2018

@JoshVarty where are you getting stuck? Have you located the test_api binary? You can write your test here

@JoshVarty

This comment has been minimized.

Copy link
Contributor Author

JoshVarty commented Dec 6, 2018

@goldsborough I can't seem to find the binary, but I do see a folder called test_api/.

I'm running:

python setup.py build
cd build
find test_api

Which returns:

test_api/
test_api/CMakeFiles
test_api/CMakeFiles/progress.marks
test_api/CMakeFiles/CMakeDirectoryInformation.cmake
test_api/CMakeFiles/test_api.dir
test_api/CMakeFiles/test_api.dir/tensor.cpp.o
test_api/CMakeFiles/test_api.dir/build.make
test_api/CMakeFiles/test_api.dir/module.cpp.o
test_api/CMakeFiles/test_api.dir/DependInfo.cmake
test_api/CMakeFiles/test_api.dir/ordered_dict.cpp.o
test_api/CMakeFiles/test_api.dir/depend.internal
test_api/CMakeFiles/test_api.dir/tensor_options_cuda.cpp.o
test_api/CMakeFiles/test_api.dir/misc.cpp.o
test_api/CMakeFiles/test_api.dir/integration.cpp.o
test_api/CMakeFiles/test_api.dir/depend.make
test_api/CMakeFiles/test_api.dir/dataloader.cpp.o
test_api/CMakeFiles/test_api.dir/static.cpp.o
test_api/CMakeFiles/test_api.dir/link.txt
test_api/CMakeFiles/test_api.dir/__
test_api/CMakeFiles/test_api.dir/__/common
test_api/CMakeFiles/test_api.dir/__/common/main.cpp.o
test_api/CMakeFiles/test_api.dir/expanding-array.cpp.o
test_api/CMakeFiles/test_api.dir/progress.make
test_api/CMakeFiles/test_api.dir/cmake_clean.cmake
test_api/CMakeFiles/test_api.dir/serialize.cpp.o
test_api/CMakeFiles/test_api.dir/sequential.cpp.o
test_api/CMakeFiles/test_api.dir/tensor_cuda.cpp.o
test_api/CMakeFiles/test_api.dir/tensor_options.cpp.o
test_api/CMakeFiles/test_api.dir/jit.cpp.o
test_api/CMakeFiles/test_api.dir/CXX.includecache
test_api/CMakeFiles/test_api.dir/any.cpp.o
test_api/CMakeFiles/test_api.dir/parallel.cpp.o
test_api/CMakeFiles/test_api.dir/optim.cpp.o
test_api/CMakeFiles/test_api.dir/memory.cpp.o
test_api/CMakeFiles/test_api.dir/modules.cpp.o
test_api/CMakeFiles/test_api.dir/rnn.cpp.o
test_api/CMakeFiles/test_api.dir/flags.make
test_api/cmake_install.cmake
test_api/CTestTestfile.cmake
test_api/Makefile

P.S. I believe there's a PyTorch Slack right? If it's easier, we could discuss this build stuff there. I sent a couple emails to slack@pytorch.org last week but I haven't heard back.

@goldsborough

This comment has been minimized.

Copy link
Contributor

goldsborough commented Dec 6, 2018

You should run python setup.py build develop and then run find . -name test_api, find test_api just lists that folder. I think the binary is in build/bin/test_api. @soumith can you check about the slack invite?

@JoshVarty

This comment has been minimized.

Copy link
Contributor Author

JoshVarty commented Dec 6, 2018

I wanted to reimplement these tests in C++ but I'm having trouble because those tests use scipy.stats.kstest to compare two distributions against one another.

Is there a similar implementation in C++? Or would I essentially be looking at re-implementing this SciPy functionality? If I'd have to reimplement it, it may be better to create a new issue and discuss that approach there. One positive is that it would also allow us to test the xavier_* weight initialization methods as well.

Show resolved Hide resolved test/cpp/api/misc.cpp Outdated
@goldsborough

This comment has been minimized.

Copy link
Contributor

goldsborough commented Dec 7, 2018

@JoshVarty One thing you can do is verify that the output in C++ matches that in Python. We do this for the optimizers for example: https://github.com/pytorch/pytorch/blob/master/test/cpp/api/optim_baseline.py

For this, you'd write a small PyTorch script that generates a C++ header file containing some constants for every test case, and then include that header in the C++ test file, and compare the output of the same code in C++ to the Python outputs you generated. What do you think of this idea?

Show resolved Hide resolved torch/csrc/api/include/torch/nn/init.h Outdated
Show resolved Hide resolved torch/csrc/api/include/torch/nn/init.h Outdated
Show resolved Hide resolved torch/csrc/api/src/nn/init.cpp Outdated
Show resolved Hide resolved torch/csrc/api/src/nn/init.cpp Outdated

JoshVarty added some commits Dec 7, 2018

@ezyang

This comment has been minimized.

Copy link
Contributor

ezyang commented Dec 20, 2018

@goldsborough is this ready to go?

@goldsborough

This comment has been minimized.

Copy link
Contributor

goldsborough commented Jan 3, 2019

@ezyang It is not because there are no tests yet. @JoshVarty are you still interested in wrapping up this PR by adding some tests? It looks great so far, thanks for your work.

@JoshVarty

This comment has been minimized.

Copy link
Contributor Author

JoshVarty commented Jan 7, 2019

Yeah I would like to add the tests. I haven't had much time to look at this over the holidays but I anticipate I'll have some free time this week to look at it again.

@JoshVarty JoshVarty requested a review from yf225 as a code owner Jan 29, 2019

@JoshVarty

This comment has been minimized.

Copy link
Contributor Author

JoshVarty commented Jan 30, 2019

@fmassa If you guys are comfortable writing the tests, feel free. I haven't been able to build since merging master into this branch and can't seem to figure out what's wrong. :(

@goldsborough

This comment has been minimized.

Copy link
Contributor

goldsborough commented Jan 30, 2019

@JoshVarty What do your build failures look like? If all else fails re-cloning your fork and re-building from scratch should work.

@JoshVarty

This comment has been minimized.

Copy link
Contributor Author

JoshVarty commented Feb 1, 2019

I'm not sure why but for some reason it no longer builds. What I'm running is:

git clone --recursive git@github.com:JoshVarty/pytorch.git

cd pytorch

git checkout KaimingInitCpp

python setup.py build develop

(I should note that I get the same errors when trying to build current pytorch master.)

After compiling 56% of the project I get the following:

REMOVED to help with issue length.

I'm not sure why it stopped working on my machine once I pulled in master, perhaps I changed something locally with my build tools but it was working because certain build steps were cached before?

@ShahriarSS

This comment has been minimized.

Copy link

ShahriarSS commented Feb 1, 2019

Hi.
I am able to build and test the code so if there is anything I can do to speed this along please let me know.

@JoshVarty

This comment has been minimized.

Copy link
Contributor Author

JoshVarty commented Feb 2, 2019

I wiped my Ubuntu partition and started from scratch and that seems to have got everything working again. I've started the baselines and will create the C++ tests shortly.

@yf225

This comment has been minimized.

Copy link
Contributor

yf225 commented Feb 4, 2019

@JoshVarty Could you pull master into your branch to fix the macos test errors? Thanks :)

@JoshVarty

This comment has been minimized.

Copy link
Contributor Author

JoshVarty commented Feb 5, 2019

@yf225 I merged master and the tests re-ran but they seem to fail on linux.

@yf225

This comment has been minimized.

Copy link
Contributor

yf225 commented Feb 5, 2019

@JoshVarty the current CI errors are unrelated and can be ignored

@goldsborough @fmassa do you think the tests look good and the PR is ready to go?

@goldsborough
Copy link
Contributor

goldsborough left a comment

Looks great @JoshVarty, thanks! I left a few comments on things that could be improved, like the lambdas in init.cpp and moving the other nn::init-related tests into init.cpp. I'll approve it already, and merge it once you have time for the last few fixes

Show resolved Hide resolved test/cpp/api/init.cpp
Show resolved Hide resolved test/cpp/api/init.cpp
Show resolved Hide resolved test/cpp/api/init.cpp Outdated
Show resolved Hide resolved test/cpp/api/init.cpp Outdated
Show resolved Hide resolved test/cpp/api/init.cpp Outdated
Show resolved Hide resolved test/cpp/api/init.cpp
Show resolved Hide resolved test/cpp/api/misc.cpp Outdated
@goldsborough

This comment has been minimized.

Copy link
Contributor

goldsborough commented Feb 7, 2019

image

@JoshVarty

This comment has been minimized.

Copy link
Contributor Author

JoshVarty commented Feb 12, 2019

@goldsborough This is probably good for another review if you get a minute. I think the only outstanding issue is my comment/question about how to handle the lambdas.

@goldsborough

This comment has been minimized.

Copy link
Contributor

goldsborough commented Feb 12, 2019

Looks great, I'll land it tomorrow. Thanks again!

@facebook-github-bot
Copy link
Contributor

facebook-github-bot left a comment

@goldsborough is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@goldsborough goldsborough changed the title [WIP][C++ Frontend] Kaiming Initialization [C++ Frontend] Kaiming Initialization Feb 15, 2019

@facebook-github-bot
Copy link
Contributor

facebook-github-bot left a comment

@goldsborough is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot left a comment

@goldsborough is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@JoshVarty JoshVarty deleted the JoshVarty:KaimingInitCpp branch Feb 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment