Closed kartikdutt18 closed 4 years ago
@kartikdutt18 it sounds a good idea of defining known architectures as typedef
I even thought of defining a header file LeNet.hpp
that defines the architecture which is separate from the CLI
executables files. For example, these models header files can be installed on the user machine. This will allow the user to do the following:
#include <mlpack/models/LeNet.hpp>
int main()
{
mlpack::models::LeNet le_net;
le_net.Train(//callbacks can go here with data set as usual); // if you want to train
le_net.Import_weights(/* Path for weights*/); // transfer learning
le_net.another_functions()
}
Since these architectures are usually not modified, we can create a generic API for all these models files. What do you think @kartikdutt18 @rcurtin ?
@shrit, That would be great. What I have imagined is that we can have a file called object_detection.cpp (which would have all CLI related stuff). And of course users would have access to models that are installed as you mentioned so that they can be used in some other files as well. So this way we can have ready to train and run models to solve classical problems as well as models that can be used for user specific problems as well.
I will be pushing some commits after write some unit tests.
Then we could do something like:
./object_detection DATASET = path_to_dataset MODEL = model_name EPOCHS = 50 OTHER_PARAMS=...
Awesome work! :rocket: Looks good to me so far, just a few comments:
I think I agree that maybe a simple neural network isn't necessary here. Someone could already use, e.g., mlpack's logistic regression implementation or random forest implementation if they're looking for a simple baseline.
I might suggest structuring the directories such that, e.g., there's models/lenet5/
and this directory would contain both the lenet5.hpp
file that defines the LeNet5
class, and also the binding, e.g., lenet5_main.cpp
, which defines the mlpackMain()
and associated parameters that could be compiled into a mlpack_lenet5
command-line program or mlpack.lenet5()
Python function, etc. The research/
name doesn't make too much sense to me though.
Another thought might be, it could be good to do this as incremental one-at-a-time PRs instead of a single big one. :) But up to you. :+1:
I think that makes more sense to me, Maybe this makes sense :
I think that way we would have 3 PRs and each state we would have a working repo. @shrit, @rcurtin, What do you think?
The research/ name doesn't make too much sense to me though.
I couldn't think of any name so I borrowed the name from pytorch.
Removed WIP tag, At least C++ portion is almost done. Just have to finish the tests and the error with linking boost tests(undefined main). If anyone has an idea what I might be missing, that would be great.
Hey @rcurtin, @zoq, @shrit, Could you please review this PR or leave some general comments. I have got it working for the most part. I am training LeNet right now and I will add test for that and url for their download as well. As @zoq suggested, I also plan on adding a download option through cmake as some people might want to download their datasets before runtime so that they directly run/test the model. Few things that are left:
./bin/models_test -t UtilsTest
however the Ctest is still empty(I have to figure that out).The subsequent PRs would be about changing to git sub modules and then adding CLI.
Also would it possible to add Azure pipelines and style-checks here as well. I will update travis-ci for the same.
@mlpack-jenkins test this please
@mlpack-jenkins test this please
About setting up azure pipelines, we would need a build strategy first.
Also, once https://github.com/mlpack/jenkins-conf/pull/18 is merged, the style check should work correctly.
Great, Also Can I also get your opinion on this PR. It would really mean a lot. Thanks.
About setting up azure pipelines, we would need a build strategy first.
Does this refer to yaml for running azure pipelines ? I can add that as well here but that will definitely be changed when this repo switches to submodules. I am probably missing something here.
Yes, once the yaml file there, I can point azure to it.
Yes, once the yaml file there, I can point azure to it.
Great, I will add in the next couple of days.
@mlpack-jenkins test this please
Sorry I got caught up in codejam and an assignment submission that was due today. Should I resolve conflicts maybe that is causing the issue?
@mlpack-jenkins test this please
Shouldn't be a problem
Sorry I got caught up in codejam and an assignment submission that was due today. Should I resolve conflicts maybe that is causing the issue?
No need, we don't build the code, looks like it's fixed now.
Ahh great, Thanks a lot. Will fix make all changes tonight.
Hey @rcurtin, I think we discussed most of the points in the video meet, However here is the summary incase I missed something.
What's the use case for object_recognition.cpp?
The current file will be adapted in another PR to support CLI. To specify model, training params and dataset from CLI so that a user can train a model from command line on any dataset. Added a comment on top of the file to mention the same.
leNet as just an FFN.
I am testing the implementation as FFN right now, once it works I will push it as well.
Having a build network function.
I haven't implemented that as it will introduce redundant code as various versions of the model have the same building blocks. Incase I have missed something, kindly let me know.
I have also read up a bit about azure pipelines and I will add a commit for build tomorrow as well.
I have a long weekend this week, I plan on completing this PR during the same.
I will take care of style issues together (I hope that's okay). Sorry. I have also replied to some comments that I haven't resolved yet. Thanks a lot for the review.
That is a pretty minimal example, so I don't think we will run into any problem here, but we might want to mention the examples (add a link).
Ahh, great. I will update them with style fixes in the next commit.
Hey @zoq, @rcurtin, If you get a chance, could you please take another look at this. I have fixed all style issues pertaining to the files that I have added. Sorry for the delayed response, I had to prepare for my online assessment. I'll try to finish up as much as I can during the same period. I will push the yaml files as well. I would really appreciate your suggestions. Thanks a ton.
For style changes in this repo and examples that are referring to the current files, I can open another PR.
Hey @zoq, when you get a chance could you please point the azure pipelines to the yaml file here. I have only added OSX for now and I'll add Windows and Linux in the subsequent commits. Thanks, Regards.
Great, we just need an initial config file, so once https://github.com/mlpack/models/pull/7 is merged I can set up the pipeline.
Great, Thanks a lot.
Looks like there are some errors in the ci.yaml file but the pipeline works now.
Great. Thanks, I'll shorten the PR shortly. Thanks a lot.
Hey @zoq, @saksham189, It seems like I am unable to create / write file (in runtime) testing in azure pipelines. I added a bunch of warning during download to see if it created files as well as wrote a test which gives a warning if the machine was unable to create a new file. Both of these warnings appear in CTest. This works fine locally, I am most likely missing something, Could you please help me out with this. Thanks a ton.
So, I think with the build issues solved, what is left is to have some form of documentation, or do you like to add that in another PR?
So, I think with the build issues solved, what is left is to have some form of documentation, or do you like to add that in another PR?
I could do that in another PR, I tried reviewing this and it's a bit too long. I also have the image dataloader, lenet PR almost ready so once this done I can open them sequentially. I also need to add windows build before we merge this. Let me know what you think.
Sounds good, let's integrate the windows ci config here and open other PR's for each extension. Nice work!
Let me know if you need any help with the windows build.
Hey @zoq, I did try a few things today so I was able to configure armadillo in models repo and mlpack. I am still facing issues with linking mlpack to models repo which hopefully I'll be able to resolve by tomorrow, if you have any suggestions to fix the build that would be great. Thanks a lot for your time.
Hey @zoq, The windows is built is almost done, The build links everything that is needed However it fails because of the following error :
file STRINGS file
"D:/a/1/s/mlpack/build/Release/include/mlpack/core/util/version.hpp" cannot
be read.
Once this is resolved the windows build should pass as well. Could you point me to the direction in which I should look to fix the above error.
Ahh, so the windows build is fixed now.
I have opened mlpack/mlpack#2409 to discuss windows build. Regards.
Awesome, The build passed. Hey @zoq, @saksham189, when you get a chance could you please take another look at this one. Thanks a lot.
Great, Thanks a lot for the review and the help.
Awesome, Thanks a lot everyone! For the reviews and amazing help. Thanks a lot. I'll open a PR with documentation and LeNet (separate PRs) first thing tomorrow. Regards.
Basic Structure of models repo:
There a ton to do, so I have added a list below of what I plan to accomplish in this PR:
Adding a timeline here to complete remaining points to ensure that I don't fall behind: