[Core] Add `SetRank` method to `GlobalPointer` + documentation by loumalouomega · Pull Request #11762 · KratosMultiphysics/Kratos

loumalouomega · 2023-11-03T10:11:38Z

📝 Description

Add SetRank method to GlobalPointer, plus adding documentation

🆕 Changelog

Add SetRank method to GlobalPointer + documentation

roigcarlo · 2023-11-03T10:16:24Z

Why exactly do you need to change the rank of a global pointer?

GP were designed to be created in the partition they belong, its hard for me to find a use case in which this construction is valid and does not lead to problems.

loumalouomega · 2023-11-03T10:18:54Z

Why exactly do you need to change the rank of a global pointer?

GP were designed to be created in the partition they belong, its hard for me to find a use case in which this construction is valid and does not lead to problems.

Because somethimes the rank is not provided in construction time

roigcarlo · 2023-11-03T10:20:15Z

Well yes, but can you elaborate? how can you not have the rank in the processor its mean to create the GP?

loumalouomega · 2023-11-03T10:23:55Z

Well yes, but can you elaborate? how can you not have the rank in the processor its mean to create the GP?

Because the GeometricalObjectBins generates SpatialSearchResult, which contains a GlobalPointer, but does not provide the rank, so it is assumed to be 0 always.

See: d6873a9

roigcarlo · 2023-11-03T10:47:38Z

if this is the problem I would personally change the global_pointer.h default to something like:

GlobalPointer(TDataType data, int rank = ParallelEnvironment::GetDefaultRank())

But maybe I am biased here, ping @KratosMultiphysics/mpi

loumalouomega · 2023-11-03T10:49:12Z

if this is the problem I would personally change the global_pointer.h default to something like:
GlobalPointer(TDataType data, int rank = ParallelEnvironment::GetDefaultRank())
But maybe I am biased here, ping @KratosMultiphysics/mpi

That will not work, because creates a MPI dependency, will fail on linking time

roigcarlo · 2023-11-03T11:07:39Z

It does not work like that

When you start Kratos with mpi, the first time the kernel is loaded you will call:

# __init__.py:67
mpi.InitializeMPIParallelRun()

This code lives in the MPI extension of the core, and among other things does:

// Define the World DataCommunicator as a wrapper for MPI_COMM_WORLD and make it the default.
ParallelEnvironment::RegisterDataCommunicator("World", MPIDataCommunicator::Create(MPI_COMM_WORLD), ParallelEnvironment::MakeDefault);

This will invoke the code from:

void ParallelEnvironment::SetAsDefault(
    std::unordered_map<std::string, DataCommunicator::UniquePointer>::iterator& rThisCommunicator)
{
    mDefaultCommunicator = rThisCommunicator;
    const auto& r_comm = *(rThisCommunicator->second);
    mDefaultRank = r_comm.Rank();
    mDefaultSize = r_comm.Size();
}

Which will set the mDefaultcommunicator, mDefaultRank and mDefaultSize be the ones from the communicator you just created.

It does not matter that the implementation this part is MPI dependent, because it does not live in the same shared lib as the rest of KratosCore, but it will change the default registered communicator.

When you call ParallelEnvironment::GetDefaultRank(), (which does not depend on mpi) you will access proper process rank, size, or mpi communicator, even if they are mpi dependent.

roigcarlo · 2023-11-03T11:13:24Z

Btw, I will admit that SetRank would be usefull in one particular situation, which is trying to transfer GP which belong to more than one communicator (Kratos default one + other custom registered comm) with different rank ids. And even in those i am not sure if its better to create another copy of the pointer when in the context of the correct communicator.

loumalouomega · 2023-11-03T11:57:57Z

It does not work like that

When you start Kratos with mpi, the first time the kernel is loaded you will call:
# __init__.py:67
mpi.InitializeMPIParallelRun()
This code lives in the MPI extension of the core, and among other things does:
// Define the World DataCommunicator as a wrapper for MPI_COMM_WORLD and make it the default.
ParallelEnvironment::RegisterDataCommunicator("World", MPIDataCommunicator::Create(MPI_COMM_WORLD), ParallelEnvironment::MakeDefault);
This will invoke the code from:
void ParallelEnvironment::SetAsDefault(
    std::unordered_map<std::string, DataCommunicator::UniquePointer>::iterator& rThisCommunicator)
{
    mDefaultCommunicator = rThisCommunicator;
    const auto& r_comm = *(rThisCommunicator->second);
    mDefaultRank = r_comm.Rank();
    mDefaultSize = r_comm.Size();
}
Which will set the mDefaultcommunicator, mDefaultRank and mDefaultSize be the ones from the communicator you just created.

It does not matter that the implementation this part is MPI dependent, because it does not live in the same shared lib as the rest of KratosCore, but it will change the default registered communicator.

When you call ParallelEnvironment::GetDefaultRank(), (which does not depend on mpi) you will access proper process rank, size, or mpi communicator, even if they are mpi dependent.

I will try

roigcarlo · 2023-11-03T11:59:53Z

Well let see what the rest of people say, maybe I am being paranoid and setrank is fine for everyone...

loumalouomega · 2023-11-03T13:02:20Z

It does not work like that
When you start Kratos with mpi, the first time the kernel is loaded you will call:
# __init__.py:67
mpi.InitializeMPIParallelRun()
This code lives in the MPI extension of the core, and among other things does:
// Define the World DataCommunicator as a wrapper for MPI_COMM_WORLD and make it the default.
ParallelEnvironment::RegisterDataCommunicator("World", MPIDataCommunicator::Create(MPI_COMM_WORLD), ParallelEnvironment::MakeDefault);
This will invoke the code from:
void ParallelEnvironment::SetAsDefault(
    std::unordered_map<std::string, DataCommunicator::UniquePointer>::iterator& rThisCommunicator)
{
    mDefaultCommunicator = rThisCommunicator;
    const auto& r_comm = *(rThisCommunicator->second);
    mDefaultRank = r_comm.Rank();
    mDefaultSize = r_comm.Size();
}
Which will set the mDefaultcommunicator, mDefaultRank and mDefaultSize be the ones from the communicator you just created.
It does not matter that the implementation this part is MPI dependent, because it does not live in the same shared lib as the rest of KratosCore, but it will change the default registered communicator.
When you call ParallelEnvironment::GetDefaultRank(), (which does not depend on mpi) you will access proper process rank, size, or mpi communicator, even if they are mpi dependent.
I will try

It is impossible, I need many forward declarations in many different classes (20 files I have right now). It is easier to just simply keet the SetRank method

roigcarlo · 2023-11-03T14:00:26Z

Actually, the problem is the FillCommunicator. Let me take a look

loumalouomega · 2023-11-03T14:08:17Z

Actually, the problem is the FillCommunicator. Let me take a look

yes, but I needed to change many files

roigcarlo

Ok to many things are wrong in the core. Lets accept this for now and I will take care of unraveling the kernel

loumalouomega · 2023-11-03T15:24:57Z

Ok to many things are wrong in the core. Lets accept this for now and I will take care of unraveling the kernel

Indeed..., many many cross dependencies. I think we should reconsider the forward declaration policy to begin with

RiccardoRossi · 2023-11-06T09:52:29Z

kratos/includes/global_pointer.h

+        mDataPointer = nullptr;
 #ifdef KRATOS_USING_MPI
-  int mRank;
+        this->mRank = 0;


why do you want to initialize it? if it is not a correct value better leave it undefined no?

The diff is terrible, the code was already like that , see line 48 in previous implementation

RiccardoRossi · 2023-11-06T09:55:36Z

kratos/includes/global_pointer.h

+     */
+    void SetRank(const int Rank)
+    {
 #ifdef KRATOS_USING_MPI


well, i guess we should have a debug error here in case we are not in mpi mode

And rank is different than 0, because if you implement a generic code in serial it will set always to 0, otherwise you will create many false errors

well, imagine you try to set something to rank 5 in serial... that would be silently ignored

RiccardoRossi · 2023-11-06T09:56:37Z

Ok to many things are wrong in the core. Lets accept this for now and I will take care of unraveling the kernel

Indeed..., many many cross dependencies. I think we should reconsider the forward declaration policy to begin with

i am fundamentally against it!

loumalouomega · 2023-11-06T10:04:57Z

Ok to many things are wrong in the core. Lets accept this for now and I will take care of unraveling the kernel

Indeed..., many many cross dependencies. I think we should reconsider the forward declaration policy to begin with

i am fundamentally against it!

In that case we will have longer compilation times and cross dependencies issues all along the core

RiccardoRossi · 2023-11-06T10:24:14Z

well, we will need to solve those... forward declaration is really a dangerous thing ...

loumalouomega · 2023-11-06T10:26:19Z

well, we will need to solve those... forward declaration is really a dangerous thing ...

When it is dangerous?, if a same class is defined diferently in different files?, a priori you will have linking problems

RiccardoRossi · 2023-11-06T10:28:34Z

for example the dependency check mechanism (includes) gets broken by forward declarations

loumalouomega · 2023-11-06T10:36:41Z

for example the dependency check mechanism (includes) gets broken by forward declarations

I think if you what are you doing should not be problematic, and we approve changes little by little

RiccardoRossi · 2023-11-06T10:54:19Z

well, i ping @KratosMultiphysics/technical-committee ... but i anticpate you that i am completely agains this

loumalouomega · 2023-11-06T10:57:10Z

well, i ping @KratosMultiphysics/technical-committee ... but i anticpate you that i am completely agains this

I understand, but @roigcarlo also found due to this PR problems related with cross dependencies difficult to solve without this technique

[Core] Add SetRank method to GlobalPointer + documentation

bd8951c

loumalouomega added Kratos Core Documentation Incomplete Parallel-MPI Distributed memory parallelism for HPC / clusters FastPR This Pr is simple and / or has been already tested and the revision should be fast Inconsistent labels Nov 3, 2023

loumalouomega requested a review from roigcarlo November 3, 2023 10:11

loumalouomega requested a review from a team as a code owner November 3, 2023 10:11

loumalouomega mentioned this pull request Nov 3, 2023

[Core][MPI] Adding SpatialSearchResultContainer/SpatialSearchResultContainerVector and SearchWrapper (v.4.5) #11719

Open

roigcarlo approved these changes Nov 3, 2023

View reviewed changes

loumalouomega merged commit c567de0 into master Nov 3, 2023

loumalouomega deleted the core/mpi/global-pointer-setrank-documentation branch November 3, 2023 21:37

RiccardoRossi reviewed Nov 6, 2023

View reviewed changes

Conversation

loumalouomega commented Nov 3, 2023

Uh oh!

roigcarlo commented Nov 3, 2023

Uh oh!

loumalouomega commented Nov 3, 2023

Uh oh!

roigcarlo commented Nov 3, 2023

Uh oh!

loumalouomega commented Nov 3, 2023

Uh oh!

roigcarlo commented Nov 3, 2023

Uh oh!

loumalouomega commented Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roigcarlo commented Nov 3, 2023

Uh oh!

roigcarlo commented Nov 3, 2023

Uh oh!

loumalouomega commented Nov 3, 2023

Uh oh!

roigcarlo commented Nov 3, 2023

Uh oh!

loumalouomega commented Nov 3, 2023

Uh oh!

roigcarlo commented Nov 3, 2023

Uh oh!

loumalouomega commented Nov 3, 2023

Uh oh!

roigcarlo left a comment

Choose a reason for hiding this comment

Uh oh!

loumalouomega commented Nov 3, 2023

Uh oh!

RiccardoRossi Nov 6, 2023

Choose a reason for hiding this comment

Uh oh!

loumalouomega Nov 6, 2023

Choose a reason for hiding this comment

Uh oh!

RiccardoRossi Nov 6, 2023

Choose a reason for hiding this comment

Uh oh!

loumalouomega Nov 6, 2023

Choose a reason for hiding this comment

Uh oh!

RiccardoRossi Nov 6, 2023

Choose a reason for hiding this comment

Uh oh!

RiccardoRossi commented Nov 6, 2023

Uh oh!

loumalouomega commented Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RiccardoRossi commented Nov 6, 2023

Uh oh!

loumalouomega commented Nov 6, 2023

Uh oh!

RiccardoRossi commented Nov 6, 2023

Uh oh!

loumalouomega commented Nov 6, 2023

Uh oh!

RiccardoRossi commented Nov 6, 2023

Uh oh!

loumalouomega commented Nov 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

loumalouomega commented Nov 3, 2023 •

edited

Loading

loumalouomega commented Nov 6, 2023 •

edited

Loading