Exawind
Updates for openfast 3.5.*
OpenFAST 3.5.* has a bunch of fixes. Including
- https://github.com/OpenFAST/openfast/pull/2097
- https://github.com/OpenFAST/openfast/pull/2120
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
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
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.
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.
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 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
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.
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.
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-pointersbranches) 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...
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)
Other solutions to support multiple version (which we might need to do with v4 and v5):
- Add compile time defines to support different openfast versions, with a default to 3.5. Personally not a fan, though it is a cheap option.
- Build a nifty interface: better solution, not trivial.
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).
@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.
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)?
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.
@psakievich happy to do this though I might need some pointers, will reach out to you separately.
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 [email protected]:
All good with this?
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?
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 [email protected]:
Would that work?
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.
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("[email protected]:", 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
@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?
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?
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>(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.
The CI doesn't build openfast so that shouldn't be it. Can you make sure it's not a version mismatch still?
@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* | -- |
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