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.

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

Organization of Kratos Core and Application Tiers #12136

Open
roigcarlo opened this issue Mar 4, 2024 · 23 comments
Open

Organization of Kratos Core and Application Tiers #12136

roigcarlo opened this issue Mar 4, 2024 · 23 comments

Comments

@roigcarlo
Copy link
Member

roigcarlo commented Mar 4, 2024

Dear @KratosMultiphysics/all

As you may have noticed, Kratos has rapidly evolved this past years and has been adopted by many new developers. While this is good, it has also deepened some of the chronicle problems of Kratos:

  • PR take long time to merge, specially the ones changing core Features
  • Strict Deadlines are now in play, specially from industry partners.
  • Technical and Implementation committee are simply not big enough to review all Core+ related PR

From the technical committee we have been monitoring these issue for quite some time and trying to come up with solutions. After long discussing, we would like to propose a roadmap of changes intended to solve this problems. Of course the intention of this issue is to discuss what you think about the changes and make adjustments if you deem necessary:

Creation of Kratos::Future and Kratos::Legacy

The first and probably most important change we propose is the creation of two namespaces in the Kratos core (yes, we will finally have namespaces):

These namespaces: Future & Legacy will be the place to put all the changes affecting the core that need a relatively long developing time, which are not final, or that are simply needed in a particular moment in time and are not finished or have ongoing issues/discussions associated with them.

In any case these namespaces will not be a sandbox to include arbitrary features. Its just a previous place to indicate that some feature or change that has been already approved will be merged in a close future but is not yet ready for general use or will be removed soon.

We will explicit prohibit the usage of using namespace Kratos::Future / using namespace Kratos::Legacy. Future and Legacy features will need to be explicitly called in the sections of the code where are used.

In the case a Kratos::Future affects a critical part of the core, or introduces API breaking changes, we will maintain a older version of such code in Kratos::Legacy for some time, until the changes are made definitive. Think of this as an evolved KRATOS_DEPRECATED macro.

Overall things will look like this (example of a change in a utility):

Original Code

void myFunction () {
    auto my_utility = MyCoreUtility();
}

void otherPersonFunction () {
    auto my_utility = MyCoreUtility();
}

Someone updates MyCoreUtility to work with model and parameters, needs it integrated but still has issues or breaks the current api, code is included in Kratos::Future:

void myFunction () {
    auto my_utility = Kratos::Future::MyCoreUtility(rModel, rParameters);
}

void otherPersonFunction () {
    auto my_utility = MyCoreUtility();
}

After some time, implementation is final, new code moves out of Kratos::Future old code gets moved to Kratos::Legacy

void myFunction () {
    auto my_utility = MyCoreUtility(rModel, rParameters);
}

void otherPersonFunction () {
    auto my_utility = Kratos::Legacy::MyCoreUtility(),  # This call will issue a deprecation warning
}

After some time, Kratos::Legacy implementation gets removed.

void myFunction () {
    auto my_utility = MyCoreUtility(rModel, rParameters);
}

void otherPersonFunction () {
    auto my_utility = MyCoreUtility(rModel, rParameters);
}

Removal of code owners from certain parts of the core

As mentioned in the introduction, the current developers from tech and implementation committee are just not enough to review all the changes that need to be made into the core. In order to address this issue we plan to restrain the areas in which a explicit approval from the TC is necessary to merge. A lot of you have suggested that parts of the core should not have that level of control, we agree and our initial proposal is to exclude kratos\utilities and kratos\processes to see how things evolve, and extend this to other non critical parts of the core.

This should help to alleviate the number of stalled PR in the core. We are very interested in hear which parts of the core you think are not critical and could be included in this exclusion list.

Application Tiers

This is not new, and has previously been discussed
but never really implemented. Right now there is simply too much applications in Kratos, with very different levels of maturity.

While this is good, because it means that many people are developing, it es very confusing from a newcomer to know what they can try or trust.

Our proposal in this regard is to physically split (put them in different folder) applications in different tiers. Each of the list will be decision of the TC.

  • Applications (Core Applications): Applications ready for final users.
  • Utility Applications: applications adding utilities or features (Trilinos, Mapping, LinearSolvers, etc...).
  • Research Application: Everything which does not fit the categories above.

The initial proposal from the Tc is:
Core:

  • FluidDynamicsApplication
  • StructuralMechanicsApplication
  • GeoMechanicsApplication
  • CoSimulationApplication
  • ConstitutiveLawsApplication

Utilities:

  • HDF5Application
  • LinearSolversApplication
  • MedApplication
  • MeshingApplication
  • MeshMovingApplication
  • MappingApplication
  • MetisApplication
  • TrilinosApplication

The intention with this change is to provide as smaller subset of Kratos which is not threatening to new users. This will also allow for some technical changes to mitigate the disk usage stress on the CI by layers the compilation process.

@roigcarlo roigcarlo pinned this issue Mar 4, 2024
@AlejandroCornejo
Copy link
Member

The ConvectionDiffusionApp should be CORE right?. One question, the testing level and trustability of Core apps and utilities is the same?

@loumalouomega
Copy link
Member

The ConvectionDiffusionApp should be CORE right?. One question, the testing level and trustability of Core apps and utilities is the same?

And what about ContactStructuralMechanicsApplication?, I think should be Utility.

@loumalouomega
Copy link
Member

EigenSolversApplication is legacy, and it was replaced with LinerSolverApplication

@AlejandroCornejo
Copy link
Member

The ConvectionDiffusionApp should be CORE right?. One question, the testing level and trustability of Core apps and utilities is the same?

And what about ContactStructuralMechanicsApplication?, I think should be Utility.

Indeed

@AlejandroCornejo
Copy link
Member

I have also doubts in classifying the DEMApplication

@pooyan-dadvand
Copy link
Member

The ConvectionDiffusionApp should be CORE right?. One question, the testing level and trustability of Core apps and utilities is the same?

@KratosMultiphysics/technical-committee consider this as a core and transversal feature which should go into the core applications. Nevertheless, it needs some cleanup and documentation before be added.

@pooyan-dadvand
Copy link
Member

I have also doubts in classifying the DEMApplication

@KratosMultiphysics/technical-committee understands the importance and uniqueness of the @KratosMultiphysics/dem . However, we have concerns about its compatibility with the rest of the applications and in the format of the ProjectParamter.json. So, we would suggest to improve the compatibility and then we will consider its addition as a core application, given the extensive tests.

Also, we have concerns about the level of the maintenance and how fast they fix the errors and fixing backward incompatibilities.

Of course, addressing those concerns we are happy to add it to the core.

@pooyan-dadvand
Copy link
Member

ContactStructuralMechanicsApplication

@KratosMultiphysics/technical-committee consider it to be in a very good state. Nevertheless, our impression is that it is not ready yet for generic usage and not mature enough to be used without deep knowledge of it. Adding that it is not supported by the user interfaces makes its usage more difficult and more on the research side.

The status can hopefully change in the future when it gets more usage and being more battle tested.

@loumalouomega
Copy link
Member

ContactStructuralMechanicsApplication

@KratosMultiphysics/technical-committee consider it to be in a very good state. Nevertheless, our impression is that it is not ready yet for generic usage and not mature enough to be used without deep knowledge of it. Adding that it is not supported by the user interfaces makes its usage more difficult and more on the research side.

The status can hopefully change in the future when it gets more usage and being more battle tested.

GUI is supported by GiD

@AlejandroCornejo
Copy link
Member

ContactStructuralMechanicsApplication

@KratosMultiphysics/technical-committee consider it to be in a very good state. Nevertheless, our impression is that it is not ready yet for generic usage and not mature enough to be used without deep knowledge of it. Adding that it is not supported by the user interfaces makes its usage more difficult and more on the research side.
The status can hopefully change in the future when it gets more usage and being more battle tested.

GUI is supported by GiD

It is indeed, I use the GiD GUI a lot in contact.

@rubenzorrilla
Copy link
Member

ContactStructuralMechanicsApplication

@KratosMultiphysics/technical-committee consider it to be in a very good state. Nevertheless, our impression is that it is not ready yet for generic usage and not mature enough to be used without deep knowledge of it. Adding that it is not supported by the user interfaces makes its usage more difficult and more on the research side.
The status can hopefully change in the future when it gets more usage and being more battle tested.

GUI is supported by GiD

It is indeed, I use the GiD GUI a lot in contact.

In this regard we also noted that GiD is not open-source, and not quite popular in other research groups/companies other than CIMNE.

@KratosMultiphysics KratosMultiphysics deleted a comment from eonate Mar 12, 2024
@loumalouomega
Copy link
Member

ContactStructuralMechanicsApplication

@KratosMultiphysics/technical-committee consider it to be in a very good state. Nevertheless, our impression is that it is not ready yet for generic usage and not mature enough to be used without deep knowledge of it. Adding that it is not supported by the user interfaces makes its usage more difficult and more on the research side.
The status can hopefully change in the future when it gets more usage and being more battle tested.

GUI is supported by GiD

It is indeed, I use the GiD GUI a lot in contact.

In this regard we also noted that GiD is not open-source, and not quite popular in other research groups/companies other than CIMNE.

I can buy the argument that my application is "not matured enough", because that could be technically true. But the open source of the GUI is quite non-consistent argument, as to my knowledge the open source GUI support is quite limited.

@RiccardoRossi
Copy link
Member

@loumalouomega if we ask for your honest feedback, do you think that the ContactMechanicsApplication is at the same level of stability/maturity than the other applications?
I am asking for your open feedback. My personal concern, even more than the GUI (i think it is not trivial to have WORKING examples without some inner knowledge) is that if we decide for making it "core" we will need to guarantee backward compatibility, ensure its usability from the GUIs, be more conservatives in future changes, etc. That is, if we decide to go that way it will need resources/effort to be poured into it

@loumalouomega
Copy link
Member

@loumalouomega if we ask for your honest feedback, do you think that the ContactMechanicsApplication is at the same level of stability/maturity than the other applications? I am asking for your open feedback. My personal concern, even more than the GUI (i think it is not trivial to have WORKING examples without some inner knowledge) is that if we decide for making it "core" we will need to guarantee backward compatibility, ensure its usability from the GUIs, be more conservatives in future changes, etc. That is, if we decide to go that way it will need resources/effort to be poured into it

I was not asking to be core app as it is of course not matured enough. I was asking to be utility. I understand that the application includes physics not only utilities, but it is accesorial to the StructuralMechanicsApplication.

I can understand that the GUI is maybe not robust enough, my comment was to say that there is GUI support, and I commented that I didn't buy that the GUI was not open source, becasue applying that argument almost nothing has opensourced GUI support.

@rubenzorrilla
Copy link
Member

rubenzorrilla commented Mar 12, 2024

@loumalouomega, let me clarify something based on your last response.

Utility applications necessarily need to not include any physics. The ContactStructuralMechanicsApplication does so, meaning that it cannot be a utility application. Hence, it can only be either core application or research.

@KratosMultiphysics/technical-committee maybe we need to further clarify this.

@loumalouomega
Copy link
Member

@loumalouomega, let me clarify something based on your last response.

Utility applications necessarily need to not include any physics. The ContactStructuralMechanicsApplication does so, meaning that it cannot be a utility application. Hence, it can only be either core application or research.

@KratosMultiphysics/technical-committee maybe we need to further clarify this.

In that case I can understand the decission, but maybe some intermediate category is necessary. For example the ConstitutiveLawsApplication (@AlejandroCornejo) for most laws is quite stable, but there are some research developments too. And at the same time is a "sub app" like ContactStructuralMechanicsApplication which is nothing without StructuralMechanicsApplication. A priori ConstitutiveLawsApplication is more generic and can be used in more apps, but does "nothing" without those apps.

@matekelemen
Copy link
Contributor

matekelemen commented Mar 16, 2024

@roigcarlo is there any way to set a timeout for how long a feature can be stuck in the Future namespace (as well as in a deprecated state)? I wouldn't want to end up in a fractured mess that noone would take care of updating.

What I'd like to see is something like a date, after which a given function/class/whatever would fail to compile. Then someone would be forced to take a look and decide whether to move it from Future to the standard namespace, or prolong the timer.

Example:

namespace Future {

KRATOS_TIMED_DEPRECATION(1, 4, 2024) // <== fails to compile after the 1st of April, 2024
void NewStuff() {}

} // namespace Future

P.S.: this feature would obviously be disabled by default, and only enabled in the CI.

@roigcarlo
Copy link
Member Author

Yes I can add that check at compile time, no problem

@loumalouomega
Copy link
Member

@roigcarlo is there any way to set a timeout for how long a feature can be stuck in the Future namespace (as well as in a deprecated state)? I wouldn't want to end up in a fractured mess that noone would take care of updating.

What I'd like to see is something like a date, after which a given function/class/whatever would fail to compile. Then someone would be forced to take a look and decide whether to move it from Future to the standard namespace, or prolong the timer.

Example:

namespace Future {

KRATOS_TIMED_DEPRECATION(1, 4, 2024) // <== fails to compile after the 1st of April, 2024
void NewStuff() {}

} // namespace Future

P.S.: this feature would obviously be disabled by default, and only enabled in the CI.

Just to mention I tried to add that long time ago and didn't worked, but maybe now with better compilers will work

@roigcarlo
Copy link
Member Author

Yep I remember, we were using the wrong approach. We can obtain the epoch from the system through cmake and let the macros handle the conversions to use time.

@loumalouomega
Copy link
Member

Yep I remember, we were using the wrong approach. We can obtain the epoch from the system through cmake and let the macros handle the conversions to use time.

@roigcarlo
Copy link
Member Author

@loumalouomega btw I merged #11790 which has a list of applications. That's not final but I needed a name and an initial, what we decide here will reflect in that list (don't kill me pls 🙏 )

@eonate

This comment was marked as off-topic.

@Igarizza Igarizza unpinned this issue Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants