Re-add support to in memory Opencv io #1068

bhack · 2014-09-11T01:42:18Z

I've tried to recover @kloudkl commit. @shelhamer I know that we have a warning: "CV_XADD" redefined but we could limit this to osx and remove when the next opencv release will be out.

bhack · 2014-09-11T15:55:54Z

@jeffdonahue Can you restart the build of this PR?

bhack · 2014-09-12T17:40:34Z

@kamjagin Can you test on OSX? @shelhamer @kloudkl What do you think?

bhack · 2014-09-18T20:47:57Z

@shelhamer any news? In the meantime it is just not mergiable anymore.. Is there a way to check before merging a PR in dev what other PR invalidates?

sguada · 2014-09-19T02:58:46Z

I'm working in PR for data transformation that will allow cv::Mat inputs.
That should allow to solve this easily.

On Thursday, September 18, 2014, bhack notifications@github.com wrote:

@shelhamer https://github.com/shelhamer any news? In the meantime it is
just not mergiable anymore.. Is there a way to check before merging a PR in
dev what other PR invalidates?

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

Sergio

bhack · 2014-09-19T06:06:55Z

@sguada as I can see you are not working on memory data layer. How your branch superseed this? @shelhamer can we try to not waste time and generate new merging conflicts? @kloudkl worked on this, then closed the old PR to open a new one, then this code was removed for that PR, then I recovered to this PR before asking to you. Then @sguada seems to be working on something similar on his branch that probably will create other conflict with this. I think that core developer need to find a way to better coordinate the efforts and evaluate what kind of PR will let to be unmergiable when they are merging some other PR in dev because in some cases rebasing is not so trivial. Actually the PR review workflow will tend to charge rebasing effort on PR reviewed later.

shelhamer · 2014-09-19T06:29:01Z

@bhack the OpenCV IO change was dropped from the previous PR because it
wasn't essential to the refactoring and it broke Caffe on OS X. It was nice
of you to reintroduce the change but re-submitting it doesn't mean it can
be merged right away.

PRs are merged by priority instead of order of arrival. Yes, this can and
does introduce conflicts for PRs later in the queue. That's the nature of
collaborative development and distributed versioning. Picking PRs by
project impact is that only way we can channel limited time into overall
progress.

The general community and BVLC members alike need to rebase to keep pace
with our active development. In preparing this release I had to rebase
others' PRs to weave it all together. Sequencing can help but there's no
way to coordinate everything ahead of time. So be it.

Please let us know if you have a suggestion for handling the PR queue. As
it is contributions make their case for merge by purpose and polish so that
important and well-executed changes can be reviewed by the core developers
in their finite time. This requires effort on the part of the PR author,
that effort includes rebasing, and if the change is believed in it's not a
waste of time at all.

On Thursday, September 18, 2014, bhack notifications@github.com wrote:

@sguada https://github.com/sguada as I can see you are not working on
memory data layer. How your branch superseed this? @shelhamer
https://github.com/shelhamer can we try to not waste time and generate
new merging conflicts? @kloudkl https://github.com/kloudkl worked on
this, then closed the old PR to open a new one, then this code was removed
for that PR, then I recovered to this PR before asking to you. Then
@sguada https://github.com/sguada seems to be working on something
similar on his branch that probably will create other conflict with this. I
think that core developer need to find a way to better coordinate the
efforts and evaluate what kind of PR will let to be unmergiable when they
are merging some other PR in dev because in some cases rebasing is not so
trivial. Actually the PR review workflow will tend to charge rebasing
effort on PR reviewed later.

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

bhack · 2014-09-19T06:48:44Z

I don't think that proposing again a PR will let it merge. But I was always available to discuss and work on reviewed feedbacks. There is not an automatic way on github to preview what kind of PR a candidate merge invalidates (there is any cross PR merge checks O(n^2)?). One easy way at human level is to handle a well codified tagging of PR status (WIP,NEED REVIEW, DEPEND ON etc.) so that we have also a good triage. With this we could handle a little bit the merging queue handling a balance between reviewers interests and time of arrival (or time a PR is in a certain status). It is a way to incentive contributors that if not find the spare time to rebase in time probably the PR status will enter in a sort of drift. I.e. see @sguada multi label dev PR last comments.

sguada · 2014-09-19T07:37:54Z

I will push more changes including memory data layer tomorrow. Then you can
see if that serve your purpose or not.

On Thursday, September 18, 2014, bhack notifications@github.com wrote:

I don't think that proposing again a PR will let it merge. But I was
always available to discuss and work on reviewed feedbacks. There is not an
automatic way on github to preview what kind of PR a candidate merge
invalidates (there is any cross PR merge checks O(n^2)?). One easy way at
human level is to handle a well codified tagging of PR status (WIP,NEED
REVIEW, DEPEND ON etc.) so that we have also a good triage. With this we
could handle a little bit the merging queue handling a balance between
reviewers interests and time of arrival (or time a PR is in a certain
status). It is a way to incentive contributors that if not find the spare
time to rebase in time probably the PR status will enter in a sort of
drift. I.e. see @sguada https://github.com/sguada multi label dev PR
last comments.

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

Sergio

bhack · 2014-09-19T08:08:43Z

@sguada It is fine that you will work also on memory data layer and data transformation layer (take in account @shelhamer cited OSX issue that rejected opencv code and I've experimented to override here). But this kind of logic are not clear to me. Probably you do also face to face meeting at BVLC so it is normal that a "collaborator" PR could often have a fast track. Sometimes the goal are the same in other cases the priority could be different but it could be nice to see some kind of workflow logic. Often reimplementing something that is already in a PR in a larger one that will superseed it is caused by the fact that PR was not reviewed or fixing work was not done in time. So it is quite strange that a large PR that probably require more time to analyze the impact on the project superseded on a fast track a small one (I don't how this PR is in a good shape because was only partially reviewed) or that probably will procrastinate a small change while waiting for the superseding. I repeat that is not probably a problem of common goal. It is a problem workflow logic. Also you commented on multi label PR that you not want to invest more time to rebase it if somebody don't have time to review it in time (in time for me it is without requiring some other futher not trivial rebasing slot). So I think that is a general issue.

see ipython/ipython#5686

* added gflags requirement in CMake * fixed a bug that linked some tests into caffe lib * renamed tools/caffe due to conflicting target names with caffe lib * rebased onto bvlc/caffe

(Required as I had to change the tool target names to '.bin' but give them an OUTPUT_NAME, but the .bin made the test_net tool collide with the test_net unit test.)

- All the instructions mentioned for CentOS/RHEL are also valid for Fedora Linux so add it. - The package name for atlas is atlas-devel not libatlas-devel for CentOS/RHEL/Fedora so fix it. - Add 'sudo' prefix to missing places to be consistent with the rest of the document. - Add instructions for installing python headers in Fedora into the python support section. - Update Linux installation section to reflect Fedora, refactor a bit.

bhack · 2014-09-19T13:58:59Z

I've rebased. But i think that we need to try to find some kind of policy to coordinate and handle PR queue without discouraging external contributions with PR minimizing not trivial drift and superseding.

nicodjimenez · 2014-09-19T17:22:23Z

I've been following caffe development for a while, waiting for a method to feed a vector of OpenCV Mat's to make its way to the master branch because I need it for a real time application. Given how many people ask "How to make a prediction in C++" on github and on the mailing list, it's strange to me that this development would not have high priority. Just my two cents!

shelhamer · 2014-09-19T17:47:18Z

@bhack @nicodjimenez absent community input the Caffe development priorities are set by research since the BVLC members are all PhD students / postdocs / researchers. Thanks for voicing your votes for this feature.

bhack · 2014-09-19T17:59:10Z

@shelhamer I've never told that this is an high priority for the project. It is a priority for me and this is obvious because I'm using my time to try to let this merge. I've never pretend to set your priority or the priority of BVLC group. I only want a clear workflow to contribute. Actually relay only on hidden (to the community) priority IMHO is not the best way to attract external contribution.

shelhamer · 2014-09-19T18:12:15Z

@bhack right, thanks for making the community need for this feature clear and suggesting a review of the PR policy. We try to highlight PRs by milestones but that doesn't settle conflicts among contributions which we can try to figure out how to address.

bhack · 2014-09-19T18:18:12Z

@shelhamer thanks. I know that it is hard to figure how to adapt the governance and workflow on a project that was historically mainly developed by a small group of people that often work and meet in the same physical place. But I'm sure the your have got something from my feedback and you can discuss with other core member to review a little bit the workflow.

jeffdonahue · 2014-09-19T18:35:48Z

Note that our development guide suggests that prospective contributors "Make PRs as soon as development begins, to let discussion guide development." In practice people haven't really followed this, instead usually making a PR when most development is already complete. That's fine, but it means that the feature might never make it into the official Caffe repo -- we certainly do not have a policy of accepting every proposed pull request.

I'm not entirely sure what the situation is with this particular line of work on opencv mats, but it seems that @sguada is working on a revamp of the all the data layer mess, so given that that is likely coming soon, it would make sense to hold off on adding new data input formats until that gets merged. So if the opencv mat PR had been opened before development began, one of the core developers could have commented to recommend holding off until the consolidation is complete, so as to avoid the annoyance of a major rebase later.

The beauty of open source and git is that if you really want a feature right now, though, you are always welcome to write it on your own. But while we really value the community contributions, we really do not have the resources to guarantee little work for everyone who wants to contribute any feature to the official BVLC repo, and our own research needs are inevitably going to come before contributor preferences. Don't like it? That's why you have a fork.

bhack · 2014-09-19T19:01:34Z

@shelhamer discarded this in an old PR only because there was an OSX issue with opencv and cuda. I asked to him before trying to reopen this PR. Datalayer was modified often in this last weeks and @sguada is developing on his own repository and I cannot follow every single caffe fork. @jeffdonahue We meet for a short time at ECCV on the tutorial. If this is not your first open source project you know well that "your are free to fork" it is not the solution to all the problems. Maintaining a fork need an investment of resources for merge/rebase it (like abi api changes) more or less regularly and it is the life of an open source project to accept contribution. The bottleneck cannot be only the lack of reviewers.

sguada · 2014-09-19T21:56:40Z

@bhack I didn't want to discourage you from rebasing or working on this. But since I know that rebasing takes sometime, then some of the code I wrote could make your life easier.
As you know several of my PR have also been superseded or took long to get review it. Give the current time limitations of BVLC core members not everything can be reviewed quickly.

I looked a your PR and it seems a bit convoluted, you did a lot of work to read cv::Mat into datum and Blobs, but I think a simpler approach can be better. For example letting io just read cv::Mat and letting the transform_data.cpp do the transformation into Blobs simplify the code a lot. However I think some of the optimizations you did could be used.

Please take a look to the updated #1070 Although I didn't add yet Transform(vectorcv::Mat, Blob) that should be fairly easy. Please feel free to comment it and see points where we could join efforts and make join PR

bhack · 2014-09-19T22:51:38Z

@sguada I know what mean PR drift and rebasing. We are blocked on a branch with your multi label PR merged and at this time require an heavy rebasing effort. For this PR as you can see in commit history I've mainly recovered removed commits of another PR and I've added some little fix. What is your transformer goal? You can get some of the optimization in Mat copying. You will have the same problem of the old PR with opencv cuda and osx.. Do you want to get clang define override (it is an experiment I don't have an Osx host)?

sguada · 2014-09-20T00:00:13Z

@bhack in #1070 I wanted to streamline the data transformation, unifying the the interface and allowing to take different kinds of data sources (Datum, cv::Mat, Blob) and transformed them into a blob that the Data Layers can use.
This allowed to simplify the logic of DataLayer, ImageDataLayer and MemoryDataLayer, or a VideoDataLayer to come.
I'm going to redo the multi label PR, since a this point the commits and rebase would be to difficult.

bhack · 2014-09-20T06:54:13Z

@sguada OK it make sense. I'll comment on your.

bhack · 2014-09-23T15:50:29Z

@sguada @shelhamer opencv/opencv#3255 was merged in the 2.4 branch so will available in the next 2.4.x release.

shelhamer · 2014-09-23T16:05:24Z

Great, thanks for pursuing a fix @bhack.

On Tuesday, September 23, 2014, bhack notifications@github.com wrote:

@sguada https://github.com/sguada @shelhamer
https://github.com/shelhamer opencv/opencv#3255
opencv/opencv#3255 was merged in the 2.4 branch
so will available in the next 2.4.x release.

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

shelhamer · 2014-09-25T08:01:04Z

@bhack this fails to build with the same error:

/usr/local/cuda/bin/nvcc -ccbin=/usr/bin/clang++ -Xcompiler -fPIC -DNDEBUG -O2 -DUSE_MKL -I/Users/shelhamer/anaconda/include -I/Users/shelhamer/anaconda/include/python2.7 -I/Users/shelhamer/anaconda/lib/python2.7/site-packages/numpy/core/include -I/usr/local/include -I.build_release/src -I./src -I./include -I/usr/local/cuda/include -I/opt/intel/mkl/include -gencode arch=compute_30,code=sm_30 -gencode arch=compute_35,code=sm_35 -c src/caffe/layers/absval_layer.cu -o .build_release/src/caffe/layers/absval_layer.cuo 2> .build_release/src/caffe/layers/absval_layer.cuo.warnings.txt \
    || (cat .build_release/src/caffe/layers/absval_layer.cuo.warnings.txt; exit 1)
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/clang/6.0/include/emmintrin.h(1225): error: identifier "__builtin_ia32_movnti64" is undefined
/usr/local/include/opencv2/core/mat.hpp(117): error: type name is not allowed
/usr/local/include/opencv2/core/mat.hpp(117): error: identifier "_Atomic" is undefined
/usr/local/include/opencv2/core/mat.hpp(117): error: expected an expression
/usr/local/include/opencv2/core/mat.hpp(117): error: identifier "__c11_atomic_fetch_add" is undefined

while producing warnings as you mentioned:

./include/caffe/data_layers.hpp:29:15: warning: 'CV_XADD' macro redefined
#      define CV_XADD(addr, delta) __c11_atomic_fetch_add \

bhack · 2014-09-25T08:18:28Z

@shelhamer i'dont have a osx host to test the exact condition. Can you try to use directly the else branch of the define and enable sse2 compiler flag?

sguada · 2014-09-25T15:32:55Z

@shelhamer https://github.com/shelhamer can you test the #1070 branch in
Osx, if it works, then this would not be needed.

On Thursday, September 25, 2014, bhack notifications@github.com wrote:

@shelhamer https://github.com/shelhamer i'dont have a osx host to test
the exact condition. Can you try to use directly the else branch of the
define and enable sse2 compiler flag?

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

Sergio

shelhamer · 2014-09-25T15:53:00Z

@sguada I did and it doesn't: #1070 (comment)

Good to know that #1070 is now a replacement for this so we can close this on merge.

bhack · 2014-09-25T16:05:50Z

@shelhamer try to simple override with then we can tuning condition..
define CV_XADD(addr, delta) __atomic_fetch_add(_Atomic(static_cast<int*>(addr), delta, 4)

For __builtin_ia32_movnti64 try to enable sse2

bhack · 2014-09-25T16:41:47Z

@shelhamer What clang version are you using?

shelhamer · 2014-09-25T16:44:01Z

Apple LLVM version 6.0 (clang-600.0.51) (based on LLVM 3.5svn)

On Thu, Sep 25, 2014 at 9:41 AM, bhack notifications@github.com wrote:

@shelhamer https://github.com/shelhamer What clang version are you
using?

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

bhack · 2014-09-25T16:51:33Z

@shelhamer can you add -DUSE_64 flag for clang?

shelhamer · 2014-10-04T19:01:56Z

This should be settled with the merge of #1070. @bhack let us know if not.

bhack mentioned this pull request Sep 11, 2014

Opencv io support #1061

Closed

bhack force-pushed the opencv_io branch 7 times, most recently from 164c035 to b404609 Compare September 12, 2014 17:30

bhack mentioned this pull request Sep 15, 2014

how to make a prediction in C++ #499

Closed

shelhamer force-pushed the dev branch from 64258b6 to 403b56b Compare September 19, 2014 04:38

shelhamer and others added 9 commits September 19, 2014 15:28

[example] fix example names

f5f4f01

[example] change notebook name metadata to avoid conflict

783ce58

see ipython/ipython#5686

added gflags + bugfixes + rebase on bvlc/caffe

1c12fab

* added gflags requirement in CMake * fixed a bug that linked some tests into caffe lib * renamed tools/caffe due to conflicting target names with caffe lib * rebased onto bvlc/caffe

restore .testbin extension, and move caffe_tool back to "caffe".

415f23a

(Required as I had to change the tool target names to '.bin' but give them an OUTPUT_NAME, but the .bin made the test_net tool collide with the test_net unit test.)

[docs] include cuDNN in installation and performance reference

61c7391

Add utility function to convert OpenCV image into Datum and Blob

ab12068

Generalize the conversions between Blob and OpenCV Mat

290ce8d

Directly copy memory into Datum when image is 1 channel & continuous

25176d0

bhack force-pushed the opencv_io branch from b404609 to 20415d8 Compare September 19, 2014 13:37

Override CV_XADD for OSX and fix test

c716ebc

bhack force-pushed the opencv_io branch from 20415d8 to c716ebc Compare September 19, 2014 13:52

sguada mentioned this pull request Sep 20, 2014

Refactor data_transform to allow datum, cv:Mat and Blob transformation #1070

Merged

shelhamer added in progress and removed in progress labels Sep 21, 2014

shelhamer closed this Oct 4, 2014

Re-add support to in memory Opencv io #1068

Re-add support to in memory Opencv io #1068

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!