FTorch icon indicating copy to clipboard operation
FTorch copied to clipboard

Introduce integration tests based on the examples

Open joewallwork opened this issue 1 year ago • 4 comments

Closes #115.

Turns existing examples into integration tests using CTest.

joewallwork avatar Apr 15 '24 14:04 joewallwork

That didn't turn out to be too difficult. A few notes:

  1. Couldn't figure out how to get the Python scripts under CTest without making them executable - hopefully not an issue.
  2. The 2nd example depends on numpy but this wasn't in the requirements so I added it.
  3. I used a Python venv approach because that's what I'm familiar with and know how to easily determine the ${Torch_DIR} env variable.
  4. Added a "proof of existence" assert at the end of the pt2ts.py scripts in the first two examples. Maybe this is something useful to have in the version in src, too?
  5. Note that I had to truncate the outputs to 6 d.p. in the 2nd example to get the tests to pass.
  6. Skipped the 3rd demo (multi-GPU) because we don't currently have access to GPUs in the CI.

joewallwork avatar Apr 15 '24 15:04 joewallwork

Can you add some documentation about how to run locally?

dorchard avatar Apr 16 '24 07:04 dorchard

FTorch development meeting discussion:

  • As part of CMake under a test flag create an empty directory in src/ to hold tests.
  • Copy code across from examples
  • build and run tests in the new test directory if test flag is enabled
  • Add the name of this directory to .gitignore in case anyone is using git as a developer etc.

jatkinson1000 avatar Apr 29 '24 10:04 jatkinson1000

@jatkinson1000 @dorchard Okay, I think I addressed the requests above. If FTorch is built with -DCMAKE_BUILD_TESTS=TRUE then the examples get build as integration tests in src/test/examples. The tests can then be run by going to the subdirectory of interest and running ctest, as described in src/test/README.md.

Note that I considered a different approach 115_integration_tests_take2 that moves the CMakeLists.txt to the root level (as well as moving the recommended build directory down one level), rather than copying over example files. I think either could work, just a matter of preference.

(Possibly) still to do:

  • Add information to the user doc pages, as well as the READMEs?
  • Add ctest setup for the MultiGPU example, even if it doesn't get run in the CI?

joewallwork avatar Apr 30 '24 08:04 joewallwork

At the first invocation of cmake build I get a warning that it cannot find FTorch (obviously) for the exercises. Is there any way to suppress this at all?

Please could you share the warning message? I don't think I see such a warning.

During cmake --build . I get a warning about duplicate linking:

ld: warning: ignoring duplicate libraries: '-lemutls_w', '-lgcc'

which would be nice to stop.

Hm, I haven't seen that one. Will look into it.

joewallwork avatar May 31 '24 08:05 joewallwork

It was not immediately clear that I need to go into ./test/examples/examplename/ to invoke ctest. This also feels a bit clunky. I wonder if we could use a shell-script or something to allow users to run the tests from the build or test directory in a one-shot? Feel free to push back if here if you would prefer to get this done and just write clear docs.

Provided a shell script in fe6bbd2, added some text, and hooked it up in the CI test suite. Let me know if it helps.

joewallwork avatar May 31 '24 08:05 joewallwork

Finally, I think there is an element of 'works on my machine' as the regex didn't match on mine causing the tests to fail. Hopefully this can be easily fixed with formatted output...:

@jatkinson1000 please could you rerun these with verbose output and send them over (e.g., in Slack)? So that I can better interpret why the regexes are failing. (For verbose mode, use ctest -V.)

joewallwork avatar May 31 '24 08:05 joewallwork

Okay @jatkinson1000, I added a pages/testing.md page as requested and linked it in elsewhere.

There's a comment above on two new warnings that haven't been addressed. Perhaps we could raise these as issues? Otherwise, ready for re-review.

joewallwork avatar May 31 '24 13:05 joewallwork

Thanks very much for your review @jatkinson1000! Will merge now.

joewallwork avatar May 31 '24 14:05 joewallwork