mdolab / adflow

ADflow is a finite volume RANS solver tailored for gradient-based aerodynamic design optimization.
Other
229 stars 100 forks source link

Tapenade automake #319

Closed DavidAnderegg closed 1 year ago

DavidAnderegg commented 1 year ago

Purpose

This replaces the tapenade-makefile with a python script to generate differentiated fortran code.

The old makefile would allways differentiate all the files. This by itself takes a lot of time. Also building ADflow afterwards takes a lot of time because the build process thinks, all differentiated files have changed and rebuilds all of them.

The proposed python script in this PR detects when a source file is newer than its differentiated version and selectively runs tapenade only on the changed files. This speeds up the whole process.

Expected time until merged

1 week..?

Type of change

Testing

Checklist

flake8 fails because the AD-routine definition could have a lot of characters per line. It is possible to break those strings, but I think this makes it highly unreadable.

codecov[bot] commented 1 year ago

Codecov Report

Merging #319 (a35c65f) into main (ab4323d) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #319   +/-   ##
=======================================
  Coverage   41.98%   41.98%           
=======================================
  Files          13       13           
  Lines        4001     4001           
=======================================
  Hits         1680     1680           
  Misses       2321     2321           

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

DavidAnderegg commented 1 year ago

I played a bit around now, i found the following 'bugs': 1) when a routine-definiton in run_tapenade.py is changed, it does not trigger re-differentiation as this file itself is not monitored. One has to manually trigger re-differentiation. 2) Apparently, tapenade differentiates stuff in the dependency files as needed. When one differentiates file A that depends on dependency X, only the content that file A needs is differentiated in X. But if there is a second file B, that also depends on X, this content is removed from dependency X as tapenade does not know about file B, when there is only file A running.

I think there is no good solution to bug 1).

For bug 2), i would argue, it is not a big problem because the missing content in dependency X shows up in the git diff. Also ADflow does not compile if it is missing. To fix this, we would probably need to edit the files in autoEdit as they copy everything into the differentiated folder and thus overwriting existing files. But I think it is an edge case and probably not worth fixing atm. When in doubt, one can still differentiate everything and get the same behavior as with the old make file.

eirikurj commented 1 year ago

I have not looked at the script in detail or run it, just skimmed over it. While the idea is great I have some initial comments/concerns:

DavidAnderegg commented 1 year ago

@eirikurj ~~I think your first point is what i call bug 2 in the comment above. Right? I might want to add that I see the partial differentiation more as a debugging tool to improve speed. I would argue the person debugging will understand that bug quickly. But i agree, it could lead to weird behavior. But the final test running in '.github/build_tapenade.sh' would catch it as it is forced to differentiate everything.~~

I played around a bit more and I now understand your point. I should have talked to you before starting this...

Regarding your second point, i see what you mean. I would argue it is already partially possible with those flags:

I can easily add:

Let me know what you think?

DavidAnderegg commented 1 year ago

As agreed on in the maintenance meeting, there is no point in continuing this because tapenade needs to know all routines.

Thus closing.