Namespace and include guard

[Aakash-kaushik]

Implements issues discussed in #54 Replaces #55 closes #54

[Aakash-kaushik]

@rcurtin, @zoq this is the new PR to sort out the actual changes that were reflecting in a bad way in the previous PR.

[Aakash-kaushik]

Nice, no further comments from my side.

Hey i still have to implement the regex part for ctest test parsing.

[zoq]

Nice, no further comments from my side.

Hey i still have to implement the regex part for ctest test parsing.

Ahh, thanks for the info.

[Aakash-kaushik]

Requesting changes till tests pass.

The test failure is because of the parsing of tests, i need to learn about that part as i am a bit unaware of cmake as a language.

[Aakash-kaushik]

Hey so as i used a component of Catch2 to parse tests for ctest and we just use the catch2.hpp for testing so that's why these tests are failing is it okay if i install catch2 on the testing machines or is it too much to add as a dependency?

[zoq]

Hey so as i used a component of Catch2 to parse tests for ctest and we just use the catch2.hpp for testing so that's why these tests are failing is it okay if i install catch2 on the testing machines or is it too much to add as a dependency?

Catch2 is part of the repository https://github.com/mlpack/models/blob/master/tests/catch.hpp but maybe you need a newer version, if that is the case, please feel free to include the updated file as part of the PR. But maybe there is another file that is needed?

[Aakash-kaushik]

Hey so as i used a component of Catch2 to parse tests for ctest and we just use the catch2.hpp for testing so that's why these tests are failing is it okay if i install catch2 on the testing machines or is it too much to add as a dependency?

Catch2 is part of the repository https://github.com/mlpack/models/blob/master/tests/catch.hpp but maybe you need a newer version, if that is the case, please feel free to include the updated file as part of the PR. But maybe there is another file that is needed?

Hey yes i see that but the way i have used catch2 for parsing tests inside cmake that requires that catch2 is installed on the system and not just as a file.

[rcurtin]

We definitely can't assume that users have catch2 installed on the system---that's precisely why we have catch included in our sources. Why not just use CMake's regex parsing facilities to extract all possible test suite names, then filter the list for uniques? I think this is what the old code did.

[zoq]

We definitely can't assume that users have catch2 installed on the system---that's precisely why we have catch included in our sources. Why not just use CMake's regex parsing facilities to extract all possible test suite names, then filter the list for uniques? I think this is what the old code did.

No need to install catch, you can just reference the header that we already package, see the comment above. I think that's the easiest solution, we just have to swap find_package(Catch2 REQUIRED) with add_subdirectory(tests) and we should be set.

[Aakash-kaushik]

We definitely can't assume that users have catch2 installed on the system---that's precisely why we have catch included in our sources. Why not just use CMake's regex parsing facilities to extract all possible test suite names, then filter the list for uniques? I think this is what the old code did.

No need to install catch, you can just reference the header that we already package, see the comment above. I think that's the easiest solution, we just have to swap find_package(Catch2 REQUIRED) with add_subdirectory(tests) and we should be set.

Hey @zoq adding tests as a subdirectory inside the catch file in the tests folder wouldn't make sense, right ? and this is also giving me an error that i can't find this.

[Aakash-kaushik]

We definitely can't assume that users have catch2 installed on the system---that's precisely why we have catch included in our sources. Why not just use CMake's regex parsing facilities to extract all possible test suite names, then filter the list for uniques? I think this is what the old code did.

No need to install catch, you can just reference the header that we already package, see the comment above. I think that's the easiest solution, we just have to swap find_package(Catch2 REQUIRED) with add_subdirectory(tests) and we should be set.

Hey @zoq adding tests as a subdirectory inside the catch file in the tests folder wouldn't make sense, right ? and this is also giving me an error that i can't find this.

the function i used is described here https://github.com/catchorg/Catch2/blob/devel/extras/Catch.cmake and i think is not shipeed with the catch.hpp we use.

[Aakash-kaushik]

We definitely can't assume that users have catch2 installed on the system---that's precisely why we have catch included in our sources. Why not just use CMake's regex parsing facilities to extract all possible test suite names, then filter the list for uniques? I think this is what the old code did.

I have been avoiding this way because i have been getting ton of erros and not really good at it.

[zoq]

We definitely can't assume that users have catch2 installed on the system---that's precisely why we have catch included in our sources. Why not just use CMake's regex parsing facilities to extract all possible test suite names, then filter the list for uniques? I think this is what the old code did.

No need to install catch, you can just reference the header that we already package, see the comment above. I think that's the easiest solution, we just have to swap find_package(Catch2 REQUIRED) with add_subdirectory(tests) and we should be set.

Hey @zoq adding tests as a subdirectory inside the catch file in the tests folder wouldn't make sense, right ? and this is also giving me an error that i can't find this.

Right, I looked at the wrong section, I left a comment above, that should work.

[rcurtin]

Ah, yeah, if Catch has a specific CMake file that can extract the tests, I don't see any issue with including that in this repository. In fact, I suppose we could do the same in the main mlpack repository. (Note that I have not read the comments here very closely, so I may have overlooked something.)

[Aakash-kaushik]

We definitely can't assume that users have catch2 installed on the system---that's precisely why we have catch included in our sources. Why not just use CMake's regex parsing facilities to extract all possible test suite names, then filter the list for uniques? I think this is what the old code did.

No need to install catch, you can just reference the header that we already package, see the comment above. I think that's the easiest solution, we just have to swap find_package(Catch2 REQUIRED) with add_subdirectory(tests) and we should be set.

Hey @zoq adding tests as a subdirectory inside the catch file in the tests folder wouldn't make sense, right ? and this is also giving me an error that i can't find this.

Right, I looked at the wrong section, I left a comment above, that should work.

Hey i tried that and i am still facing an error, i pushed a commit so you can see that too if you check out the build models section.

[Aakash-kaushik]

Ah, yeah, if Catch has a specific CMake file that can extract the tests, I don't see any issue with including that in this repository. In fact, I suppose we could do the same in the main mlpack repository. (Note that I have not read the comments here very closely, so I may have overlooked something.)

Hey so Catch does have a file that can extract tests and is also shipped with their package but we have to include it ourselves as we are not making catch2 a dependency.

[Aakash-kaushik]

I see two clear ways for this, the first is to use the fetch content of cmake or second is to clone the catch2 repo in the models repo, there might be a way to make it work with just these two files which are Catch.cmake and CatchAddTests.cmake but i wasn't able to make it work.

[zoq]

This is because if you run bin/models_test it returns the number of tests instead of 0 and

https://github.com/catchorg/Catch2/blob/023b5306b444ebe3c20a008cdd150cf5510ff36d/extras/CatchAddTests.cmake#L44 https://github.com/catchorg/Catch2/blob/023b5306b444ebe3c20a008cdd150cf5510ff36d/extras/CatchAddTests.cmake#L61

checks if the tests executable returns successfully, so either we remove the two checks from the CMake file or we update the tests/main.cpp to return 0 at the end. I guess updating the main is probably the easiest solution.

[Aakash-kaushik]

This is because if you run bin/models_test it returns the number of tests instead of 0 and

https://github.com/catchorg/Catch2/blob/023b5306b444ebe3c20a008cdd150cf5510ff36d/extras/CatchAddTests.cmake#L44 https://github.com/catchorg/Catch2/blob/023b5306b444ebe3c20a008cdd150cf5510ff36d/extras/CatchAddTests.cmake#L61

checks if the tests executable returns successfully, so either we remove the two checks from the CMake file or we update the tests/main.cpp to return 0 at the end. I guess updating the main is probably the easiest solution.

that last commit should fix most of the things but as i checked last the windows build was still failing. if it fails again i will have to look into it.

[zoq]

I mean it worked before, so hopefully this is just a temporary issue.

[Aakash-kaushik]

I mean it worked before, so hopefully this is just a temporary issue.

It failed again, why does windows has to be so hard to work with. Do you have any idea about the error because this seems too much cryptic for me.

[zoq]

I mean it worked before, so hopefully this is just a temporary issue.

It failed again, why does windows has to be so hard to work with. Do you have any idea about the error because this seems too much cryptic for me.

Can you explicitly set the CMAKE_INSTALL_PREFIX to -DCMAKE_INSTALL_PREFIX=$(Agent.ToolsDirectory)\ hopefully that helps.

[Aakash-kaushik]

I mean it worked before, so hopefully this is just a temporary issue.

It failed again, why does windows has to be so hard to work with. Do you have any idea about the error because this seems too much cryptic for me.

Can you explicitly set the CMAKE_INSTALL_PREFIX to -DCMAKE_INSTALL_PREFIX=$(Agent.ToolsDirectory)\ hopefully that helps.

Hey i checked the error too and most of the places it is highlighted as a permission error and we haven't changed anything in the windows build step so why would these just fail like that.

btw do you how to sepcify that a powershell command should be executed as admin in the yaml file for azure builds ?

[Aakash-kaushik]

Hey @zoq checkout #57 i think the windows test failure might be because of the changes in cmake maybe ?

[rcurtin]

I see two clear ways for this, the first is to use the fetch content of cmake or second is to clone the catch2 repo in the models repo, there might be a way to make it work with just these two files which are Catch.cmake and CatchAddTests.cmake but i wasn't able to make it work.

It looks like in the time since you wrote that, you got it working, and I think it looks great! I just tested it locally, and it's very nice to be able to have CTest see all the different tests:

$ ctest .
Test project /home/ryan/src/models/build
      Start  1: ResizeAugmentationTest
 1/15 Test  #1: ResizeAugmentationTest ...................   Passed    0.01 sec
      Start  2: DarknetModelTest
 2/15 Test  #2: DarknetModelTest .........................   Passed   31.45 sec
      Start  3: YOLOV1ModelTest
 3/15 Test  #3: YOLOV1ModelTest ..........................   Passed    7.83 sec
      Start  4: CSVDataLoaderTest
 4/15 Test  #4: CSVDataLoaderTest ........................   Passed    0.04 sec
      Start  5: MNISTDataLoaderTest
 5/15 Test  #5: MNISTDataLoaderTest ......................   Passed    6.65 sec
      Start  6: ObjectDetectionDataLoaderFieldTypeTest
 6/15 Test  #6: ObjectDetectionDataLoaderFieldTypeTest ...   Passed    2.86 sec
      Start  7: ObjectDetectionDataLoaderMatTypeTest
 7/15 Test  #7: ObjectDetectionDataLoaderMatTypeTest .....   Passed    1.04 sec
      Start  8: LoadImageDatasetFromDirectoryTest
 8/15 Test  #8: LoadImageDatasetFromDirectoryTest ........   Passed    3.08 sec
      Start  9: YOLOPreProcessor
 9/15 Test  #9: YOLOPreProcessor .........................   Passed    0.01 sec
      Start 10: DownloadFileTest
10/15 Test #10: DownloadFileTest .........................   Passed    0.04 sec
      Start 11: CheckSumTest
11/15 Test #11: CheckSumTest .............................   Passed    0.04 sec
      Start 12: PathExistsTest
12/15 Test #12: PathExistsTest ...........................   Passed    0.01 sec
      Start 13: RemoveFileTest
13/15 Test #13: RemoveFileTest ...........................   Passed    0.01 sec
      Start 14: ExtractFilesTest
14/15 Test #14: ExtractFilesTest .........................   Passed    0.06 sec
      Start 15: CurlDownloadTest
15/15 Test #15: CurlDownloadTest .........................   Passed    0.21 sec

100% tests passed, 0 tests failed out of 15

Total Test time (real) =  53.33 sec

One of the really nice things about this is that now, we could (if we wanted) run the tests in parallel. :+1:

[zoq]

Right, we only have to fix the Windows build, so maybe we should patch this out for now and open another pull-request with the changes, just to not delay the PR any longer. I can at least reproduce the Windows build issue locally, but I guess at this point we all know that this could take some more time to figure out.

[Aakash-kaushik]

I see two clear ways for this, the first is to use the fetch content of cmake or second is to clone the catch2 repo in the models repo, there might be a way to make it work with just these two files which are Catch.cmake and CatchAddTests.cmake but i wasn't able to make it work.

It looks like in the time since you wrote that, you got it working, and I think it looks great! I just tested it locally, and it's very nice to be able to have CTest see all the different tests:

$ ctest .
Test project /home/ryan/src/models/build
      Start  1: ResizeAugmentationTest
 1/15 Test  #1: ResizeAugmentationTest ...................   Passed    0.01 sec
      Start  2: DarknetModelTest
 2/15 Test  #2: DarknetModelTest .........................   Passed   31.45 sec
      Start  3: YOLOV1ModelTest
 3/15 Test  #3: YOLOV1ModelTest ..........................   Passed    7.83 sec
      Start  4: CSVDataLoaderTest
 4/15 Test  #4: CSVDataLoaderTest ........................   Passed    0.04 sec
      Start  5: MNISTDataLoaderTest
 5/15 Test  #5: MNISTDataLoaderTest ......................   Passed    6.65 sec
      Start  6: ObjectDetectionDataLoaderFieldTypeTest
 6/15 Test  #6: ObjectDetectionDataLoaderFieldTypeTest ...   Passed    2.86 sec
      Start  7: ObjectDetectionDataLoaderMatTypeTest
 7/15 Test  #7: ObjectDetectionDataLoaderMatTypeTest .....   Passed    1.04 sec
      Start  8: LoadImageDatasetFromDirectoryTest
 8/15 Test  #8: LoadImageDatasetFromDirectoryTest ........   Passed    3.08 sec
      Start  9: YOLOPreProcessor
 9/15 Test  #9: YOLOPreProcessor .........................   Passed    0.01 sec
      Start 10: DownloadFileTest
10/15 Test #10: DownloadFileTest .........................   Passed    0.04 sec
      Start 11: CheckSumTest
11/15 Test #11: CheckSumTest .............................   Passed    0.04 sec
      Start 12: PathExistsTest
12/15 Test #12: PathExistsTest ...........................   Passed    0.01 sec
      Start 13: RemoveFileTest
13/15 Test #13: RemoveFileTest ...........................   Passed    0.01 sec
      Start 14: ExtractFilesTest
14/15 Test #14: ExtractFilesTest .........................   Passed    0.06 sec
      Start 15: CurlDownloadTest
15/15 Test #15: CurlDownloadTest .........................   Passed    0.21 sec

100% tests passed, 0 tests failed out of 15

Total Test time (real) =  53.33 sec

One of the really nice things about this is that now, we could (if we wanted) run the tests in parallel. 👍

Hey, yes first i thought that we need catch as a library because I don't know why but it expects a 0 return from your test executable's main. @zoq pointed that out and so it all worked.

[Aakash-kaushik]

Right, we only have to fix the Windows build, so maybe we should patch this out for now and open another pull-request with the changes, just to not delay the PR any longer. I can at least reproduce the Windows build issue locally, but I guess at this point we all know that this could take some more time to figure out.

Hey @zoq i agree to that so if you feel like it's ok to merge this, feel free to go ahead.

[zoq]

Right, we only have to fix the Windows build, so maybe we should patch this out for now and open another pull-request with the changes, just to not delay the PR any longer. I can at least reproduce the Windows build issue locally, but I guess at this point we all know that this could take some more time to figure out.

Hey @zoq i agree to that so if you feel like it's ok to merge this, feel free to go ahead.

I mean we can use models_tests instead of ctest for the windows build right, I think that should at least work just fine.

[Aakash-kaushik]

Right, we only have to fix the Windows build, so maybe we should patch this out for now and open another pull-request with the changes, just to not delay the PR any longer. I can at least reproduce the Windows build issue locally, but I guess at this point we all know that this could take some more time to figure out.

Hey @zoq i agree to that so if you feel like it's ok to merge this, feel free to go ahead.

I mean we can use models_tests instead of ctest for the windows build right, I think that should at least work just fine.

Yes but for now it fails on the build step so do you suggest that i write specific rules for windows in test cmake and if that's what you meant i can do that in another PR right after this. and we can get this one merged.

[zoq]

Right, we only have to fix the Windows build, so maybe we should patch this out for now and open another pull-request with the changes, just to not delay the PR any longer. I can at least reproduce the Windows build issue locally, but I guess at this point we all know that this could take some more time to figure out.

Hey @zoq i agree to that so if you feel like it's ok to merge this, feel free to go ahead.

I mean we can use models_tests instead of ctest for the windows build right, I think that should at least work just fine.

Yes but for now it fails on the build step so do you suggest that i write specific rules for windows in test cmake and if that's what you meant i can do that in another PR right after this. and we can get this one merged.

Right, in this case we just have to revert some changes.

[Aakash-kaushik]

Right, we only have to fix the Windows build, so maybe we should patch this out for now and open another pull-request with the changes, just to not delay the PR any longer. I can at least reproduce the Windows build issue locally, but I guess at this point we all know that this could take some more time to figure out.

Hey @zoq i agree to that so if you feel like it's ok to merge this, feel free to go ahead.

I mean we can use models_tests instead of ctest for the windows build right, I think that should at least work just fine.

Yes but for now it fails on the build step so do you suggest that i write specific rules for windows in test cmake and if that's what you meant i can do that in another PR right after this. and we can get this one merged.

Right, in this case we just have to revert some changes.

I can change the cmake file so it works exactly as before for windows without the addition of ctests in windows, basically the same old cmake code for windows and the new one for linux ans mac, I will push an update for this in the morning and then you can have a look.

[zoq]

Sounds great to me.

[Aakash-kaushik]

Sounds great to me.

Looks like the windows build is going to pass this time.

[Aakash-kaushik]

Nice, All builds passed :rocket:

[Aakash-kaushik]

Hi @zoq, @rcurtin I have pushed all the requested changes.

[Aakash-kaushik]

Thanks for keeping up with all the comments, I already approved this but looks like I can approve it again, @kartikdutt18 is there anything else from your side?

Looks like kartik needs to approve it too, before it can be merged.