You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Finish transition to models/examples repository #1
I just wanted to create an issue to point out that there are some cleanup bits that need to be done in this repository after the changes we decided to make to the repository structure in mlpack/examples#61.
This is the "models" repository, which will contain ready-to-use implementations of popular machine learning model types that aren't available in the normal mlpack codebase. These can be used as bindings.
So, here's a basic todo:
update README
set up mlpack-bot
set up labels
remove any "simple" examples and replace with bindings that have CLI support, etc.
set up CMake configuration so that the models can be compiled. Maybe that means using git submodules and the mlpack repository's CMake configuration?
Thanks @rcurtin for opening this issue. Maybe it makes sense to remove one example at a time and replace it with the new model because when I made the changes earlier for all examples I had added 1700 lines and removed about 6k lines which would be difficult to review, so I'll open a PR tomorrow to replace mnist portion of the repo. I'll also try to write cmake such that it matches for git submodules. Once that is done, I think it will be easier to make changes for the rest. I can do them in 1 PR also if that makes more sense. Thanks a lot.
Agreed, sounds good. Getting the submodule part set up might require a little bit more work. (@shrit maybe you have some input on the best way to do that?) Basically, if we use submodules, it would probably mean that users can't build from this repository directly. I guess that's okay?
@rcurtin The good news is that we can build from the repository itself, even if we add it as a submodule (Assuming as before that mlpack and all other dependencies are installed correctly). In all cases, we need to have a cmake build system for the models, which is going to be used by mlpack cmake to build all the models if a user wants.
I can work on a pull request on mlpack to add this repository as a submodule and compile it directly from mlpack.
@shrit that sounds good, we can try that and see how it works. It might be good to wait until we finish transitioning this repository so that it has a bunch of bindings that use the CLI framework in it though? Either way is fine I guess. :)
@rcurtin Yes, I am able to integrate models into mlpack. The main issue so far is the compilation order (models are compiling before mlpack). Since models cmake system is going to be modified in all cases. I think I am going to wait until the model repository converge
@shrit take a look at the end of src/mlpack/CMakeLists.txt, it could cause some problems. Basically, the if (BUILDING_PYTHON_BINDINGS) section and post_markdown_setup() need to be called after all bindings are added. But we would have to add all models/ bindings before calling this code. So some restructuring of the core mlpack CMake code might be necessary (that's not a problem though).
It looks like the item remove any "simple" examples and replace with bindings that have CLI support, etc. is being handled by #3. Thanks @kartikdutt18!
I'll update the website and references to this repository within the next day.
This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! 👍
Activity
kartikdutt18 commentedon Mar 20, 2020
Thanks @rcurtin for opening this issue. Maybe it makes sense to remove one example at a time and replace it with the new model because when I made the changes earlier for all examples I had added 1700 lines and removed about 6k lines which would be difficult to review, so I'll open a PR tomorrow to replace mnist portion of the repo. I'll also try to write cmake such that it matches for git submodules. Once that is done, I think it will be easier to make changes for the rest. I can do them in 1 PR also if that makes more sense. Thanks a lot.
rcurtin commentedon Mar 20, 2020
Agreed, sounds good. Getting the submodule part set up might require a little bit more work. (@shrit maybe you have some input on the best way to do that?) Basically, if we use submodules, it would probably mean that users can't build from this repository directly. I guess that's okay?
I opened a PR to update the readme in #2.
shrit commentedon Mar 21, 2020
@rcurtin The good news is that we can build from the repository itself, even if we add it as a submodule (Assuming as before that mlpack and all other dependencies are installed correctly). In all cases, we need to have a cmake build system for the models, which is going to be used by mlpack cmake to build all the models if a user wants.
I can work on a pull request on mlpack to add this repository as a submodule and compile it directly from mlpack.
rcurtin commentedon Mar 24, 2020
@shrit that sounds good, we can try that and see how it works. It might be good to wait until we finish transitioning this repository so that it has a bunch of bindings that use the CLI framework in it though? Either way is fine I guess. :)
shrit commentedon Mar 24, 2020
@rcurtin Yes, I am able to integrate models into
mlpack. The main issue so far is the compilation order (modelsare compiling beforemlpack). Sincemodelscmake system is going to be modified in all cases. I think I am going to wait until the model repository convergercurtin commentedon Mar 26, 2020
@shrit take a look at the end of
src/mlpack/CMakeLists.txt, it could cause some problems. Basically, theif (BUILDING_PYTHON_BINDINGS)section andpost_markdown_setup()need to be called after all bindings are added. But we would have to add allmodels/bindings before calling this code. So some restructuring of the core mlpack CMake code might be necessary (that's not a problem though).It looks like the item
remove any "simple" examples and replace with bindings that have CLI support, etc.is being handled by #3. Thanks @kartikdutt18!I'll update the website and references to this repository within the next day.
mlpack-bot commentedon Apr 25, 2020
This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! 👍
Merge pull request #1 from kartikdutt18/FixBuild