Add tests for PET BMI module (replaces PR#401)

[stcui007]

Add PET module and related .github test runners

Additions

data/example_bmi_multi_realization_config_w_noah_pet_cfe__linux.json
data/example_bmi_multi_realization_config_w_noah_pet_cfe__macos.json

Removals

Changes

.github/workflows/module_integration.yml
.github/workflows/test_and_validate.yml
include/realizations/catchment/Bmi_Module_Formulation.hpp

Testing

unit test
bmi tests
compare_pet

Screenshots

Notes

Todos

Checklist

• PR has an informative and human-readable title
• Changes are limited to a single goal (no scope creep)
• Code can be automatically merged (no conflicts)
• Code follows project standards (link if applicable)
• Passes all existing automated tests
• Any change in functionality is tested
• New functions are documented (with a description, list of inputs, and expected output)
• Placeholder code is flagged / future todos are captured in comments
• Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
• Reviewers requested with the Reviewers tool ➡️

Testing checklist (automated report can be put here)

Target Environment support

• Linux

[mattw-nws]

So your Linux build problem happens at linking, and it looks like after #417 (specifically 891d6c2 ) you will need to include {$NETCDF_LIBRARIES} in the compare_pet target (see the compare_cfe target right before it in CMakeLists.txt in test directory), or set the use_netcdf parameter of the build action to off. This may also solve your macOS build problem but I haven't looked at it.

You also seem to be getting a linker issue with pthreads, but linking against core::mediator should include that, so I'm not sure why that's happening... solve the NetCDF problem first and see what happens.

[stcui007]

Thanks. I was looking at the error details and was wondering how netCDF got involved since Bmi_C_Pet_IT.cpp doesn't use netCDF. Also Bmi_C_Pet_IT.cpp uses CsvPerFeatureForcingProvider so that probably needs to be considered also. Not sure why compare_pet doesn't fail in testing on UCS machines when using Forcing.h.

…

On Mon, Aug 1, 2022 at 11:02 AM Matt Williamson ***@***.***> wrote: So your Linux build problem happens at linking, and it looks like after #417 <#417> (specifically 891d6c2 <891d6c2> ) you will need to include {$NETCDF_LIBRARIES} in the compare_pet target (see the compare_cfe target right before it in CMakeLists.txt in test directory), or set the use_netcdf parameter of the build action to off. This may also solve your macOS build problem but I haven't looked at it. You also seem to be getting a linker issue with pthreads, but linking against core::mediator should include that, so I'm not sure why that's happening... solve the NetCDF problem first and see what happens. — Reply to this email directly, view it on GitHub <#429 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACA4SRMG5KLTU67TNACMWXLVW7YHFANCNFSM55BZ7UWQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[mattw-nws]

Not sure why compare_pet doesn't fail in testing on UCS machines when using Forcing.h

I don't think the use of Forcing.h (or CsvPerFeatureForcingProvider.h for that matter) in Bmi_C_Pet_IT.cpp file does anything. The forcing params are passed up to the Formulation_Manager which obtains the forcings via whatever the config says.

As for the code building successfully in other environments, that probably depends on the build options used. The point of my comment above is that #417 defaulted building with NetCDF to "on" in the Github workflows... if it's not on in your build, then it probably works fine. That said, if you turn NetCDF support on in your build, it will probably break like it is here in the workflow.

[stcui007]

Made some progress, able to link to netCDF. But still getting the "Can't init bmi_c_pet; unreadable shared library file ''" error.

…

On Mon, Aug 1, 2022 at 11:56 AM Matt Williamson ***@***.***> wrote: Not sure why compare_pet doesn't fail in testing on UCS machines when using Forcing.h I don't think the use of Forcing.h (or CsvPerFeatureForcingProvider.h for that matter) in Bmi_C_Pet_IT.cpp file does anything. The forcing params are passed up to the Formulation_Manager which obtains the forcings via whatever the config says. As for the code building successfully in other environments, that probably depends on the build options used. The point of my comment above is that #417 <#417> defaulted building with NetCDF to "on" in the Github workflows... if it's not on in your build, then it probably works fine. That said, if you turn NetCDF support on in your build, it will probably break like it is here in the workflow. — Reply to this email directly, view it on GitHub <#429 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACA4SRMEHOG3E7LXACNCVHDVW76TJANCNFSM55BZ7UWQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[stcui007]

This PR has passed all tests. On Mon, Aug 1, 2022 at 3:46 PM Shengting Cui - NOAA Affiliate < ***@***.***> wrote:

…

Made some progress, able to link to netCDF. But still getting the "Can't init bmi_c_pet; unreadable shared library file ''" error. On Mon, Aug 1, 2022 at 11:56 AM Matt Williamson ***@***.***> wrote: > Not sure why compare_pet doesn't fail in testing on UCS machines when > using Forcing.h > > I don't think the use of Forcing.h (or CsvPerFeatureForcingProvider.h > for that matter) in Bmi_C_Pet_IT.cpp file does anything. The forcing > params are passed up to the Formulation_Manager which obtains the > forcings via whatever the config says. > > As for the code building successfully in other environments, that > probably depends on the build options used. The point of my comment above > is that #417 <#417> defaulted > building with NetCDF to "on" in the Github workflows... if it's not on in > your build, then it probably works fine. That said, if you turn NetCDF > support on in your build, it will probably break like it is here in the > workflow. > > — > Reply to this email directly, view it on GitHub > <#429 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/ACA4SRMEHOG3E7LXACNCVHDVW76TJANCNFSM55BZ7UWQ> > . > You are receiving this because you authored the thread.Message ID: > ***@***.***> >

[mattw-nws]

Since there's a redundant/copy file in here and #430 got done at the same time, lets remove both the __linux and __macos realization files and edit the remaining realization config to remove the extension (see changes to JSON and YAML in #430).

[mattw-nws]

This file is a copy of the one without __linux...

[mattw-nws]

Okay those changes look good but you did a merge on Github (probably) that should have been a rebase: acd4a49

[stcui007]

I did a rebase to upstream master and then merged the master into the ngen_devPET.

[stcui007]

All were done on linux, not github.

[mattw-nws]

The merge step there is unnecessary... the rebase is the merge. I think if you rebase onto upstream/master again that merge commit will disappear. Try that and push (force) again.

[stcui007]

Do I run the rebase on the ngen_devPET branch directly or run it on the master branch and then 'git checkout ngen_devPET'? Does force push overwrite the changes I made to module_integration.yml and realization json?

…

On Thu, Aug 4, 2022 at 2:59 PM Matt Williamson ***@***.***> wrote: The merge step there is unnecessary... the rebase is the merge. I think if you rebase onto upstream/master again that merge commit will disappear. Try that and push (force) again. — Reply to this email directly, view it on GitHub <#429 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACA4SRPRYUYABQUMCMJDIBTVXQOK3ANCNFSM55BZ7UWQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[mattw-nws]

git checkout ngen_devPET
git rebase upstream/master # assumes you have a remote named upstream
git log # commit acd4a49 should be missing but everything else should be the same
git push --force ngen_devPET # will push what you just saw in git log

[stcui007]

I tried to run "git pull --rebase upstream master" in the ngen_devPET, it failed because there are changes in module_integration.yml.

…

On Thu, Aug 4, 2022 at 2:59 PM Matt Williamson ***@***.***> wrote: The merge step there is unnecessary... the rebase is the merge. I think if you rebase onto upstream/master again that merge commit will disappear. Try that and push (force) again. — Reply to this email directly, view it on GitHub <#429 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACA4SRPRYUYABQUMCMJDIBTVXQOK3ANCNFSM55BZ7UWQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[stcui007]

I got an error:
$ git rebase upstream/master
fatal: Needed a single revision
invalid upstream upstream/master

$ git remote -v
origin https://github.com/stcui007/ngen (fetch)
origin https://github.com/stcui007/ngen (push)
upstream https://github.com/NOAA-OWP/ngen.git (fetch)
upstream https://github.com/NOAA-OWP/ngen.git (push)

[stcui007]

$git log
commit e6139ba
Author: Shengting Cui [email protected]
Date: Thu Aug 4 18:42:49 2022 +0000

Make changes to accomodate upstream master change using extensionless lib file name.

commit acd4a49
Merge: 7f3e05e f64ffd3
Author: Shengting Cui [email protected]
Date: Thu Aug 4 18:15:19 2022 +0000

Merge branch 'master' into ngen_devPET

commit f64ffd3
Author: Matt Williamson matthew.williamson@gov
Date: Wed Aug 3 20:54:13 2022 +0000

Added extensionless solution also.