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

Default to DEBUG=OFF and PROFILE=OFF. #662

Closed
wants to merge 44 commits into from

Conversation

rcurtin
Copy link
Member

@rcurtin rcurtin commented May 31, 2016

This is because people are using mlpack from git and wondering why it is so
slow.

This issue came to my attention because I came across this paper:
http://arxiv.org/abs/1602.02514
and in the revision I read (the first or probably the second revision on arXiv), I found that the numbers for mlpack were extremely slow. After consulting with the author it turned out he had just compiled as-is off Github, with debugging symbols and all. So I think maybe compiling without debugging symbols by default will be helpful for people who just want to use mlpack, and don't bother to look into how to configure it, and then wonder why mlpack is so slow.

I'm opening this as a PR for discussion, because there are advantages and disadvantages to this. Once we reach some consensus we can make a decision. So I'm for the change (which makes sense because I opened the PR :)).

rcurtin and others added 30 commits May 16, 2016 14:57
…itm for classification models. To be precise, this is is a Variance Reduces classification reinforcement learning rule.
…xtract a retina-like representation of the input image.
 Instead of including: methods/neighbor_search/ns_traversal_info.hpp
 Include the definition in: core/tree/traversal_info.hpp
Properly use Enum type, in rann and range_search.
Remove duplicated code for traversal info.
Deprecated arma function replaced by new arma constant
@zoq
Copy link
Member

zoq commented Jun 1, 2016

At least for me, the master/developer branch is meant to be used for development only, not to run fair benchmarks against competing methods. Isn't that one reason why we have a release build which will not generate debug symbols suitable for debugging or post-processing?

@rcurtin
Copy link
Member Author

rcurtin commented Jun 2, 2016

I agree that the master branch is being used for development. It's possible we could create a separate "develop" branch and push to that instead, and push to master only when releases happen, but this then means that lots of experimental features aren't available to the average user. I also think mlpack developers don't have any problem enabling debugging and/or profiling symbols, whereas users might be confused about how to disable that.

I'm not sure, do you have any ideas for other solutions to this problem?

@mentekid
Copy link
Contributor

mentekid commented Jun 2, 2016

Most people (myself included) will closely follow directions in the README before trying alternative install configurations, meaning if we make it slightly easier for them to just copy-paste commands on the terminal they will stop compiling with debug symbols.

For example, now README.md describes the installation process like this:

The next step is to run CMake to configure the project [...]
$ cmake ../
You can specify options to compile with debugging information and profiling information:
$ cmake -D DEBUG=ON -D PROFILE=ON ../

We could have it say instead:

The next step is to run CMake to configure the project [...]
$ cmake -D DEBUG=OFF -D PROFILE=OFF ../

You can specify options to compile with debugging information and profiling information:
$ cmake -D DEBUG=ON -D PROFILE=ON ../
Be warned, this will make mlpack significantly slower and is intended for mlpack developers

(Or something like that)

@zoq
Copy link
Member

zoq commented Jun 3, 2016

So, I think the README is probably misguiding some users since it refers to one of the releases:

If you run CMake with no options, it will configure the project to build with no debugging symbols and no profiling information:

That's not true, for the master branch. However, I think @mentekid is right, and most people take a look at the README to figure out how to build mlpack. So I'm also for the change, but I think we don't need to change the README.

@SterlingPeet
Copy link

Using master for mainline development (or not) is an interesting issue. Doing so is closely aligned with the SVN 'trunk' development and branching model, but may or may not be appropriate in git.

I have noticed that git based projects (especially on GitHub) present a much more professional look and feel when using master as the stable branch, in no small part because GitHub presents the master branch as the default landing page. So, perhaps it might be worth considering the master branch in light of it being the first public introduction for someone interested in trying out mlpack.

For unrelated reasons, my personal development evolved to follow the Successful Git Branching Model proposed/documented by Vincent Driessen. This model uses master as the branch that holds the stable, release code only. Release tags in this model all point into the master branch somewhere.

That model clearly doesn't address the concerns of making experimental features available to the end user, but that may not be a bad thing. Personally, I feel that the need for unavailable experimental features indicates that surpassing a higher barrier to entry (really just the ability to checkout a different git branch in this case) is not unreasonable to ask, and my professionally developed projects reflect that philosophy.

In any case, I think the readme in the master branch should have and indication of what branch of code a user wants for normal use cases, like benchmarking or advanced (or unreleased) features.

Hopefully these comments are useful for considering how to mitigate the root concern, dealing with the unexpected things people do is challenging.

@nilayjain nilayjain force-pushed the master branch 2 times, most recently from fddfc18 to 1f562a1 Compare June 5, 2016 12:00
@rcurtin
Copy link
Member Author

rcurtin commented Jul 5, 2016

Ok, it seems like everyone thinks using default DEBUG=OFF and PROFILE=OFF is a good idea, so I went ahead and merged commit 3fe0b72 to merge this PR (I did it manually because there is weirdness in this PR and I did not want to try to merge it and break everything...).

@SterlingPeet: thanks for the detailed response. I enjoyed the article you linked to. I think maybe at some point it may be worth changing to the model you suggested, but I am not sure that is a huge deal at the moment (maybe as the project grows that will be more necessary). As it stands now, we do use feature branches, which only get merged when the code is tested and ready. So I guess realistically master only contains code-that-will-be-released-but-maybe-isn't-quite-yet. Anyway, I'll think about it, but it would be some amount of effort to change, so unless someone else wants to take the lead, I'm fine leaving it how it is now (until there is a later problem, perhaps).

@rcurtin rcurtin closed this Jul 5, 2016
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.

None yet

7 participants