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

Feature/mpi sendrecv replace #57

Merged

Conversation

andreapiacentini
Copy link
Contributor

This is a tentative implementation of issue #53
It most probably will require some adjustment by the reviewers, especially for the Serial implementation.
Nevertheless, this feature is quite urgently needed for an oops development.
Thanks

@FussyDuck
Copy link

FussyDuck commented Feb 22, 2023

CLA assistant check
All committers have signed the CLA.

@dtip
Copy link
Contributor

dtip commented Feb 24, 2023

Hi @andreapiacentini - thanks for this PR! Could you sign the ECMWF CLA please.

@wdeconinck
Copy link
Member

Hello @andreapiacentini that looks good to me.
But please could you add a unit test both for parallel and serial execution?
I will then test it out locally with MPI before approving, because the eckit Github-actions-CI does not use MPI (yet).

Copy link
Member

@wdeconinck wdeconinck left a comment

Choose a reason for hiding this comment

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

Unit-test required

@wdeconinck
Copy link
Member

@dtip do you have any clue why CI is failing? It does not seem related to this PR.

@dtip
Copy link
Contributor

dtip commented Feb 27, 2023

@dtip do you have any clue why CI is failing? It does not seem related to this PR.

Yep, it's a permissions issue with accessing the secrets we need to run CI from this public PR. We'll update the CI shortly to fix it.

@dtip
Copy link
Contributor

dtip commented Feb 27, 2023

@wdeconinck do we want to also modify the CI to compile and test both with and without MPI enabled?

@andreapiacentini
Copy link
Contributor Author

@wdeconinck I've commited unit tests for these features.
The MPI test passes and I am happy with that.
The serial test passes too and I am pleasantly astonished. It passes as well if I comment out all the contents of the serial implementation of sendReceiveReplace and I just return an empty Status.
Most probably it is telling me that a "Replace" function is completely meaningless in serial mode. What do you think?
If you agree on that, please proceed to clipping the serial implementation.

@dtip
Copy link
Contributor

dtip commented Feb 27, 2023

We've (hopefully) just fixed the CI issue in #58

@andreapiacentini could you rebase this PR onto the latest develop branch please, then @wdeconinck can approve the CI run.

@andreapiacentini
Copy link
Contributor Author

@dtip I did it via the github "Sync fork" link.
I hope it worked ok.
Thanks

@wdeconinck
Copy link
Member

@wdeconinck do we want to also modify the CI to compile and test both with and without MPI enabled?

I would definitely suggest to enable MPI. For Atlas and fckit this was enough reason to build eckit as an internal CI cache rather than reuse an existing artifact. It would also be able to test MPI feature changes here rather than offline.

@tlmquintino
Copy link
Member

@wdeconinck do we want to also modify the CI to compile and test both with and without MPI enabled?

I would definitely suggest to enable MPI. For Atlas and fckit this was enough reason to build eckit as an internal CI cache rather than reuse an existing artifact. It would also be able to test MPI feature changes here rather than offline.

I think we need to have test with and without MPI. This should be part of the matrix of builds, and we did discuss the possibility of having different builds based on options not only on platforms x compilers.

@dtip
Copy link
Contributor

dtip commented Feb 27, 2023

I think we need to have test with and without MPI. This should be part of the matrix of builds, and we did discuss the possibility of having different builds based on options not only on platforms x compilers.

Exactly - we can build both with and without MPI with a matrix of build options.

Copy link
Member

@wdeconinck wdeconinck left a comment

Choose a reason for hiding this comment

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

I have added some cosmetic changes, and tested both with Parallel and Serial backend. All seems to be in good shape.

@wdeconinck
Copy link
Member

@dtip something is not right with the GitHub token

@dtip
Copy link
Contributor

dtip commented Feb 28, 2023

@wdeconinck Yep, the fix we made yesterday didn't work so we've had to revert it. If you're confident with the PR, please feel free to merge it. Otherwise, if you want to run CI, the temporary workaround is to push the come from this PR to a branch within the eckit repo, then open a new PR.

@wdeconinck wdeconinck merged commit 2bda26c into ecmwf:develop Feb 28, 2023
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.

5 participants