Avoiding unnecessary setup operations #503
Conversation
Needed problem information are now collected in one step to avoid unneeded setup operations.
write_needed_inputs() back in FASTOADProblem When not using high-level API, it will allow to have problem analysis done once when doing write_needed_inputs() and not in subsequent operations.
christophe-david
force-pushed
the
setup-optimisation
branch
from
6af190d to
9f9c86c
Compare
christophe-david
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #503 +/- ##
==========================================
+ Coverage 84.16% 84.24% +0.08%
==========================================
Files 132 132
Lines 6061 6081 +20
Branches 891 896 +5
==========================================
+ Hits 5101 5123 +22
+ Misses 804 803 -1
+ Partials 156 155 -1 ☔ View full report in Codecov by Sentry. |
| problem = FASTOADProblem() | ||
| self._build_model(problem) |
There was a problem hiding this comment.
It is not a change in the PR but I'm wondering here if this line means that we rebuild the problem each time we call the get_problem method. For instance if we were to keep calling the write_needed_inputs from the FASTOADProblemConfigurator class we would construct the problem twice.
There was a problem hiding this comment.
Indeed, the problem is built each time get_problem is called. I think it's better like this because:
- if a user called
get_problem, he is able to keep the generated instance, and I think he has no reason to callget_problemagain unless he wants a fresh new instance. - we could imagine a case where the configuration is read,
get_problemis called, then the configuration is programmatically modified (e.g. optimization definition) andget_problemis called again. In such case, a fresh new instance is really better.
This is why write_needed_inputs implementation has been moved to the FASTOADProblem class, which allows doing this operation on the instance that will be used for the computation.
src/fastoad/openmdao/_utils.py
Outdated
|
|
||
| A deepcopy operation may crash if problem.comm is not pickle-able, like a | ||
| def get_problem_copy_without_mpi(problem: T) -> T: |
There was a problem hiding this comment.
I'm not sure about the name of the function. From the description, I seem to understand that this function allows to avoid the problem with MPI by removing the non-pickable object beforehand and readding it afterwards, which would work regardless of whether the problem has MPI or not. So the wihout_mpi seems more like a mpi_safe ?
There was a problem hiding this comment.
Good point. Name adopted in 9bba940.
src/fastoad/openmdao/problem.py
Outdated
| self.model.add_subsystem(SHAPER_SYSTEM_NAME, ivc, promotes=["*"]) | ||
| self.self_analysis() | ||
|
|
||
| self._analysis.fills_dynamically_shaped_inputs(self) |
There was a problem hiding this comment.
Isn't this line already ran when you do a ProblemAnalysis ?
There was a problem hiding this comment.
I had hesitated making a property for that, that would control the running of the analysis at the first usage.
Your question made me decide it was the way to go. Modification done in 4eeb488.
src/fastoad/openmdao/problem.py
Outdated
| } | ||
| ) | ||
| return dynamic_vars | ||
| self.model.set_order([subsystem_name] + self._analysis.subsystem_order) |
There was a problem hiding this comment.
Again not linked with this PR, but do we need to reorder the subsystems ? Isn't naturaly done when we add the models ?
There was a problem hiding this comment.
A sure thing is that the component are added in the order of the add_subsystem calls.
The fact is that removing this line makes 2 tests fail (these tests do optimization on the Sellar problem).
Other tests (unit and integration) still work fine.
For the record, with the last OpenMDAO version installed, using self.model.options["auto_order"] = True makes the 2 tests work again.
Actually, the 2 tests fail (give incorrect results) because there is no solver at the Problem.model level. Indeed, with an IVC in last position, a loop is needed to have the IVC output correctly fill the needed inputs.
christophe-david
force-pushed
the
setup-optimisation
branch
from
a5ae7ac to
52fe649
Compare
It will avoid any question about when calling the analysis method. The analysis will be done the first time the property will be used.
christophe-david
force-pushed
the
setup-optimisation
branch
from
b7c3c26 to
4eeb488
Compare
This PR fixes some performance problems of FAST-OAD.
To get knowledge of the structure of OpenMDAO problems, a setup operation is needed. Until now, due to the several changes that were done separately, this setup operation was done almost each time information was needed, which proved to be CPU costly.
Here the problem analysis is done in only one place, which allows to reduce to the minimum the number of setup calls. There is still one "additional" setup operation needed to analyze the problem, and possibly another one if dynamically shaped variables are inputs of the problem.
Also,
write_needed_inputsimplementation has been put back inFASTOADPoblem(while keeping theFASTOADProblemConfiguratormethod), which allows to spare 2 setup calls when called on the problem instance that will do the computation (when done fromFASTOADProblemConfigurator, it is done on a separate instance).As a result, the integration test
test_oad_processuses now 2 setup calls, instead of 10 previously.