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 2 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
38 changes: 28 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 @@ -698,41 +698,59 @@ catch(...) { Block KRATOS_THROW_ERROR(std::runtime_error, "Unknown error", MoreI
#endif
#define KRATOS_REGISTER_GEOMETRY(name, reference) \
KratosComponents<Geometry<Node>>::Add(name, reference); \
if(!Registry::HasItem("geometries."+Registry::GetCurrentSource()+"."+name)){ \
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 if this is a correct behavior. We don't want to have different geometries, elements and conditions with the same names in different applications.

And we don't want to deregister an entity if is created by someone else before us.

So I would check if the name exists in that typeof branch and Add an item to the [Registry::GetCurrentSource()].deregister_list.name

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.

Agree, but this is currently how components are added ( as a matter of fact, you can have several components from the same app or different applications with the same name as long as the types are different ):

    static void Add(const std::string& rName, const TComponentType& rComponent)
    {
        // Check if a different object was already registered with this name, since this is undefined behavior
        auto it_comp =  msComponents.find(rName);
        KRATOS_ERROR_IF(it_comp != msComponents.end() && typeid(*(it_comp->second)) != typeid(rComponent)) << "An object of different type was already registered with name \"" << rName << "\"!" << std::endl;
        msComponents.insert(ValueType(rName , &rComponent));
    }

Nevertheless it is true that this could cause a registered component to be unloaded by an incorrect app. Just to confirm that we want to avoid this case:

App1 Loads A
App2 Loads A
App2 Unloads A <-- Problem.

I will change it to keep track of this case

Copy link
Member

Choose a reason for hiding this comment

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

We are in the same page!

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I am misunderstanding something, but

App1 Loads A
App2 Loads A
App1 Unloads A <-- Problem, too (if you intend to keep using App2)

I think that what you need here is something that behaves like a counted pointer, not keeping track of who registered what.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because the implementation of KratosComponents is misleading.

If two applications want to add a component with the same name, provided that all checks pass, this will be executed:

msComponents.insert(ValueType(rName , &rComponent));

Which will insert the component if not present, but will do nothing the second time (msComponents is a map and insert only does something if the key is not existent). That implies that the moment we unload the application that has added the component, the component becomes invalid: 1 - because the reference is no longer correct, 2 - because the second app did nothing.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, but in this situation the second app is left in an invalid state. Essentially, once the first app is unloaded, anything else that shared a name with it is left in an incomplete state and cannot be trusted to work.

If this is the case, wouldn't it be simpler to just have an "unload everything" (or maybe "unload all applications, leaving core untouched") mechanism instead of keeping track of individual apps?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the second app was never in a valid state, it was referencing something it does not own (our eternal discussion if a variable can be registered by two apps). I think we don't have any example in which only a single application is unloaded.

The implementation we wanted to replace (can be found isolated in #11511) actually does what you suggest (the kernel is in charge of clearing all the components) but @RiccardoRossi commented it had a very important flaw: It cannot deal with custom types. In fact #11506 would fail if we try to register a preconditioned for example, as it is a component unknown by the kernel.

or maybe "unload all applications, leaving core untouched"

This cannot be done (and basically is the root of why I want to use tests to change the current behavior) because creating an instance of the kernel automatically tries to register the "KratosMultiphysics" app, and while the kernel class has a lifetime with initialization and destruction, the components are static and do not. Consider this code:

void do_something() {
    auto kernel_instance = std::shared_ptr<Kratos::kernel>())
}

for (int i = 0; i < 2; i++) {
     do_something();
}

At i=0 the first instance of the kernel is created, the we call:

if (!IsImported("KratosMultiphysics")) {
    this->ImportApplication(mpKratosCoreApplication);
}

At i=1 If code is left untouched (no deregisters, etc...) this will work, because even if the kernel was destroyed, the list applications loaded and the list of components was never cleared, which implies that applications, and components are "eternal", once registered there is no way back. In other worlds, if the kernel was a singleton it could never be destroyed even if you wanted to.

Now consider now what happens with the new tests (for example, but a custom workflow from the GeoMechanics or anything that does not use Python as an entry point has the same problem)

The base test code defines the life cycle that will be executed per every test:

class KRATOS_API(KRATOS_TEST_UTILS) KratosCoreFastSuite : public ::testing::Test 
{
    public:
        void SetUp() override;
        void TearDown() override;

    protected:
        KratosCoreFastSuite(): mKernel() {  // we are initializing the kernel here for every test
        }

        ~KratosCoreFastSuite() {}
        }

    private:
        Kratos::Kernel mKernel;
};

Now, for every app I will be able to derive this using:

// Header
class KratosStructuralMechanicsIntegrationSuite : public KratosCoreFastSuite
{
public:
    KratosStructuralMechanicsIntegrationSuite();

private:
    KratosStructuralMechanicsApplication::Pointer mpStructuralApp;
    KratosLinearSolversApplication::Pointer mpLinearSolversApp;
};

// Source 
KratosStructuralMechanicsIntegrationSuite::KratosStructuralMechanicsIntegrationSuite() : KratosCoreFastSuite()
{
    mpStructuralApp = std::make_shared<KratosStructuralMechanicsApplication>();
    this->ImportApplicationIntoKernel(mpStructuralApp);
    mpLinearSolversApp = std::make_shared<KratosLinearSolversApplication>();
    this->ImportApplicationIntoKernel(mpLinearSolversApp);
}

Of course, I can
1 - Check if the application is already loaded and do nothing: Works without changing the current code. We use the same kernel across all tests, which is deeply wrong
2 - I unloaded the applications manually keeping the kernel: I have no way to instruct the applications to de-register custom component types and I don't know what was loaded by the kernel or by the apps
3 - I unloaded everything throw the kernel: Works, but as 2 I have no way to de-register custom components.
4 - Everyone keeps track of who is an what has registered and is in charge or de-register: No problems.

Honestly, any solution can work with we assume the consequences but for me 4 is by far the good solution.

That being said, the fact that two applications can register the same thing and the second does nothing without a warning is super dangerous and should be changed, but probably in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, understood. I just think that we need to be clear that this solves the testing use-case by delegating the cleaning-up of the components to the applications, but it does not make applications unloadable in any meaningful way. Or, in other words, I do not think this solution would work for any use case that does not use python as an entrypoint, but I agree that it works for this one.

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 just think that we need to be clear that this solves the testing use-case by delegating the cleaning-up of the components to the applications, but it does not make applications unloadable in any meaningful way

Completely agree 👍

Registry::AddItem<RegistryItem>("geometries."+Registry::GetCurrentSource()+"."+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)){ \
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