[Core] Making aplications deregistrable V3 #12306

roigcarlo · 2024-04-23T15:58:24Z

📝 Description
Third version of attempting to make applications deregistrable.

Applied the comments of @philbucher @pooyan-dadvand and @jcotela (and @loumalouomega ❤️ )

OVERVIEW

Registry now has a Setter/Getter to know who is using the register macros
Register macros add things to the registry
At application we check if the registry key for the different components exist for the given app, if not is created
At destruction we iterate over all the keys under the current application and delete it from the components.

Currently only the name of the component is stored, subject to change to a reference in the future, but I tried to keep the PR super small.

TODO

Add preconditioners
Consistenly renaming all Unregister to Deregister
Cleanup Debug Strings

Changes for the IGA and name consistency will come in separate PR if you like this one.

loumalouomega · 2024-04-23T16:47:14Z

kratos/sources/kratos_application.cpp

-      mpRegisteredObjectsName(&(Serializer::GetRegisteredObjectsName())) {}
+      mpRegisteredObjectsName(&(Serializer::GetRegisteredObjectsName())) {
+
+        Registry::SetCurrentSource(mApplicationName);


It is me or the spacing is inconsistent?

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

kratos/sources/kratos_application.cpp

loumalouomega

Some comments

avdg81

Just a few questions and comments from my side. Hope you find them useful.

kratos/includes/define.h

avdg81 · 2024-04-25T15:44:18Z

kratos/includes/kratos_application.h

+     * @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:


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:

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

avdg81 · 2024-04-25T15:50:38Z

kratos/sources/registry.cpp

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


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?

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.

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

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

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.

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

roigcarlo · 2024-04-29T08:47:05Z

Ping @pooyan-dadvand

pooyan-dadvand

Third iteration seems not converged but the convergence rate is quadratic! ;-)

pooyan-dadvand · 2024-05-06T20:40:54Z

kratos/includes/define.h

@@ -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)){                                              \


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

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

We are in the same page!

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.

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.

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?

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.

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.

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 👍

kratos/includes/define.h

pooyan-dadvand · 2024-05-06T20:46:01Z

kratos/sources/kratos_application.cpp

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


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.

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

You are right! Still we need that for components

pooyan-dadvand · 2024-05-06T20:49:22Z

kratos/sources/registry.cpp

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


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

pooyan-dadvand · 2024-05-06T21:30:27Z

kratos/sources/registry.cpp

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


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

kratos/sources/registry.cpp

roigcarlo · 2024-05-10T14:42:09Z

@sunethwarna Please help 🙏 . Seems that there is a problem with the registry in centos while adding a std::string value to a RegistryItem

I've narrowed down to:

    std::string Registry::GetCurrentSource()
    {
        using namespace std::literals;

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

        auto & entry = Registry::GetItem("CurrentContext");

        // If no context set, set the default one
        if (!entry.HasItem("value")) {
            entry.AddItem<std::string>("value", std::string("KratosMultiphysics"));
        }

        return Registry::GetItem("CurrentContext.value").GetValue<std::string>();
    }

Apparently if change it to:

if (!entry.HasItem("value")) {
    entry.RemoveItem("value");
}

entry.AddItem<std::string>("value", std::string("KratosMultiphysics"));
return Registry::GetItem("CurrentContext.value").GetValue<std::string>();

It works, but does not matter what is the previous value, if i leave it unchanged its always fail. Interestingly, if I punt debug prints:

GettingCurrentSource
I have a value
Value obtained KratosMultiphysics
Process Id: 18441
Importing    KratosStructuralMechanicsApplication
    KRATOS   ___|  |                   |                   |
           \___ \  __|  __| |   |  __| __| |   |  __| _` | |
                 | |   |    |   | (    |   |   | |   (   | |
           _____/ \__|_|   \__,_|\___|\__|\__,_|_|  \__,_|_| MECHANICS
Initializing KratosStructuralMechanicsApplication...
GettingCurrentSource
I have a value
Value obtained StructuralMechanicsApplication
GettingCurrentSource
I have a value
Value obtained StructuralMechanicsApplication

It seems to happen when the application is changed, but at the 3rd or 4th time something calls that function.

roigcarlo · 2024-05-10T15:35:26Z

Apparently, the same thing happens if you try to register the string hashed, so its not a problem being a string.

sunethwarna · 2024-05-13T08:14:38Z

Hi @roigcarlo I am also checking this out now. Sorry for the late reply :/

roigcarlo · 2024-05-13T08:37:29Z

If it helps:

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

        auto & entry = Registry::GetItem("CurrentContext");

        if (entry.HasItem("value")) {
            entry.RemoveItem("value");
        }

        entry.AddItem<std::string>("value", rCurrentSource);

        if (entry.HasItem("value")) {
            entry.RemoveItem("value");
        }

        entry.AddItem<std::string>("value", rCurrentSource);

This also crashes :S

…n gcc 8.3 (centos)

…nce in the registry keyç

…cted)

roigcarlo · 2024-05-22T09:01:53Z

kratos/includes/registry_item.h

+#if defined (__GNUC__) && __GNUC__ <= 8 && __GNUC_MINOR__ <= 3
+    #include <boost/any.hpp>
+#else
+    #include <any>
+#endif 


@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

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

roigcarlo · 2024-05-24T07:50:06Z

@ping @KratosMultiphysics/technical-committee

sunethwarna

I have following questions:

Are we gonna change the registering macros for variables the same way? If so, deregistering an app should not remove the variables which are added from multiple apps.
Throwing an error if there is an component with the same name rather than skipping or ignoring [Except for may be Flags and Variable].

Thanks for the work @roigcarlo . Sorry could not be of much help.

sunethwarna · 2024-05-25T11:36:39Z

kratos/includes/define.h

+#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));                                                        \
+    }                                                                                                               \


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?

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

kratos/includes/kratos_application.h

sunethwarna · 2024-05-25T11:38:26Z

kratos/includes/registry.h

+    /** 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
+    */


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

sunethwarna · 2024-05-25T11:39:34Z

kratos/includes/registry_item.h

+#if defined (__GNUC__) && __GNUC__ <= 8 && __GNUC_MINOR__ <= 3
+    #include <boost/any.hpp>
+#else
+    #include <any>
+#endif 


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

sunethwarna · 2024-05-25T11:50:02Z

kratos/includes/define.h

+    if(!Registry::HasItem("conditions."+Registry::GetCurrentSource()+"."+name) &&                                   \
+       !Registry::HasItem("components."+std::string(name))){                                                                     \
+        Registry::AddItem<RegistryItem>("conditions."+Registry::GetCurrentSource()+"."+name);                       \
+        Registry::AddItem<RegistryItem>("components."+std::string(name));                                                        \


Is this addition to components temporary?

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.

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

kratos/includes/define.h

sunethwarna · 2024-05-25T11:51:52Z

kratos/sources/kratos_application.cpp

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


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.

roigcarlo · 2024-06-04T12:40:11Z

@pooyan-dadvand Ping

sunethwarna

Minor comments

sunethwarna · 2024-06-05T13:47:40Z

kratos/includes/define.h

+    if(!Registry::HasItem("conditions."+Registry::GetCurrentSource()+"."+name) &&                                   \
+       !Registry::HasItem("components."+std::string(name))){                                                                     \
+        Registry::AddItem<RegistryItem>("conditions."+Registry::GetCurrentSource()+"."+name);                       \
+        Registry::AddItem<RegistryItem>("components."+std::string(name));                                                        \


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

sunethwarna · 2024-06-05T13:53:55Z

kratos/sources/kratos_application.cpp

+        for (auto component : {"geometries", "elements", "conditions", "constraints", "modelers", "constitutive_laws"}) {
+            if (!Registry::HasItem(std::string(component))) {
+                Registry::AddItem<RegistryItem>(std::string(component)+"."+mApplicationName);
+            }
+        }
+      }


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?

sunethwarna

Thanks @roigcarlo

Alternative implementation

4d4ed88

loumalouomega reviewed Apr 23, 2024

View reviewed changes

kratos/sources/kratos_application.cpp Outdated Show resolved Hide resolved

loumalouomega reviewed Apr 23, 2024

View reviewed changes

roigcarlo mentioned this pull request Apr 24, 2024

[Core] Making kratos applications unloadable #12281

Closed

avdg81 reviewed Apr 25, 2024

View reviewed changes

roigcarlo mentioned this pull request Apr 26, 2024

GTest #11506

Open

6 tasks

Missing geometries

fac137a

pooyan-dadvand reviewed May 6, 2024

View reviewed changes

Change proposal based on comments

a9a29d3

roigcarlo commented May 8, 2024

View reviewed changes

kratos/sources/registry.cpp Outdated Show resolved Hide resolved

roigcarlo added 6 commits May 9, 2024 16:39

Fixed loop over invalidated iterators

9694b6a

Removing mCurrentSource

ecd6e0b

Previnting the de-register of non existing keys

c609e2c

fixing possible auto deduction failure in lambda

117b3bf

Missing def

8d77e96

Trying more simple approach

21aae0f

roigcarlo added 3 commits May 15, 2024 17:19

Implementing current context as a key extension to avoid regression i…

02df3ac

…n gcc 8.3 (centos)

typo

b637f0d

Prevent std::bad_any cast due to string_view to invalid string refere…

0fc4296

…nce in the registry keyç

roigcarlo marked this pull request as ready for review May 21, 2024 12:09

roigcarlo requested a review from a team as a code owner May 21, 2024 12:09

roigcarlo added 3 commits May 21, 2024 14:10

Disable deregister until tests (Failures in previous commit were expe…

79f8840

…cted)

Missing .

aaaacdb

Probably a bug in gcc

6b7fbe9

Missing

701be9b

roigcarlo commented May 22, 2024

View reviewed changes

sunethwarna reviewed May 25, 2024

View reviewed changes

sunethwarna reviewed Jun 5, 2024

View reviewed changes

sunethwarna approved these changes Jun 5, 2024

View reviewed changes

roigcarlo merged commit 43fd714 into master Jun 5, 2024
11 checks passed

roigcarlo deleted the core/unregister_registry branch June 5, 2024 14:14

roigcarlo mentioned this pull request Jun 7, 2024

Revert "[Core] Making aplications deregistrable V3" #12437

Closed

roigcarlo restored the core/unregister_registry branch June 7, 2024 14:18

[Core] Making aplications deregistrable V3 #12306

[Core] Making aplications deregistrable V3 #12306

Conversation

roigcarlo commented Apr 23, 2024 • edited

OVERVIEW

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loumalouomega left a comment

Choose a reason for hiding this comment

avdg81 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roigcarlo May 7, 2024 • edited

Choose a reason for hiding this comment

roigcarlo commented Apr 29, 2024

pooyan-dadvand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roigcarlo May 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roigcarlo commented May 10, 2024 • edited

roigcarlo commented May 10, 2024

sunethwarna commented May 13, 2024

roigcarlo commented May 13, 2024

roigcarlo May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roigcarlo commented May 24, 2024

sunethwarna left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roigcarlo commented Jun 4, 2024

sunethwarna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunethwarna left a comment

Choose a reason for hiding this comment

roigcarlo commented Apr 23, 2024 •

edited

roigcarlo May 7, 2024 •

edited

roigcarlo May 7, 2024 •

edited

roigcarlo commented May 10, 2024 •

edited

roigcarlo May 22, 2024 •

edited

sunethwarna left a comment •

edited