Skip to content

switch data layer default source to lmdb from leveldb #1131

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.

Sign up for GitHub

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

Jump to bottom
Closed
shelhamer wants to merge 1 commit into from
Closed

switch data layer default source to lmdb from leveldb #1131

shelhamer wants to merge 1 commit into from

Conversation

@shelhamer
Copy link
Member

@shelhamer shelhamer commented Sep 22, 2014

DATA layers now default to lmdb storage instead of leveldb. See #1128 for reasoning.

**Update your model definitions if the models rely on the leveldb default for loading your input data by setting backend: LEVELDB in data_layer_param.

  • try both formats
  • warn if mismatched

@sguada
Copy link
Contributor

sguada commented Sep 22, 2014

I wouldn't change defaults of params.

On Sunday, September 21, 2014, Evan Shelhamer notifications@github.com
wrote:

DATA layers now default to lmdb storage instead of leveldb. See #1128
#1128 for reasoning.

**Update your model definitions if the models rely on the leveldb default
for loading your input data by setting backend: LEVELDB in

data_layer_param.

You can merge this Pull Request by running

git pull https://github.com/shelhamer/caffe proto-default-lmdb

Or view, comment on, or merge it at:

#1131
Commit Summary

  • switch proto default to lmdb instead of leveldb

File Changes

Patch Links:

—
Reply to this email directly or view it on GitHub
#1131.

Sergio

@shelhamer
Copy link
Member Author

shelhamer commented Sep 22, 2014

@sguada the plan is to announce it in the release like we do deprecations
and other models changes. While existing models will need to add a one-line
diff to their data layers, I think the good of a better default outweighs
the harm. The single-access issue with leveldb was an annoyance to us when
we started and I imagine is bothersome for new users too.

Given that do you still see changing the default as too problematic?

On Mon, Sep 22, 2014 at 8:25 AM, Sergio Guadarrama <notifications@github.com

wrote:

I wouldn't change defaults of params.

On Sunday, September 21, 2014, Evan Shelhamer notifications@github.com
wrote:

DATA layers now default to lmdb storage instead of leveldb. See #1128
#1128 for reasoning.

**Update your model definitions if the models rely on the leveldb
default
for loading your input data by setting backend: LEVELDB in

data_layer_param.

You can merge this Pull Request by running

git pull https://github.com/shelhamer/caffe proto-default-lmdb

Or view, comment on, or merge it at:

#1131
Commit Summary

  • switch proto default to lmdb instead of leveldb

File Changes

Patch Links:

—
Reply to this email directly or view it on GitHub
#1131.

Sergio

—
Reply to this email directly or view it on GitHub
#1131 (comment).

@sguada
Copy link
Contributor

sguada commented Sep 22, 2014

I understand, maybe in this case is okay, but in general I prefer to keep
compatibly.

The same argument would apply in the other case if anyone who wants it just
need to switch one line to LMDB in the protobuf too.

On Monday, September 22, 2014, Evan Shelhamer notifications@github.com
wrote:

@sguada the plan is to announce it in the release like we do deprecations
and other models changes. While existing models will need to add a
one-line
diff to their data layers, I think the good of a better default outweighs
the harm. The single-access issue with leveldb was an annoyance to us when
we started and I imagine is bothersome for new users too.

Given that do you still see changing the default as too problematic?

On Mon, Sep 22, 2014 at 8:25 AM, Sergio Guadarrama <
notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');

wrote:

I wouldn't change defaults of params.

On Sunday, September 21, 2014, Evan Shelhamer <notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');>
wrote:

DATA layers now default to lmdb storage instead of leveldb. See #1128
#1128 for reasoning.

**Update your model definitions if the models rely on the leveldb
default
for loading your input data by setting backend: LEVELDB in

data_layer_param.

You can merge this Pull Request by running

git pull https://github.com/shelhamer/caffe proto-default-lmdb

Or view, comment on, or merge it at:

#1131
Commit Summary

  • switch proto default to lmdb instead of leveldb

File Changes

Patch Links:

—
Reply to this email directly or view it on GitHub
#1131.

Sergio

—
Reply to this email directly or view it on GitHub
#1131 (comment).

—
Reply to this email directly or view it on GitHub
#1131 (comment).

Sergio

@jeffdonahue
Copy link
Contributor

jeffdonahue commented Sep 22, 2014

It seems like we could also just try loading it as the DB type specified by
the proto (or the default), then if that fails, try the other one -- then
nobody has to worry about it. This seems like a case where it's not really
dangerous to override the user preference IMO.
On Sep 22, 2014 9:17 AM, "Sergio Guadarrama" notifications@github.com
wrote:

I understand, maybe in this case is okay, but in general I prefer to keep
compatibly.

The same argument would apply in the other case if anyone who wants it
just
need to switch one line to LMDB in the protobuf too.

On Monday, September 22, 2014, Evan Shelhamer notifications@github.com
wrote:

@sguada the plan is to announce it in the release like we do
deprecations
and other models changes. While existing models will need to add a
one-line
diff to their data layers, I think the good of a better default
outweighs
the harm. The single-access issue with leveldb was an annoyance to us
when
we started and I imagine is bothersome for new users too.

Given that do you still see changing the default as too problematic?

On Mon, Sep 22, 2014 at 8:25 AM, Sergio Guadarrama <
notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');

wrote:

I wouldn't change defaults of params.

On Sunday, September 21, 2014, Evan Shelhamer <
notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');>
wrote:

DATA layers now default to lmdb storage instead of leveldb. See
#1128
#1128 for reasoning.

**Update your model definitions if the models rely on the leveldb
default
for loading your input data by setting backend: LEVELDB in

data_layer_param.

You can merge this Pull Request by running

git pull https://github.com/shelhamer/caffe proto-default-lmdb

Or view, comment on, or merge it at:

#1131
Commit Summary

  • switch proto default to lmdb instead of leveldb

File Changes

Patch Links:

—
Reply to this email directly or view it on GitHub
#1131.

Sergio

—
Reply to this email directly or view it on GitHub
#1131 (comment).

—
Reply to this email directly or view it on GitHub
#1131 (comment).

Sergio

—
Reply to this email directly or view it on GitHub
#1131 (comment).

@shelhamer
Copy link
Member Author

shelhamer commented Sep 22, 2014

Good call Jeff. Seems like the best case for the user and no harm is done.
We could even do away with the backend field in that case because who cares?

On Mon, Sep 22, 2014 at 9:23 AM, Jeff Donahue notifications@github.com
wrote:

It seems like we could also just try loading it as the DB type specified
by
the proto (or the default), then if that fails, try the other one -- then
nobody has to worry about it. This seems like a case where it's not really
dangerous to override the user preference IMO.
On Sep 22, 2014 9:17 AM, "Sergio Guadarrama" notifications@github.com
wrote:

I understand, maybe in this case is okay, but in general I prefer to
keep
compatibly.

The same argument would apply in the other case if anyone who wants it
just
need to switch one line to LMDB in the protobuf too.

On Monday, September 22, 2014, Evan Shelhamer notifications@github.com

wrote:

@sguada the plan is to announce it in the release like we do
deprecations
and other models changes. While existing models will need to add a
one-line
diff to their data layers, I think the good of a better default
outweighs
the harm. The single-access issue with leveldb was an annoyance to us
when
we started and I imagine is bothersome for new users too.

Given that do you still see changing the default as too problematic?

On Mon, Sep 22, 2014 at 8:25 AM, Sergio Guadarrama <
notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');

wrote:

I wouldn't change defaults of params.

On Sunday, September 21, 2014, Evan Shelhamer <
notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');>
wrote:

DATA layers now default to lmdb storage instead of leveldb. See
#1128
#1128 for reasoning.

**Update your model definitions if the models rely on the leveldb
default
for loading your input data by setting backend: LEVELDB in

data_layer_param.

You can merge this Pull Request by running

git pull https://github.com/shelhamer/caffe proto-default-lmdb

Or view, comment on, or merge it at:

#1131
Commit Summary

  • switch proto default to lmdb instead of leveldb

File Changes

Patch Links:

—
Reply to this email directly or view it on GitHub
#1131.

Sergio

—
Reply to this email directly or view it on GitHub
#1131 (comment).

—
Reply to this email directly or view it on GitHub
#1131 (comment).

Sergio

—
Reply to this email directly or view it on GitHub
#1131 (comment).

—
Reply to this email directly or view it on GitHub
#1131 (comment).

@sguada
Copy link
Contributor

sguada commented Sep 22, 2014

Maybe just showing a warning about it would be enough.

@jeffdonahue
Copy link
Contributor

jeffdonahue commented Sep 22, 2014

Well, we should keep and deprecate it rather than drop it. I'm fine with simplifying the design by ignoring it (and maybe showing a warning if it's specified), though.

@shelhamer
Copy link
Member Author

shelhamer commented Sep 22, 2014

Right, deprecate and warn sounds good (by drop I didn't mean remove it from
the schema since that would cause trouble).

On Mon, Sep 22, 2014 at 10:56 AM, Jeff Donahue notifications@github.com
wrote:

Well, we should keep and deprecate it rather than drop it. I'm fine with
simplifying the design by ignoring it (and maybe showing a warning if it's
specified), though.

—
Reply to this email directly or view it on GitHub
#1131 (comment).

@shelhamer shelhamer mentioned this pull request Oct 8, 2014
Merged
@shelhamer
Copy link
Member Author

shelhamer commented Aug 26, 2015

Closing since the dev branch is deprecated. Please send PRs to master.

@shelhamer shelhamer closed this Aug 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

in progress

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@shelhamer @sguada @jeffdonahue