[core] Variable deregistry #12554
[core] Variable deregistry #12554
Conversation
| void Register() const { | ||
| const std::string variable_path("variables.all."+Name()); | ||
| if (!Registry::HasItem(variable_path)) { | ||
| Registry::AddItem<VariableType>(variable_path, *this); | ||
| Registry::AddItem<VariableType>("variables."+Registry::GetCurrentSource()+"."+Name(), *this); | ||
| } else if(!Registry::GetItem(variable_path).IsSameType(*this)) { | ||
| KRATOS_ERROR << "Attempting to register " << Name() | ||
| << " but a variable with the same name and different type already exists" | ||
| << std::endl; | ||
| } | ||
| } |
There was a problem hiding this comment.
I am curious, since registering the same variable from multiple apps causes some seg fault at the moment, shoudn't we throw an error if it exists, rather than checking the type?
There was a problem hiding this comment.
I think that Kratos should be able to support registering the same variable from multiple applications, as long as the type is the same. Using the registry in this way would be a step in that direction, since users of the registry are sure to always get the same thing when retrieving a given variable (something that is not so clear with components if Kratos allows overwriting variables as is happening now).
| void KratosApplication::DeregisterVariables() { | ||
| const std::string path = "variables."+mApplicationName; | ||
| if (Registry::HasItem(path)) { | ||
| auto& r_variables = Registry::GetItem(path); | ||
| // Iterate over items at path. For each item, remove it from the mappers.all branch too | ||
| for (auto i_key = r_variables.KeyConstBegin(); i_key != r_variables.KeyConstEnd(); ++i_key) { | ||
| Registry::RemoveItem("variables.all."+*i_key); | ||
| } | ||
| Registry::RemoveItem(path); | ||
| } | ||
| } |
There was a problem hiding this comment.
This removes the variables registered by the application from the all, hence we don't allow registering same variable from multiple apps. So, I would as mentioned in the earlier comment, throw an error if the same variable name is registered from multiple places.
There was a problem hiding this comment.
Please note that the scope of this PR is very narrow (it is intended only for testing, it is not a general "unload application" mechanism). What I am doing here is just ensuring that whoever registered the variable takes care of de-registering it, not ensuring that the kernel remains in a complete state after application de-registry. I understand that this is consistent with what the de-registration logic does for other types of objects.
|
Ok, it seems that this PR is not helping with the pipeline errors. We do need this capability it on the Altair side to avoid memory errors (since we have tests involving multiple applications and this becomes more of an issue), but we can work around it if it is not taken care of by the application de-registry automatically. I will keep the PR open for a while for discussion. |
📝 Description
Each application should deregister the variables it registered. They are not being removed from the registry currently, and I suspect this is causing memory errors on tests. (It definitely did for the tests at Altair involving multiple applications).
I'm creating this as a draft for now, just to see if it passes CI. If this ends up going in, I think we need an implementation of something like RegistryItem::IsSameType (added in this PR) that does not rely on a try/catch.
FYI @loumalouomega @pooyan-dadvand @roigcarlo
🆕 Changelog