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

add option for lmdb #431

Merged
merged 9 commits into from
Jun 16, 2014
Merged

add option for lmdb #431

merged 9 commits into from
Jun 16, 2014

Conversation

mavenlin
Copy link
Contributor

This is to replace PR #386.

@shelhamer
Copy link
Member

Thanks for the rebase and making this optional! However, it doesn't build. Perhaps a missing include?

src/caffe/layers/data_layer.cpp:206:24: error: use of undeclared identifier 'MDB_NOTLS'
        MDB_RDONLY|MDB_NOTLS, 0664), MDB_SUCCESS) << "mdb_env_open failed";

@mavenlin
Copy link
Contributor Author

@shelhamer I did include lmdb.h in the data_layer.hpp file.
Strange it does not complain about lmdb.h not found but instead MDB_NOTLS undeclared.
It builds on my machine though.

@sergeyk
Copy link
Contributor

sergeyk commented May 21, 2014

Yeah I'm trying to figure out what's going on with the homebrew lmdb. Its header file does not include MDB_NOTLS flag, but all other ones I can find on the internet do. Trying to build lmdb from source right now

@shelhamer
Copy link
Member

Alright, this works fine on Ubuntu 12.04, although I had to install from source since there isn't a package:

git clone git://gitorious.org/mdb/mdb.git
cd mdb/libraries/liblmdb
make && sudo make install

@shelhamer
Copy link
Member

@sergeyk if you get lmdb to install right on osx please PR it to homebrew.

@sergeyk
Copy link
Contributor

sergeyk commented May 21, 2014

Okay, I got lmdb to work correctly with this brew recipe: https://gist.github.com/sergeyk/bb8274c218ada01d1fea

@shelhamer could you check that this works for you?

@sergeyk
Copy link
Contributor

sergeyk commented May 21, 2014

@mavenlin Great PR!

One last thing: could you include a test for the lmdb functionality? It should go into test_data_layer.cpp; you basically need to add a FillLMDB method, analogous to FillLevelDB, and then include some cases.

Bonus contributor points if you also adjust the docs/installation.md to note the new dependency on lmdb

@shelhamer
Copy link
Member

@sergeyk your formula works for me, and clones from the same repo I built on linux. I dropped my homebrew fork to silence some notifications, could you PR it?

@sergeyk
Copy link
Contributor

sergeyk commented May 21, 2014

@shelhamer yup

@sergeyk
Copy link
Contributor

sergeyk commented Jun 1, 2014

@mavenlin As soon as you include a test for the lmdb functionality (see my comment above), we will merge. I like LMDB.

@mavenlin
Copy link
Contributor Author

mavenlin commented Jun 2, 2014

Hi @sergeyk, I've been busy with my project these days, I'll get back asap.

@sergeyk
Copy link
Contributor

sergeyk commented Jun 2, 2014

NIPS time, we're in the same boat. Thanks!

@mavenlin
Copy link
Contributor Author

@sergeyk I've updated the tests, basically just copied from test_data_layer, changed FillLevelDB to FillLMDB.

@shelhamer
Copy link
Member

lmdb now properly installs by homebrew as of Homebrew/legacy-homebrew@182654e

@jeffdonahue
Copy link
Contributor

@sergeyk fyi I did some testing and cleanup of this last night, looks good to merge to me.

@jeffdonahue
Copy link
Contributor

(Though ideally we'd have an option in convert_imageset.cpp to output to LMDB, which I think @mavenlin said he was working on.)

@mavenlin
Copy link
Contributor Author

Tools added, if anything missing, please let me know.

@sergeyk
Copy link
Contributor

sergeyk commented Jun 16, 2014

Looks great, thanks for the great PR @mavenlin! I'm a new fan of lmdb -- seems like the best way to have multiple processes reading the same large dataset.

sergeyk added a commit that referenced this pull request Jun 16, 2014
Add support for LMDB (LevelDB alternative)
@sergeyk sergeyk merged commit b50f591 into BVLC:dev Jun 16, 2014
@kloudkl
Copy link
Contributor

kloudkl commented Jun 17, 2014

I rebased #478 and found that the steps to install lmdb were not added in the documentation. @sergeyk, could you fix it?

@sguada
Copy link
Contributor

sguada commented Jun 17, 2014

@sergeyk @mavenlin, now we need to add one more requirement to the installation.

Maybe we should define a way to make new requirements optional.

@mavenlin
Copy link
Contributor Author

@sguada @kloudkl @sergeyk
I think there are several ways:

  1. create git submodules, put lmdb as a submodule of caffe, so that it will be automatically checkout and made during make of caffe. Other dependencies managed by git can also be add as submodules.
  2. create a plugin system for caffe, so that database backends can be added without changing the main code of caffe. The plugin system can extend to other components of caffe, for example, layers.
  3. define a macro, so that lmdb backend will not be made if the user's system does not support.
  4. create a ppa for lmdb, so that it can be install the same way as all the other components through apt-get.

@sergeyk
Copy link
Contributor

sergeyk commented Jun 18, 2014

(3) is easiest, (2) is best. @mavenlin want to take a stab at (3)?

@mavenlin
Copy link
Contributor Author

@sergeyk no problems

@kloudkl
Copy link
Contributor

kloudkl commented Jun 18, 2014

Likewise, OpenCL is optional in #415. It's up to the users to enable it explicitly.

@sguada
Copy link
Contributor

sguada commented Jun 19, 2014

@mavenlin could you also add the instructions to install lmdb to the documentation?

@hyc
Copy link

hyc commented Jun 20, 2014

LMDB is in Ubuntu repos already... https://launchpad.net/ubuntu/+source/lmdb

@mavenlin
Copy link
Contributor Author

@hyc but in trusty tahr only?
My group member and I tried trusty tahr, but it turns out to be very slow running caffe.
It might be due to the driver, we tried a whole bunch of drivers, but none work out.

@hyc
Copy link

hyc commented Jun 20, 2014

What kernel is in trusty tahr? We've hit a lot of performance issues in recent kernels. http://www.openldap.org/lists/openldap-technical/201312/msg00133.html
http://www.openldap.org/lists/openldap-devel/201309/msg00008.html

@mavenlin
Copy link
Contributor Author

@hyc We also tried downgrading the kernel to older ones, but failed. We didn't explore more on this but just downgrade the whole system to 13.10.

@mavenlin
Copy link
Contributor Author

@hyc Oh, I didn't mean that lmdb is slowed down on 14.04, either with leveldb and lmdb the performance got very slow. I think it is cuda, as previous issues pointed out that some versions of nvidia drivers cause caffe to be slow. Only a limited number of nvidia drivers can be installed on 14.04, older versions usually fail the installation.

@shelhamer
Copy link
Member

@mavenlin the installation docs still need updating to include lmdb for at least Ubuntu 12.04 and OS X since these are the official platforms (and ideally more recent Ubuntu versions too). Thanks.

@shelhamer shelhamer mentioned this pull request Aug 8, 2014
mitmul pushed a commit to mitmul/caffe that referenced this pull request Sep 30, 2014
Add support for LMDB (LevelDB alternative)
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.

7 participants