NOAA-EMC / RDASApp

Regional DAS
GNU Lesser General Public License v2.1
0 stars 5 forks source link

Add scripts for setting up an experiment from ctest data and more #25

Closed delippi closed 3 weeks ago

delippi commented 4 weeks ago

The main purpose of this PR is to add scripts to help a user set up and run their own test case from the ctest data. Here is a list of updates that were made:

Also, please follow the draft instructions on my fork's wiki page. Once those instructions are also approved I will update the wiki page here on NOAA-EMC/RDASApp delippi/RDASApp wiki

delippi commented 4 weeks ago

Hi @ShunLiu-NOAA , @CoryMartin-NOAA , @TingLei-NOAA. Please note that this is a draft PR! It is still a work in progress. As of right now I only have things working for FV3 and Hera. As we finalize how we wish to proceed with things I will also add the similar tools for MPAS test case and also for Orion. Also, please follow the draft instructions on my fork's wiki page (the link is up in the description). Once those instructions are also approved I will update the wiki page here on NOAA-EMC/RDASApp

TingLei-NOAA commented 4 weeks ago

@delippi those changes look good to me. Thanks. Just a couple minor comments. One is that in https://github.com/NOAA-EMC/RDASApp/blob/feature/rrfs-test-updates/rrfs-test/CMakeLists.txt, line 19 and 49, it is conditioned on fv3_core and mpas_core to decide if corresponding ctestst should be added. They should be coordinated with the new "-r" option. And, it is still confusion to me. "-r" =No means we don't add rrfs tests or just we can skip the cloning of data because data is already available?

delippi commented 4 weeks ago

@TingLei-NOAA, Right now I have the -r option to mean that the user wants to include the data. I don't know if we should make it such that it downloads by default and the option turns it off.

I'm a little confused with the other CMakeLists how this is all set up, because I thought the actual copying was done in the other CMakeList. Anyway, it seems to work how I have it, and the logic I added, I think, keeps it from downloading data for both dycores unless specified, but didn't test that thoroughly. I will though.

delippi commented 4 weeks ago

@TingLei-NOAA, do the wiki instructions on my fork look okay? And by that I mean did it work? I can clean up some of the language as we discussed earlier. If you followed them all the way through you should have been able to create an increment plot of my single ob case.

ShunLiu-NOAA commented 4 weeks ago

Your fork's wiki instructions appear good to me. It will be very helpful to those who want to begin a simple VAR test.

delippi commented 3 weeks ago

I think I have everything working for FV3/hera. Let's go ahead and try to get this merged sooner rather than waiting for MPAS/orion/hera and FV3/orion to be ready. I just changed from draft PR.

TingLei-NOAA commented 3 weeks ago

@delippi Does it work for mpasjedi? If not, we 'd better wait for this function that the current develop branch has will not be affected.

TingLei-NOAA commented 3 weeks ago

@TingLei-NOAA, Right now I have the -r option to mean that the user wants to include the data. I don't know if we should make it such that it downloads by default and the option turns it off.

I'm a little confused with the other CMakeLists how this is all set up, because I thought the actual copying was done in the other CMakeList. Anyway, it seems to work how I have it, and the logic I added, I think, keeps it from downloading data for both dycores unless specified, but didn't test that thoroughly. I will though.

@delippi I am still a little confused by what you meant by "downloads" using this -r option. Now, all those data are local in the location (RDAS_RRFS_DATA_ROOT). Did you mean we just don't need to copy those data there to the user's bundle dir? if so, as I mentioned previously, some coordination are needed, while in this case the rrfs ctest should not be added too (the adding would fail too if there is no such data). if you meant -r would give users another option to directly download test data from github, that does make sense. But that means we need to store data of large size on github and some corresponding changes are needed in the Cmake files in bundle.

delippi commented 3 weeks ago

@TingLei-NOAA, I will try to run mpasjedi again.

As for your confusion with my word "downloads" I just mean copy it from the staged local location. If you only build FV3 then we don't need to also copy the MPAS data and vice versa. It will only copy both sets of data if both FV3andMPAS (default) is the chosen dycore.

TingLei-NOAA commented 3 weeks ago

@delippi Thanks for this clarification about "-r " option, so , basically, it controls if the corresponding ctests are needed , right? a minor suggestion is to use a more clear name like " SET_RRFS_TESTS". Another thing is that , then, changes in rrfs-tests/C*txt are needed to make sure when test data are not avaialbe, the the corresponding blocks of adding ctests will not be run.
A simple test is to turn off this "-r" option and see if the whole building process would finish smoothly and give the expected results ( now, no rrfs tests are avaiable)

delippi commented 3 weeks ago

@TingLei-NOAA , it doesn't control which ctest will run. It just controls which data will be copied from your staged location. If you compile as FV3andMPAS, both test data will be copied. If you only compile FV3 or MPAS, only the corresponding data set will be copied from your location. If you only compile one dycore, the other dycore ctest will fail regardless of having the data or not. You would then expect some failures having only downloaded the data and compiled for one dycore.

As for adding checks to see if the data exists before running the corresponding ctest, it might be okay to not do that?? I think you would want it to fail if you didn't download and compile everything. I don't think you would want it to give a false positive if all the tests (FV3 and MPAS) weren't run. It might be something we think about doing though.

So I think keeping the variable name as "CLONE_RRFSDATA" instead of "SET_RRFS_TEST" is fine. The CLONE_RRFSDATA is just following the naming convention for the JCSDA ctest file but in this case we are just copying local data.

TingLei-NOAA commented 3 weeks ago

@delippi What I meant is that, "-r" option controls if the test data are available to the users ( since it doesn't care if the data is already in the user's dir and, hence, no need to copy data to the users' account) and hence it also "decides" if the corresponding ctests could be added ( while, the changes in rrfs-test cmake file are needed as i mentioned, when -r option is turned off). Maybe you could just run building with -r turned off and see how it is going.

delippi commented 3 weeks ago

@TingLei-NOAA, I think -r option should be considered much like the -d option. Its only purpose is to decide whether or not to get the data. I believe if you compile without -d option, you will have many ctest failures, right? Or does the -d option also control some ctests that are run? I am not sure.

TingLei-NOAA commented 3 weeks ago

@delippi No, when -d option is turned off, those corresponding ctests would not be added while the whole building process will finish successfully. That is exactly what I hope you could do in a coordinated way and don't let the building fail. In fact, it should be very simple, just make corresponding changes in cmake files in rrfs-test.

delippi commented 3 weeks ago

@TingLei-NOAA, okay. Could you point me to an example of where the option -d would turn on/off certain ctests? I want to make sure I do it consistently with how that is done.

TingLei-NOAA commented 3 weeks ago

@delippi in the rrfs-test/C*txt, you can decide how to using "if" avoid those adding tests blocks . I see one option is just to use your parameter to replace that hardwired rrfs-test-data_FOUND=1 on https://github.com/NOAA-EMC/RDASApp/blob/c1d9427e11c6e2e9c6b8eb2f8f5122b148e4066d/rrfs-test/CMakeLists.txt#L7.

delippi commented 3 weeks ago

@TingLei-NOAA, Okay, I think I have it working as you suggested.

delippi commented 3 weeks ago

@ShunLiu-NOAA @TingLei-NOAA , I'm having trouble with the rrfs ctests. Let me try to figure that out before we merge this. I'm only currently able to run my fv3jedi experiment based off the ctest data. It is just the ctest doesn't run.

delippi commented 3 weeks ago

@ShunLiu-NOAA @TingLei-NOAA @CoryMartin-NOAA , I just committed some updates, but now I am unable to push them to github. I get the following error. Did something happen to a repo setting or is this my own issue?

remote: Personal access tokens (classic) are forbidden from accessing this repository.
fatal: unable to access 'https://github.com/NOAA-EMC/RDASApp.git/': The requested URL returned error: 403
CoryMartin-NOAA commented 3 weeks ago

All NOAA-EMC repositories are dealing with this issue. No idea what happened. I've reached out to the organization owners to see if we can resolve this quickly.

delippi commented 3 weeks ago

Thanks for that info! It seems to be working now. I pushed some recent commits. I think everything is working now and I managed to get the hera/mpas scripts to work and in this PR.