Skip to content

Revamp "builder_and_solver_settings" for StructuralMechanicsAnalysis#13272

Merged
matekelemen merged 4 commits intomasterfrom
structural/builder-and-solver-settings
May 24, 2025
Merged

Revamp "builder_and_solver_settings" for StructuralMechanicsAnalysis#13272
matekelemen merged 4 commits intomasterfrom
structural/builder-and-solver-settings

Conversation

@matekelemen
Copy link
Contributor

@matekelemen matekelemen commented Mar 26, 2025

Changes

Choosing and configuring a BuilderAndSolver in a solver is weird and inconsistent with other types of settings such as configuring a SolvingStrategy.

This PR refactors what StructuralMechanicsSolver expects in "builder_and_solver_settings"

...
"builder_and_solver_settings" : {
+   "type" : "block",
-   "use_block_builder" : true,
-   "use_lagrange_BS" : false,
    "advanced_settings" : {}
}
...

Now "type" is the only parameter that decides which BuilderAndSolver to use. Current options are

  • "block" refers to ResidualbasedBlockBuilderAndSolver
  • "block_lagrange" refers to ResidualbasedBlockBuilderAndSolverWithLagrangeMultiplier
  • "elimination" refers to either ResidualBasedEliminationBuilderAndSolver or ResidualBasedEliminationBuilderAndSolverWithConstraints, depending on whether the system has constraints

This makes it much easier and cleaner to integrate more BuilderAndSolver types. The current system is still supported, but will issue a deprecation warning.

Notes

The "block_builder" flag in the solver's settings has been deprecated for 5 years. I tried removing it but saw that most downstream apps, as well as test cases still heavily use this setting, and even add new code using it. Please don't ignore deprecation warnings.

A quick grep shows the following applications using this deprecated setting:

> rg "\"block_builder\"" -l | rg "applications/(\w*)/.*" -or '$1' | sort | uniq
ConvectionDiffusionApplication
CSharpWrapperApplication
DamApplication
FemToDemApplication
FluidTransportApplication
GeoMechanicsApplication
IgaApplication
MPMApplication
PfemFluidDynamicsApplication
PoromechanicsApplication
SolidMechanicsApplication
StructuralMechanicsApplication
TrilinosApplication

@KratosMultiphysics/dem @KratosMultiphysics/geomechanics @KratosMultiphysics/nurbs-breps @KratosMultiphysics/mpm @KratosMultiphysics/pfem @KratosMultiphysics/poromechanics @trilinos

@loumalouomega
Copy link
Member

I would agree if the @KratosMultiphysics/technical-committee agrees, specially if @KratosMultiphysics/fluid-dynamics in order to be consistent

@rubenzorrilla
Copy link
Member

Honestly I'd avoid this considering that we're getting rid of the B&S soon. Instead, I'd discuss the I/O for the new building settings.

@matekelemen
Copy link
Contributor Author

Without this PR, however I try to add my B&S from #13276 will be a massive hack. I'd prefer transitioning to the new strategies from a cleaner state rather than an ugly one.

Copy link
Member

@RiccardoRossi RiccardoRossi left a comment

Choose a reason for hiding this comment

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

my problem with this is that it destroys the backward compatibility.

we need to have a mechanism that if someone has a old file using the old mechanism it still works

on the line of:

if(parameters.Has(block_builder_and_solver) : 
    throw watning 
    type = block
else
    ...

@matekelemen
Copy link
Contributor Author

we need to have a mechanism that if someone has a old file using the old mechanism it still works

@RiccardoRossi

def __ConvertDeprecatedSettings(self, settings: KratosMultiphysics.Parameters) -> None:

JSONs (ProjectParameters.json) can use the old mechanism. I did exactly what has been done with other deprecated settings: support them but throw a deprecation warning.

@matekelemen
Copy link
Contributor Author

matekelemen commented Mar 27, 2025

By the way, our approach to deprecation is completely broken.

The deprecated settings you see in StructuralMechanicsSolver are at least 5 years old, and they are impossible to remove because many applications (even the structural mechanics application itself) still use them, so the CI wouldn't pass. At this rate, we won't ever be able to get rid of deprecated features/settings.

Relying on the due diligence of downstream developers to update their code is clearly not working if there is no threat of their code eventually breaking one day.

@loumalouomega
Copy link
Member

By the way, our approach to deprecation is completely broken.

The deprecated settings you see in StructuralMechanicsSolver are at least 5 years old, and they are impossible to remove because many applications (even the structural mechanics application itself) still use them, so the CI wouldn't pass. At this rate, we won't ever be able to get rid of deprecated features/settings.

Relying on the due diligence of downstream developers to update their code is clearly not working if there is no threat that their code will break one day.

The other day I removed the mesh_id of the processes. I understand the concerns. Maybe we need something to properly manage consistently the deprecation, I will think about it.

@loumalouomega
Copy link
Member

By the way, our approach to deprecation is completely broken.
The deprecated settings you see in StructuralMechanicsSolver are at least 5 years old, and they are impossible to remove because many applications (even the structural mechanics application itself) still use them, so the CI wouldn't pass. At this rate, we won't ever be able to get rid of deprecated features/settings.
Relying on the due diligence of downstream developers to update their code is clearly not working if there is no threat that their code will break one day.

The other day I removed the mesh_id of the processes. I understand the concerns. Maybe we need something to properly manage consistently the deprecation, I will think about it.

For example adding special tags in the parameters and a method to specifically validate parameters including deprecated ones, I need to think about it.

@philbucher
Copy link
Member

Having dealt with this particular problem a frustratingly high number of times, my recommendation is to go ahead with this PR and continue to adapt it for future developments when the need arises. Otherwise, as @matekelemen states, this is a big blocker for progress/developments

@matekelemen matekelemen requested a review from a team March 27, 2025 15:59
@matekelemen
Copy link
Contributor Author

I need a review on this PR @KratosMultiphysics/structural-mechanics.

my problem with this is that it destroys the backward compatibility.

we need to have a mechanism that if someone has a old file using the old mechanism it still works

on the line of:

if(parameters.Has(block_builder_and_solver) : 
    throw watning 
    type = block
else
    ...

@RiccardoRossi I'm not sure whether I got my point across last time: I'm already doing this.

Copy link
Member

@philbucher philbucher left a comment

Choose a reason for hiding this comment

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

Makes sense to me, proper cleanup 👍🏻

for old_setting in old_unused_settings:
if settings.Has(old_setting):
KratosMultiphysics.Logger.PrintWarning("::[MechanicalSolver]:: ", 'Settings contain no longer used setting, please remove it: "{}"'.format(old_setting))
settings.RemoveValue(old_setting)
Copy link
Member

Choose a reason for hiding this comment

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

I think its time to issue an error. This has been deprecated for years

@RiccardoRossi
Copy link
Member

ok, this makes sense also for me.

@KratosMultiphysics/technical-committee has the final decision ... (and also should take a look at the deprecation comment)

@AlejandroCornejo
Copy link
Member

ok, this makes sense also for me.

@KratosMultiphysics/technical-committee has the final decision ... (and also should take a look at the deprecation comment)

maybe we also need to update our GiD interface...

@RiccardoRossi
Copy link
Member

we should, nevertheless the old version should still work

@sunethwarna sunethwarna moved this to 👀 Next meeting TODO in Technical Commiittee Apr 30, 2025
Copy link
Member

@RiccardoRossi RiccardoRossi left a comment

Choose a reason for hiding this comment

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

@KratosMultiphysics/technical-committee agrees provided that backward compatibility is guaranteed (which i understand is the case).

I will open a separate issue to decide what to do about "historically deprecated" stuff

@RiccardoRossi
Copy link
Member

@matekelemen why you did not merge this finally?

@matekelemen matekelemen merged commit e917ac1 into master May 24, 2025
11 checks passed
@matekelemen matekelemen deleted the structural/builder-and-solver-settings branch May 24, 2025 21:21
@github-project-automation github-project-automation bot moved this from 👀 Next meeting TODO to ✅ Done in Technical Commiittee May 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

6 participants