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.

Sign up for GitHub

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

[Core] Making aplications deregistrable V3 #12306

Merged
merged 16 commits into from
Jun 5, 2024
Merged

[Core] Making aplications deregistrable V3 #12306

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
4d4ed88
Alternative implementation
roigcarlo Apr 23, 2024
fac137a
Missing geometries
roigcarlo Apr 26, 2024
a9a29d3
Change proposal based on comments
roigcarlo May 8, 2024
9694b6a
Fixed loop over invalidated iterators
roigcarlo May 9, 2024
ecd6e0b
Removing mCurrentSource
roigcarlo May 9, 2024
c609e2c
Previnting the de-register of non existing keys
roigcarlo May 10, 2024
117b3bf
fixing possible auto deduction failure in lambda
roigcarlo May 10, 2024
8d77e96
Missing def
roigcarlo May 10, 2024
21aae0f
Trying more simple approach
roigcarlo May 10, 2024
02df3ac
Implementing current context as a key extension to avoid regression i…
roigcarlo May 15, 2024
b637f0d
typo
roigcarlo May 21, 2024
0fc4296
Prevent std::bad_any cast due to string_view to invalid string refere…
roigcarlo May 21, 2024
79f8840
Disable deregister until tests (Failures in previous commit were expe…
roigcarlo May 21, 2024
aaaacdb
Missing .
roigcarlo May 21, 2024
6b7fbe9
Probably a bug in gcc
roigcarlo May 22, 2024
701be9b
Missing
roigcarlo May 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 25 additions & 10 deletions kratos/includes/define.h
View file
Open in desktop
roigcarlo marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -703,36 +703,51 @@ catch(...) { Block KRATOS_THROW_ERROR(std::runtime_error, "Unknown error", MoreI
#ifdef KRATOS_REGISTER_ELEMENT
#undef KRATOS_REGISTER_ELEMENT
#endif
#define KRATOS_REGISTER_ELEMENT(name, reference) \
KratosComponents<Element >::Add(name, reference); \
#define KRATOS_REGISTER_ELEMENT(name, reference) \
KratosComponents<Element>::Add(name, reference); \
if(!Registry::HasItem("elements."+Registry::GetCurrentSource()+"."+name)){ \
roigcarlo marked this conversation as resolved.
Show resolved Hide resolved
Registry::AddItem<RegistryItem>("elements."+Registry::GetCurrentSource()+"."+name); \
} \
Serializer::Register(name, reference);

#ifdef KRATOS_REGISTER_CONDITION
#undef KRATOS_REGISTER_CONDITION
#endif
#define KRATOS_REGISTER_CONDITION(name, reference) \
KratosComponents<Condition >::Add(name, reference); \
#define KRATOS_REGISTER_CONDITION(name, reference) \
KratosComponents<Condition>::Add(name, reference); \
if(!Registry::HasItem("conditions."+Registry::GetCurrentSource()+"."+name)){ \
Registry::AddItem<RegistryItem>("conditions."+Registry::GetCurrentSource()+"."+name); \
} \
Serializer::Register(name, reference);

#ifdef KRATOS_REGISTER_CONSTRAINT
#undef KRATOS_REGISTER_CONSTRAINT
#endif
#define KRATOS_REGISTER_CONSTRAINT(name, reference) \
KratosComponents<MasterSlaveConstraint >::Add(name, reference); \
#define KRATOS_REGISTER_CONSTRAINT(name, reference) \
KratosComponents<MasterSlaveConstraint>::Add(name, reference); \
if(!Registry::HasItem("constraints."+Registry::GetCurrentSource()+"."+name)){ \
Registry::AddItem<RegistryItem>("constraints."+Registry::GetCurrentSource()+"."+name); \
} \
Serializer::Register(name, reference);

#ifdef KRATOS_REGISTER_MODELER
#undef KRATOS_REGISTER_MODELER
#endif
#define KRATOS_REGISTER_MODELER(name, reference) \
KratosComponents<Modeler>::Add(name, reference); \
#define KRATOS_REGISTER_MODELER(name, reference) \
KratosComponents<Modeler>::Add(name, reference); \
if(!Registry::HasItem("modelers."+Registry::GetCurrentSource()+"."+name)){ \
Registry::AddItem<RegistryItem>("modelers."+Registry::GetCurrentSource()+"."+name); \
} \
Serializer::Register(name, reference);

#ifdef KRATOS_REGISTER_CONSTITUTIVE_LAW
#undef KRATOS_REGISTER_CONSTITUTIVE_LAW
#endif
#define KRATOS_REGISTER_CONSTITUTIVE_LAW(name, reference) \
KratosComponents<ConstitutiveLaw >::Add(name, reference); \
#define KRATOS_REGISTER_CONSTITUTIVE_LAW(name, reference) \
KratosComponents<ConstitutiveLaw>::Add(name, reference); \
if(!Registry::HasItem("constitutive_laws."+Registry::GetCurrentSource()+"."+name)){ \
Registry::AddItem<RegistryItem>("constitutive_laws."+Registry::GetCurrentSource()+"."+name); \
} \
Serializer::Register(name, reference);

#define KRATOS_DEPRECATED [[deprecated]]
Expand Down
26 changes: 25 additions & 1 deletion kratos/includes/kratos_application.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,12 @@ class KRATOS_API(KRATOS_CORE) KratosApplication {
mpModelers(rOther.mpModelers) {}

/// Destructor.
virtual ~KratosApplication() {}
virtual ~KratosApplication()
{
// This must be commented until tests have been fixed.
DeregisterCommonComponents();
DeregisterApplication();
}

///@}
///@name Operations
Expand All @@ -141,6 +146,25 @@ class KRATOS_API(KRATOS_CORE) KratosApplication {

void RegisterKratosCore();

/**
* @brief This method is used to unregister common components of the application.
* @details This method is used to unregister common components of the application.
* The list of unregistered components are the ones exposed in the common KratosComponents interface:
Comment on lines +153 to +155
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, I would suggest to use "deregister" everywhere, rather than mixing it with "unregister".

Suggested change
* @brief This method is used to unregister common components of the application.
* @details This method is used to unregister common components of the application.
* The list of unregistered components are the ones exposed in the common KratosComponents interface:
* @brief This method is used to deregister common components of the application.
* @details This method is used to deregister common components of the application.
* The list of deregistered components are the ones exposed in the common KratosComponents interface:

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree. It will be done like in #12281, just wanted to avoid having more changes than the necessary here as I got a lot of comments about renaming in the other PR

* - Geometries
* - Elements
* - Conditions
* - MasterSlaveConstraints
* - Modelers
* - ConstitutiveLaws
*/
void DeregisterCommonComponents();

/**
* @brief This method is used to unregister specific application components.
* @details This method is used to unregister specific application components.
*/
virtual void DeregisterApplication();

///////////////////////////////////////////////////////////////////
void RegisterVariables(); // This contains the whole list of common variables in the Kratos Core
void RegisterDeprecatedVariables(); //TODO: remove, this variables should not be there
Expand Down
13 changes: 13 additions & 0 deletions kratos/includes/registry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,18 @@ class KRATOS_API(KRATOS_CORE) Registry final

static void RemoveItem(std::string const& ItemName);

/** Sets the current source of the registry
* This function is used to keep track of which application is adding items to the registry
* @param rCurrentSource The current source of the registry
*/
Comment on lines +154 to +157
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure someone will get what this means by the comment... :/

static void SetCurrentSource(std::string const & rCurrentSource);

/** Gets the current source of the registry
* This function is used to keep track of which application is adding items to the registry
* @param return The current source of the registry
*/
static std::string GetCurrentSource();

///@}
///@name Inquiry
///@{
Expand Down Expand Up @@ -189,6 +201,7 @@ class KRATOS_API(KRATOS_CORE) Registry final
///@{

static RegistryItem* mspRootRegistryItem;
static std::string mCurrentSource;

///@}
///@name Member Variables
Expand Down
38 changes: 37 additions & 1 deletion kratos/sources/kratos_application.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,16 @@ KratosApplication::KratosApplication(const std::string& ApplicationName)
mpConditions(KratosComponents<Condition>::pGetComponents()),
mpModelers(KratosComponents<Modeler>::pGetComponents()),
mpRegisteredObjects(&(Serializer::GetRegisteredObjects())),
mpRegisteredObjectsName(&(Serializer::GetRegisteredObjectsName())) {}
mpRegisteredObjectsName(&(Serializer::GetRegisteredObjectsName())) {

Registry::SetCurrentSource(mApplicationName);
Copy link
Member

Choose a reason for hiding this comment

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

It is me or the spacing is inconsistent?

Copy link
Member Author

Choose a reason for hiding this comment

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

is consistent, the inconsistent one is the initializer which is indented at 10 chars to have it in the same column as mApplicationName. We could change it in a different PR


for (auto component : {"geometries", "elements", "conditions", "constraints", "modelers", "constitutive_laws"}) {
if (!Registry::HasItem(std::string(component))) {
Registry::AddItem<RegistryItem>(std::string(component)+"."+mApplicationName);
}
}
}
Comment on lines +123 to +128
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this? I think, the registry will create all the parent keys if they are not present ryt? Is this done for consistency?


void KratosApplication::RegisterKratosCore() {

Expand Down Expand Up @@ -295,4 +304,31 @@ void KratosApplication::RegisterKratosCore() {
// Register ConstitutiveLaw BaseClass
KRATOS_REGISTER_CONSTITUTIVE_LAW("ConstitutiveLaw", mConstitutiveLaw);
}

void KratosApplication::DeregisterCommonComponents()
{
Copy link
Member

Choose a reason for hiding this comment

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

Here, if we store the list of added items in the [Registry::GetCurrentSource()].deregister_list then we can only iterate over it and remove them from registry without even distinguishing the type.

Copy link
Member Author

Choose a reason for hiding this comment

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

We need the type to instantiate the proper KratosComponents template or I am missing something?

Copy link
Member

Choose a reason for hiding this comment

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

You are right! Still we need that for components

KRATOS_INFO("") << "Deregistering " << mApplicationName << std::endl;

auto deregister_detail = [&](std::string const & rComponentName, auto remove_detail) {
auto path = std::string(rComponentName)+"."+mApplicationName;
std::cout << "Deregistering " << path << " components" << std::endl;
roigcarlo marked this conversation as resolved.
Show resolved Hide resolved
for (auto & key : Registry::GetItem(path)) {
std::cout << "\t" << key.first << std::endl;
remove_detail(key.first);
}
};

deregister_detail("geometries", [](std::string const & key){ KratosComponents<Geometry<Node>>::Remove(key);});
deregister_detail("elements", [](std::string const & key){ KratosComponents<Element>::Remove(key);});
deregister_detail("conditions", [](std::string const & key){ KratosComponents<Condition>::Remove(key);});
deregister_detail("constraints", [](std::string const & key){ KratosComponents<MasterSlaveConstraint>::Remove(key);});
deregister_detail("modelers", [](std::string const & key){ KratosComponents<Modeler>::Remove(key);});
deregister_detail("constitutive_laws", [](std::string const & key){ KratosComponents<ConstitutiveLaw>::Remove(key);});
}

void KratosApplication::DeregisterApplication() {
// DeregisterLinearSolvers();
// DeregisterPreconditioners();
}

} // namespace Kratos.
11 changes: 11 additions & 0 deletions kratos/sources/registry.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ namespace
}

RegistryItem* Registry::mspRootRegistryItem = nullptr;
std::string Registry::mCurrentSource = "all";
roigcarlo marked this conversation as resolved.
Show resolved Hide resolved

RegistryItem& Registry::GetItem(std::string const& rItemFullName)
{
Expand Down Expand Up @@ -80,6 +81,16 @@ namespace
}
}

void Registry::SetCurrentSource(std::string const & rCurrentSource)
{
mCurrentSource = rCurrentSource;
}

std::string Registry::GetCurrentSource()
{
return mCurrentSource;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

mCurrentSource is a static data member of the class. These access functions don't seem to be thread-safe to me. Not sure how important that is here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fine, registering an application should not be done in parallel and the registry itself relies on being static.

In this regard we may want to implement Registry as a singleton and have a thread lock in the setters/getters. Ping @pooyan-dadvand.

Copy link
Member

Choose a reason for hiding this comment

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

The registry has a thread safe double lock guarded implementation. For this one, I assume that the creation of applications is serial, but making it thread safe don't harm anyone

Copy link
Member

Choose a reason for hiding this comment

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

Thinking more about it, I think it would be better to first register the application in the registry and keep the entry as an static member of the application itself:

static RegistryItem& mRegisteredApplication = Registry::AddItem<RegistryItem>("FluidDynamics");

Then we can use this name everywhere using the Application base class member function GetApplicationName (you may find better names for them to be coherent also in the core)

In the core we can use KratosCore or Core as the name.

Then we can add all items not only to their type but also as a reference under this branch (optional) and have a deregistry_list with all added items to be removed:

application_name -- elements [ ] // optional but can be useful to find the source of certain component
                 |- conditions 
                 |- ...
                 |- deregsitery_list -- elements // only the ones that we added
                                     |- conditions   // only what we added
                                     |- geometries  // ....
                                     |- .....

With all of this in place, the deregistry function can be implemented in a generic way in the base class or as a utility

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 think I get the idea but I have a comment:

Then we can use this name everywhere using the Application base class member function GetApplicationName (you may find better names for them to be coherent also in the core)

Essentially, this information is already available in https://github.com/KratosMultiphysics/Kratos/blob/master/kratos/includes/kratos_application.h#L155. The motivation for adding a mCurrentSource is to get access to this information outside the application context (for example, for registering preconditioners https://github.com/KratosMultiphysics/Kratos/blob/master/kratos/factories/standard_preconditioner_factory.cpp#L47-L50 )

With that being said, what we can do is to store this "current application" inside the registry itself instead of being a static member. That will also solve the thread-safety concerns. Modifying a bit your proposal would be like:

application_context --> @app_name
application_name -- elements [ ] // optional but can be useful to find the source of certain component
                 |- conditions 
                 |- ...
                 |- deregsitery_list -- elements // only the ones that we added
                                     |- conditions   // only what we added
                                     |- geometries  // ....
                                     |- .....

And then the registering process would be like:

auto & app_context = Registry::GetItem<RegistryItem>("ApplicationContext");
if (Registry::HasItem("elements."+app_context+"."+name)) ) // or whatever access.

Copy link
Member Author

@roigcarlo roigcarlo May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Btw I remember that we agreed that registry keys should start with the component name and then the application name (process.all.xxxx, process.FluidDynamics.yyyyy), so it should be:

application_context --> @app_name
elements 
    |- application_name
        |- deregsitery_list
conditions 
    |- application_name
        |- deregsitery_list


std::size_t Registry::size()
{
return mspRootRegistryItem->size();
Expand Down
Loading