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

Basic CI support with TravisCI #340

Merged
merged 41 commits into from
Sep 11, 2017
Merged

Conversation

bcumming
Copy link
Member

@bcumming bcumming commented Sep 1, 2017

Add support for continuous integration with Travis CI.
This implements bare bones support that can be extended over time.

Travis CI test environments:

  • All use gcc 5.
  • Test the serial distributed back end with serial and cthread threading backends.
  • Test mpi with cthread.
  • The tbb test failed sporadically because CMake, so it is disabled for now.

The test script:

  • Builds the unit tests, global_communication tests and miniapp.
  • Asserts that all unit and global_communication tests pass.
  • Asserts that the miniapp runs successfully.
    • does not test miniapp output for now.

There is plenty of scope for improving the tests.
A key improvement will be to use validated output for the validation and miniapp
to provide some validation.

There were some small fixes required to make the tests pass on Travis

  • communication/mpi.hpp now sets default size and rank values of 1 and 0 respectively
    to allow all unit tests to pass when built with MPI.
  • The wrappers around MPI API calls use const_cast to support MPI implementations that
    are not "const aware".
  • A missing header was added to tests/unit/test_range to make std::unordered_multimap
    available.`

buffer.data(), counts.data(), displs.data(), traits::mpi_type(), // receive
root, MPI_COMM_WORLD);
MPI_Gatherv(
// const_cast required for MPI implementations that don't use const* in their interfaces
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also #define MPICH2_CONST (See CMake for for BGQ)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was to work around an ancient version of OpenMPI provided by Ubuntu/TravisCI.

@@ -8,8 +8,8 @@ namespace mpi {

// global state
namespace state {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do the initial values here mean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are sane defaults (1 mpi rank with rank 0), to make the unit tests pass when compiled with MPI. The unit tests don't initialize MPI, so the default signed values of -1 were being converted to there unsigned version in the domain_decomposition unit test (the one in tests.exe, not the global_communication.exe test runner which initializes MPI).

This is not a permanent solution: @halfflat and I plan to use "executors", which will be more flexible than the current hard-coded compile time approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah -- I see, if the init is never called, it pretends to be rank 0 of a world size 1.
Could we just comment that for the time being, so that if someone trips over it, they won't be confused? -1, -1 says to me that if init is never called, mark it with illegal values -- but the new version has unclear, far away implications.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a comment, marking it as a TODO that should be removed when we add the run time executors.

Copy link
Contributor

@apeyser apeyser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment.

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

2 participants