Updates for openfast 3.5.*

marchdf commented 3 months ago

OpenFAST 3.5.* has a bunch of fixes. Including

and probably others. But this requires that we update our interface to OpenFAST. We've had discussions about this in the past but I think the time has come.

Maybe it becomes necessary to enforce this on the exawind-manager side (@psakievich)?

Tagging people for discussion: @psakievich, @gantech, @lawrenceccheung

Closes #850

andrew-platt commented 3 months ago

I expect we will release and tag OpenFAST 3.5.3 later this week.

If you need a current set of input files for OpenFAST, versions 3.5.0, 3.5.1, 3.5.2, and 3.5.3 are all identical input files. Input files can be found here: https://github.com/OpenFAST/r-test/tree/v3.5.2/glue-codes/openfast

psakievich commented 3 months ago

I don't think I like these changes unless you want to just deprecate all prior openfast versions. This one will make it so there is a banded subset of openfast versions that amr-wind can't compile against. Basically whatever is between the SCDX change and what amr-wind compiles against now.

andrew-platt commented 3 months ago

If you are using AMR-Wind with the current OpenFAST dev branch, that will be version 4.0.* (mid April). Bugfixes in OpenFAST 3.5.3 will be pulled into that version.

marchdf commented 3 months ago

It was my understanding (possibly flawed) that there are bugfixes in openfast that we want to be using. We have to rely on a random commit in openfast on Frontier (one that contains the flang update but doesn't contain the API changes). There is also the fact that checkpoints are corrupted on Frontier if we aren't able to update to 3.5.3. I get that this breaks older openfast but my thinking is that we want people to be using the newer versions anyway. But of course this means they will have to update their input files... I am open to suggestions for other ways of doing this. We talked about that interface stuff but that feels like a ton of work we don't have the resources for. But maybe that's shortsighted of me.

Maybe we are better off talking about this on a call. I can set that up but please lmk who wants to be involved (assuming the ones on this thread at the very least)

lawrenceccheung commented 3 months ago

I agree with @psakievich, if we decide to deprecate all previous versions of OpenFAST, we'll need to very clear to the user base that this is happening, and that to some degree people will want to keep using older versions of amr-wind because changing openfast and their turbine models might change their results. So some kind of interface would be helpful -- it'd be a lot of work, but could also save confusion in the future.

Lawrence

psakievich commented 3 months ago

It was my understanding (possibly flawed) that there are bugfixes in openfast that we want to be using. We have to rely on a random commit in openfast on Frontier (one that contains the flang update but doesn't contain the API changes). There is also the fact that checkpoints are corrupted on Frontier if we aren't able to update to 3.5.3. I get that this breaks older openfast but my thinking is that we want people to be using the newer versions anyway. But of course this means they will have to update their input files... I am open to suggestions for other ways of doing this. We talked about that interface stuff but that feels like a ton of work we don't have the resources for. But maybe that's shortsighted of me.

Maybe we are better off talking about this on a call. I can set that up but please lmk who wants to be involved (assuming the ones on this thread at the very least)

I am fine with breaking all older openfasts, but I would also delete the other interface in the code as well so it is consistent. My point is either break it off or find a way to support to all of it. Happy to get on a call, but I don't think I have much more to say. I am not in a position to defend the preservation of any legacy Openfast versions.

andrew-platt commented 3 months ago

From the OpenFAST perspective, I don't know why you would want to support versions prior to 3.5.* at this point. We don't provide any bugfixes for earlier versions.

In case it is of use for your planning, we do intend to maintain the 3.5.x with bugfixes for some time after 4.0.x (current dev / dev-unstable-pointers branches) and 5.0.x are released. So there might be an argument for maintaining interfaces for 3.5.x, 4.x, and 5.x simultaneously.

lawrenceccheung commented 3 months ago

Also tagging @ndevelder on this.

psakievich commented 3 months ago

From the OpenFAST perspective, I don't know why you would want to support versions prior to 3.5.* at this point. We don't provide any bugfixes for earlier versions.

In case it is of use for your planning, we do intend to maintain the 3.5.x with bugfixes for some time after 4.0.x (current dev / dev-unstable-pointers branches) and 5.0.x are released. So there might be an argument for maintaining interfaces for 3.5.x, 4.x, and 5.x simultaneously.

The only reason I can think of is to reproduce old results. To me this is more a question for the user community. I'm all for limiting the dependency. Users can always build older versions of arm-wind to reproduce as well...

marchdf commented 3 months ago

I am fine with breaking all older openfasts, but I would also delete the other interface in the code as well so it is consistent.

Good idea.

I don't know why you would want to support versions prior to 3.5.* at this point

Ok good to know

To summarize so far

lack of bug fixes in old versions of openfast means users should be on 3.5.* (or above)
This is probably user unfriendly in the short run (they need to update their input files, might cause diffs) but better in the long run (they will be avoiding time-consuming bugs)
Reverting back to old amr-wind versions is always possible (and the diffs of this kind of change are easily carried in a patch)

What I am planning to do, unless I hear otherwise

I will update the code so we only support 3.5 API and add clear documentation of that fact and with links to how to update input files (in the openfast docs).

psakievich commented 3 months ago

@marchdf I agree with your plan. We might want to also cut a release as part of this since it is a notable API for the majority of users.

marchdf commented 3 months ago

Good idea on the release.

ndevelder commented 2 months ago

This is a minor comment, but we should probably make sure either/both the spack package and the local repo openfast package are set up for version 3.5.* before we merge? And maybe put in some rules about version dependencies for older versions of openfast (maybe using Phil's tagged release idea)?

psakievich commented 2 months ago

Yes we should do that. Let's do it in the spack repo for all versions that have tags in openfast. I would prefer we do it there so we don't have to do it twice.

marchdf commented 2 months ago

@psakievich happy to do this though I might need some pointers, will reach out to you separately.

marchdf commented 2 months ago

Ok we came up with the game plan:

merge this PR
cut v2.0 amr-wind release
add versions to amr-wind and openfast spack packages with a constraint that amr-wind@2: requires openfast@3.5:

All good with this?

ndevelder commented 2 months ago

Maybe this is backward, but don't we want the tagged release before changing the api, so that there's a last good point for the older versions of openfast that we can specify? And everyone who is upgrading keeps using main?

marchdf commented 2 months ago

Oh you mean, do all this with the current stuff. Sorry I think I misunderstood your earlier comment. Updated plan:

update amr-wind spack package with a contraint that v1.3.0 uses openfast <= 3.4.1
merge this PR
cut v2.0 amr-wind release
add versions to amr-wind and openfast spack packages with a constraint that amr-wind@2: requires openfast@3.5:

Would that work?

ndevelder commented 2 months ago

I might be confusing this more...apologies...I just wanted to have a way to specify the latest old openfast compatible version without having to look up the commit sha

@psakievich is there a strict greater than constraint for version dependencies? If not what I'm suggesting wouldn't work.

We can also specify individual commits as versions in the spack package like so: version("2014-10-08", commit="9d38cd4e2c94c3cea97d0e2924814acc")

So we could make a "old-openfast" version with the current commit without cutting any release...and go with Marc's first timeline.

psakievich commented 2 months ago

I might be confusing this more...apologies...I just wanted to have a way to specify the latest old openfast compatible version without having to look up the commit sha

@psakievich is there a strict greater than constraint for version dependencies? If not what I'm suggesting wouldn't work.

They have <=

requires("openfast@3.5:", when="@2:")
requires("openfast@:3.4.5", when="@1")

This should cover us I think? I don't know the exact versions here. Just an example

We can also specify individual commits as versions in the spack package like so: version("2014-10-08", commit="9d38cd4e2c94c3cea97d0e2924814acc")

So we could make a "old-openfast" version with the current commit without cutting any release...and go with Marc's first timeline.

I don't think we need this? AMR-Wind's compatibility broke with an openfast release already right? I think we should stick to the release versions unless there is a compelling reason not to

marchdf commented 2 months ago

@psakievich and @ndevelder, I got a little lost in the discussion. Can you take a look at https://github.com/spack/spack/pull/43728 and let me know what to change?

marchdf commented 2 months ago

With #1031 fixing #1009, I think we can now merge this. Then we can bump amr-wind versions and I can update the spack package. Everyone ok with this?

ndevelder commented 2 months ago

One of our team members just had an issue compiling your PR branch.

`[ 60%] Building CXX object CMakeFiles/amrwind_obj.dir/amr-wind/wind_energy/actuator/disk/disk_ops.cpp.o

/projects/wind_advcontrol/gyalla/src/amr-wind/amr-wind/wind_energy/actuator/turbine/fast/FastIface.cpp: In instantiation of 'void exw_fast::{anonymous}::fast_func(const FType&&, Args ...) [with FType = void(int, double, const char, int, int, int, int, float, float, int, int, float, int, double, int, int, int, exw_fast::OpFM_InputType, exw_fast::OpFM_OutputType, exw_fast::SC_DX_InputType, exw_fast::SC_DX_OutputType, int, char); Args = {int, double, char, int, int, int, int, float, float, int, int, float, int, double, int, int, exw_fast::OpFM_InputType, exw_fast::OpFM_OutputType, exw_fast::SC_DX_InputType, exw_fast::SC_DX_OutputType*}]':

/projects/wind_advcontrol/gyalla/src/amr-wind/amr-wind/wind_energy/actuator/turbine/fast/FastIface.cpp:223:14: required from here

/projects/wind_advcontrol/gyalla/src/amr-wind/amr-wind/wind_energy/actuator/turbine/fast/FastIface.cpp:22:9: error: cannot convert 'exw_fast::OpFM_InputType' to 'int' in argument passing

22 | func(std::forward(args)..., &ierr, err_msg);

  |     ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[ 61%] Building CXX object CMakeFiles/amrwind_obj.dir/amr-wind/wind_energy/actuator/disk/Joukowsky_ops.cpp.o

[ 61%] Building CXX object CMakeFiles/amrwind_obj.dir/amr-wind/wind_energy/actuator/disk/uniform_ct_ops.cpp.o

make[2]: *** [CMakeFiles/amrwind_obj.dir/build.make:1378: CMakeFiles/amrwind_obj.dir/amr-wind/wind_energy/actuator/turbine/fast/FastIface.cpp.o] Error 1

make[2]: *** Waiting for unfinished jobs....

make[1]: *** [CMakeFiles/Makefile2:1625: CMakeFiles/amrwind_obj.dir/all] Error 2

make: *** [Makefile:146: all] Error 2`

All the CI checks have passed though, so maybe this is a compiler specific thing? I can investigate more.

marchdf commented 2 months ago

The CI doesn't build openfast so that shouldn't be it. Can you make sure it's not a version mismatch still?

psakievich commented 2 months ago

@ndevelder it is a mismatch (AI generated table)

| FType                                   | Args                                   |
|-----------------------------------------|----------------------------------------|
| int*                                    | int*                                   |
| double*                                 | double*                                |
| const char*                             | char*                                  |
| int*                                    | int*                                   |
| int*                                    | int*                                   |
| int*                                    | int*                                   |
| int*                                    | int*                                   |
| float*                                  | float*                                 |
| float*                                  | float*                                 |
| int*                                    | int*                                   |
| int*                                    | int*                                   |
| float*                                  | float*                                 |
| int*                                    | int*                                   |
| double*                                 | double*                                |
| int*                                    | int*                                   |
| int*                                    | int*                                   |
| int*                                    | exw_fast::OpFM_InputType*              |
| exw_fast::OpFM_OutputType*              | exw_fast::OpFM_OutputType*             |
| exw_fast::SC_DX_InputType*              | exw_fast::SC_DX_InputType*             |
| exw_fast::SC_DX_OutputType*             | exw_fast::SC_DX_OutputType*            |
| int*                                    | --                                     |
| char*                                   | --                                     |

ndevelder commented 2 months ago

Haha, yeah I was just counting out arguments and it's the extra int, ok, I'll let him know. Sorry that was a false issue

Exawind / amr-wind

Updates for openfast 3.5.* #1006

To summarize so far

Other solutions to support multiple version (which we might need to do with v4 and v5):

What I am planning to do, unless I hear otherwise