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

Data_layer (#1933) update and URI sources #2351

Closed
wants to merge 1 commit into from
Closed

Conversation

cypof
Copy link
Member

@cypof cypof commented Apr 22, 2015

This PR is meant to replace #1933. It has the same features, with a lot of cleaning, a consistent way to initialize random seeds between threads, and a better separation between prefetching and data_layer code. Also some dependencies are now optional.

  • Threads copy Caffe's thread local variables on startup, e.g. current device and mode. Each thread now has it's own seed. The main thread uses the seed given by the user, other ones use a random seed generated on the parent thread. It should be deterministic given that threads will be created at the same point on each run. The code is in the InternalThread class itself, to make sure we don't forget to do it for all threads. It is not yet enough to make the parallel branch deterministic, as examples can be picked in different order from the prefect queues, but it's a first step.
  • Input and output formats can now be specified using a URI scheme: e.g. source: "lmdb://path". This makes implementations modular, and they can be added at runtime using their URI scheme. E.g. for our grid implementation we add a Java implementation of the hdfs:// scheme at runtime through a Java wrapper. A new variable in Makefile.config disables some dependencies: LMDB, LevelDB, Snappy and HDF5 for now, it should be possible to do the same for OpenCV. We also wrote a few handy schemes that do not require dependencies: file, tcp and http. They expect a binary protobuf stream of Datum (prefixed with their size).

@cypof cypof mentioned this pull request Apr 23, 2015
@flx42
Copy link
Contributor

flx42 commented Apr 25, 2015

Wow, that's a long patch! It's difficult to review all of these changes at once.

I didn't look at the details, but I have the feeling you are adding multiple orthogonal features in a single patch.
After a quick glance, I see:

  1. Some minor changes in the Makefile like := replaced by ?=.
  2. A workaround for a boost miscompile on Mac by modifying BOOST_NOINLINE. If there is an issue on Mac, you should only modify the variable on Mac.
  3. Disabling some data layers that have external dependencies. That's a good idea, but I'm not a big fan of creating empty binaries and adding too many #ifndef in the code. It would be great if we could simply prevent some tools/C++ files from being compiled in the first place if a dependency is disabled. The ultimate goal would be to be able to disable/enable modules independently, but discussing how to achieve this would probably be a long discussion.
  4. Make the Caffe context a TLS variable. I totally agree with this patch, I'm using something similar for multi-threaded GPU classification, I would be happy to review a separate PR for this.
  5. Add support for URI sources.
    6+) And you reference Data queues, prefetching and multi-source #1933 which has a long list of features.

I'm not a Caffe maintainer, but I feel all those features should be separate PRs, or at least separate patches. For 5) and 6+), I imagine you could first do a PR for URI sources, then a PR for multiple sources, then a PR for probabilities on the sources, etc. It would make each PR shorter and thus easier to review. I think a lot of developers are currently afraid by this huge patch and don't want to spend the time to review it :)

@longjon
Copy link
Contributor

longjon commented Apr 25, 2015

@flx42, thanks for your analysis. Actually we just now had an in-person meeting with @cypof and I think we agree totally with your points; this will be refactored into smaller PRs for review.

Regarding point 4, check out #2067, which is my cut-and-paste of @cypof's code (which I'm also using for #2219). That's due to be merged soon and your comments are welcome.

@cypof
Copy link
Member Author

cypof commented May 19, 2015

Has been split in the referenced PRs.

@cypof cypof closed this May 19, 2015
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.

3 participants