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 all commits
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
54 changes: 42 additions & 12 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 @@ -696,43 +696,73 @@ catch(...) { Block KRATOS_THROW_ERROR(std::runtime_error, "Unknown error", MoreI
#ifdef KRATOS_REGISTER_GEOMETRY
#undef KRATOS_REGISTER_GEOMETRY
#endif
#define KRATOS_REGISTER_GEOMETRY(name, reference) \
KratosComponents<Geometry<Node>>::Add(name, reference); \
#define KRATOS_REGISTER_GEOMETRY(name, reference) \
KratosComponents<Geometry<Node>>::Add(name, reference); \
if(!Registry::HasItem("geometries."+Registry::GetCurrentSource()+"."+name) && \
!Registry::HasItem("components."+std::string(name))){ \
Registry::AddItem<RegistryItem>("geometries."+Registry::GetCurrentSource()+"."+name); \
Registry::AddItem<RegistryItem>("components."+std::string(name)); \
} \
Serializer::Register(name, reference);

#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) && \
!Registry::HasItem("components."+std::string(name))){ \
Registry::AddItem<RegistryItem>("elements."+Registry::GetCurrentSource()+"."+name); \
Registry::AddItem<RegistryItem>("components."+std::string(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) && \
sunethwarna marked this conversation as resolved.
Show resolved Hide resolved
!Registry::HasItem("components."+std::string(name))){ \
Registry::AddItem<RegistryItem>("conditions."+Registry::GetCurrentSource()+"."+name); \
Registry::AddItem<RegistryItem>("components."+std::string(name)); \
Copy link
Member

Choose a reason for hiding this comment

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

Is this addition to components temporary?

Copy link
Member Author

Choose a reason for hiding this comment

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

The component.[name] key is used to keep track of what is loaded regardless of who loaded it to avoid problems with multiple applications trying to load a components with the same name.

As discussed above with @pooyan-dadvand and @jcotela, the current behavior is that two applications can register the same component, as this is undefined and de-register time, I simply use this list to avoid having to make the check of

bool is_registered(): 
    for app in applications
        for comp_type in [elements, conditions, etc...]
            if component in app.comp_type then return true
return false

Which i need to prevent the component to appear as registered by the second app. Note this is consistent with what is current happening, but probably we need to rethink this situation at core level , as it may leave the kernel in an invalid state.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to have components.condition.LineLoadCondition2D2N rather than components.LineLoadCondition2D2N. Is there a reasoning behind having a flat hierarchy?

} \
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::HasItem("components."+std::string(name))){ \
Registry::AddItem<RegistryItem>("constraints."+Registry::GetCurrentSource()+"."+name); \
Registry::AddItem<RegistryItem>("components."+std::string(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::HasItem("components."+std::string(name))){ \
Registry::AddItem<RegistryItem>("modelers."+Registry::GetCurrentSource()+"."+name); \
Registry::AddItem<RegistryItem>("components."+std::string(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::HasItem("components."+std::string(name))){ \
Registry::AddItem<RegistryItem>("constitutive_laws."+Registry::GetCurrentSource()+"."+name); \
Registry::AddItem<RegistryItem>("components."+std::string(name)); \
} \
Comment on lines +759 to +765
Copy link
Member

Choose a reason for hiding this comment

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

Small question bcause I forgot, should we update macros for Flags and Variables also to do the same? Or did we agree on doing it differently?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently no, because variable and flags are not stored as components. Eventually would be interesting to move away from components and variable list and unify everything in the register, but right now is just to keep track of what is registered in the different KratosComponent<T> containetrs

Serializer::Register(name, reference);

#define KRATOS_DEPRECATED [[deprecated]]
Expand Down
29 changes: 28 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();
sunethwarna marked this conversation as resolved.
Show resolved Hide resolved
}

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

void RegisterKratosCore();

template<class TComponentsContainer>
void DeregisterComponent(std::string const & rComponentName);

/**
* @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
12 changes: 12 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
19 changes: 18 additions & 1 deletion kratos/includes/registry_item.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,12 @@
#include <string>
#include <iostream>
#include <unordered_map>
#include <any>

#if defined (__GNUC__) && __GNUC__ <= 8 && __GNUC_MINOR__ <= 3
#include <boost/any.hpp>
#else
#include <any>
#endif
Comment on lines +22 to +26
Copy link
Member Author

@roigcarlo roigcarlo May 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@sunethwarna @pooyan-dadvand Not the cleanest solution in the world, but apparently is a bug in gcc (my suspicion is that it has something to do with the type_id().name(), but gcc bugtrack is horrendous and cannot find the exact bug report).

This should be removed when we move to dev-toolset-10/11

Copy link
Member

Choose a reason for hiding this comment

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

Oh.... so the boost/any is superior than std::any.... It would be great if we can move to higher dev-toolset soon.... @pooyan-dadvand


// External includes

Expand Down Expand Up @@ -243,7 +248,11 @@ class KRATOS_API(KRATOS_CORE) RegistryItem
{
KRATOS_TRY

#if defined (__GNUC__) && __GNUC__ <= 8 && __GNUC_MINOR__ <= 3
return *(boost::any_cast<std::shared_ptr<TDataType>>(mpValue));
#else
return *(std::any_cast<std::shared_ptr<TDataType>>(mpValue));
#endif

KRATOS_CATCH("");
}
Expand All @@ -253,7 +262,11 @@ class KRATOS_API(KRATOS_CORE) RegistryItem
{
KRATOS_TRY

#if defined (__GNUC__) && __GNUC__ <= 8 && __GNUC_MINOR__ <= 3
return *std::dynamic_pointer_cast<TCastType>(boost::any_cast<std::shared_ptr<TDataType>>(mpValue));
#else
return *std::dynamic_pointer_cast<TCastType>(std::any_cast<std::shared_ptr<TDataType>>(mpValue));
#endif

KRATOS_CATCH("");
}
Expand Down Expand Up @@ -293,7 +306,11 @@ class KRATOS_API(KRATOS_CORE) RegistryItem
///@{

std::string mName;
#if defined (__GNUC__) && __GNUC__ <= 8 && __GNUC_MINOR__ <= 3
boost::any mpValue;
#else
std::any mpValue;
#endif
std::string (RegistryItem::*mGetValueStringMethod)() const;

///@}
Expand Down
67 changes: 66 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,60 @@ void KratosApplication::RegisterKratosCore() {
// Register ConstitutiveLaw BaseClass
KRATOS_REGISTER_CONSTITUTIVE_LAW("ConstitutiveLaw", mConstitutiveLaw);
}

template<class TComponentsContainer>
void KratosApplication::DeregisterComponent(std::string const & rComponentName) {
auto path = std::string(rComponentName)+"."+mApplicationName;

// Remove only if the application has this type of components registered
if (Registry::HasItem(path)) {

// Generate a temporal list with all the keys to avoid invalidating the iterator (Convert this into a transform range when C++20 is available)
std::vector<std::string> keys;
std::transform(Registry::GetItem(path).cbegin(), Registry::GetItem(path).cend(), std::back_inserter(keys), [](auto & key){return std::string(key.first);});

for (auto & key : keys) {
auto cmpt_key = "components."+key;
auto type_key = path+"."+key;

// Remove from KratosComponents
KratosComponents<TComponentsContainer>::Remove(key);

// Remove from registry general component list
if (Registry::HasItem(cmpt_key)) {
Registry::RemoveItem(cmpt_key);
Copy link
Member

Choose a reason for hiding this comment

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

This would be tricky if we are to use the same mechanism for variables, because, multiple applications can define the same variable. And deregistering one application should not remove the variable, if added from multiple apps.

} else {
KRATOS_ERROR << "Trying ro remove: " << cmpt_key << " which was not found in registry" << std::endl;
}

// Remove from registry component typed list
if (Registry::HasItem(type_key)) {
Registry::RemoveItem(type_key);
} else {
KRATOS_ERROR << "Trying ro remove: " << type_key << " which was not found in registry" << std::endl;
}
}

// Finally, remove the entry all together
Registry::RemoveItem(path);
}
}

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;

DeregisterComponent<Geometry<Node>>("geometries");
DeregisterComponent<Element>("elements");
DeregisterComponent<Condition>("conditions");
DeregisterComponent<MasterSlaveConstraint>("constraints");
DeregisterComponent<Modeler>("modelers");
DeregisterComponent<ConstitutiveLaw>("constitutive_laws");
}

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

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

void Registry::SetCurrentSource(std::string const & rCurrentSource)
{
// If context key not present, create it
if (Registry::HasItem("CurrentContext")){
Registry::RemoveItem("CurrentContext");
}

// It is needed to create a std::string explicitly copying the '"CurrentContext"+rCurrentSource' to avoid casting problems
// involing std::any_cast to a reference type which key references a string that may not be alive when invoked.
std::string context_key = std::string("CurrentContext." + rCurrentSource);

Registry::AddItem<RegistryItem>(context_key);
}

std::string Registry::GetCurrentSource()
{
// If context key not present, create it
if (!Registry::HasItem("CurrentContext")){
Registry::AddItem<RegistryItem>("CurrentContext.KratosMultiphysics");
}

return Registry::GetItem("CurrentContext").begin()->first;
}

std::size_t Registry::size()
{
return mspRootRegistryItem->size();
Expand Down
8 changes: 8 additions & 0 deletions kratos/sources/registry_item.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,21 @@ namespace Kratos
RegistryItem::SubRegistryItemType& RegistryItem::GetSubRegistryItemMap()
{
KRATOS_ERROR_IF(HasValue()) << "Item " << Name() << " has value and cannot be iterated." << std::endl;
#if defined (__GNUC__) && __GNUC__ <= 8 && __GNUC_MINOR__ <= 3
return *(boost::any_cast<SubRegistryItemPointerType>(mpValue));
#else
return *(std::any_cast<SubRegistryItemPointerType>(mpValue));
#endif
}

RegistryItem::SubRegistryItemType& RegistryItem::GetSubRegistryItemMap() const
{
KRATOS_ERROR_IF(HasValue()) << "Item " << Name() << " has value and cannot be iterated." << std::endl;
#if defined (__GNUC__) && __GNUC__ <= 8 && __GNUC_MINOR__ <= 3
return *(boost::any_cast<SubRegistryItemPointerType>(mpValue));
#else
return *(std::any_cast<SubRegistryItemPointerType>(mpValue));
#endif
}

RegistryItem::SubRegistryItemType::iterator RegistryItem::begin()
Expand Down
Loading