Skip to content

Winter is coming... #46

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

Merged
merged 121 commits into from
Feb 22, 2018
Merged

Winter is coming... #46

merged 121 commits into from
Feb 22, 2018

Conversation

aminiussi
Copy link
Member

Right now, develop passes all its testing with both develop and master branch of serialization.
Having both is a security I think.

They pass on g++5.2.0 OpenMpi on ubuntu and Intel 15.0.3 (based on g++4.4.7 lib, pretty antique, which is reassuring for compatibility) default mode and intel MPI on CentOS
I think we should check both default compile option and explicit C++11 activation.

I can try intel 17 on CentOS7.

@Belcourt I think you have access to other platforms, I'm afraid I do not have access to MS environment.

aminiussi and others added 30 commits September 4, 2014 14:12
The API should be complete although primitive.
No documentation yet.
Removed a few "unsigned int" comparuson warnings with g++
Splitted bug test_main function
Conflicts:
	src/communicator.cpp
Fix: adapt to renaming of serialization::array_wrapper

Tested that both graph_parallel and MPI work fine with El Capitan and clang, thanks for the patch!
Newer MPI uses MPI_Comm_set_errhandler and MPI_Comm_get_attr.
Added test case to send recv a vector of udt with an
overload of get_mpi_datatype.  Added test to Jamfile
for nightly testing.
Add remark in Jamfile that test case requires -std=c++11.
@Lastique
Copy link
Member

Lastique commented Jul 2, 2017

@aminiussi, I think you are one of Boost.MPI maintainers, aren't you? If you feel #47 and #46 are ready for master, you could merge them yourself.

IMO, merging the fixes from develop and #47 is better than doing nothing since Boost.MPI is completely broken in 1.64 and will be still as broken in 1.65.

@aminiussi
Copy link
Member Author

Yes, I saw a few months ago I was in that list. But being both the submitter/reviewer/executioner of those pull requests makes me uncomfortable. If I did, it would be in opposition with the current cherry pick release model (which I do not approve, but still) without having heard of the exacts problems/platforms mentioned by @Belcourt.
Also, it would be mostly based on the tests I can run locally.
http://www.boost.org/development/tests/develop/developer/mpi.html shows failures with no useful details (some are clearly configuration issues, but for other we do not know). I could re-activate my test to show some results on the web page, but that is quite some work and I'm a little reluctant putting in the effort if the conclusion is that the work won't go to master for some unknown regression on some unknown platform (I asked but got no answer yet, see this discussion for details).
So, I'll try to relauch my test, but would like to have some inputs regarding what people thing we should do and have as much feed back as possible regarding the test. Does anybody knows who's running these Sandia failing test at: http://www.boost.org/development/tests/develop/developer/mpi.html .

@Lastique
Copy link
Member

Lastique commented Jul 3, 2017

Well, at least merging #47 to develop would be desirable so that it gets tested and fixes a few test failures.

Regarding community opinion I think it's best to write a message to the Boost developers mailing list. Personally, as I mentioned, I think the changes make the library "better" (at least, it compiles) and therefore should be merged before the 1.65 release.

Regarding Sandia, I think @Belcourt manages these testers. You can also request test logs through the dev ML.

@Rombur
Copy link

Rombur commented Feb 13, 2018

Is there any plan to merge this PR? Right now I have to patch boost mpi to be able to use it with nvcc (this has been fixed a year ago in this branch)

@aminiussi
Copy link
Member Author

aminiussi commented Feb 15, 2018 via email

@dalg24
Copy link

dalg24 commented Feb 20, 2018

@aminiussi would you please make sure 90e84fe makes it into 1.67.0?

@danieljames
Copy link
Contributor

If this is a breaking change, then it really should be merged today, if not then there's a week left. If that isn't possible, let us (the release managers) know.

@aminiussi
Copy link
Member Author

aminiussi commented Feb 20, 2018 via email

@danieljames
Copy link
Contributor

I'm not keen on massive merges, but if it's unavoidable then I think the important thing is to try to get people to test the beta.

@Lastique
Copy link
Member

IMHO, it's been tested in develop for ages. It doesn't matter how big it is, we should accept develop as the "best" Boost.MPI version there is and just merge it. Otherwise, if develop is considered broken somehow then let's revert it to master state.

@danieljames
Copy link
Contributor

I don't think the length of time they've been tested for makes much of a difference, as it's always the same tests. I think there were problems with pointer containers in the last release, which was a merge of old develop changes.

@Belcourt
Copy link
Member

Belcourt commented Feb 20, 2018 via email

@aminiussi
Copy link
Member Author

aminiussi commented Feb 20, 2018 via email

@danieljames
Copy link
Contributor

Sorry, pointer containers is another library, not your problem. I was just using it to illustrate a point.

I'm afraid it's up to you whether or not develop is good enough. But don't be over-cautious, it looks like people want these improvements. We understand that you're in a difficult position and appreciate your work. I hope I wasn't discouraging.

@aminiussi
Copy link
Member Author

aminiussi commented Feb 20, 2018 via email

@Lastique
Copy link
Member

I think there were problems with pointer containers in the last release, which was a merge of old develop changes.

Even if so, I assume those problems were localized and sorted out. Which ultimately resulted in a better code rather than stagnation.

@aminiussi
Copy link
Member Author

aminiussi commented Feb 21, 2018 via email

@aminiussi aminiussi merged commit 9e3cadb into master Feb 22, 2018
aminiussi added a commit that referenced this pull request Feb 22, 2018
@aminiussi
Copy link
Member Author

aminiussi commented Feb 22, 2018 via email

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.