-
Notifications
You must be signed in to change notification settings - Fork 312
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fates parameter file auto-build for all tests #2336
base: master
Are you sure you want to change the base?
Conversation
Here is a list of the parameter test file binaries it generates, one fates test creates 5.9M of data:
|
This should probably happen in a SystemTest, not in |
See also discussion from CTSM SE meeting here. |
I'm unable to get this to work on Izumi. It might be something wrong with my environment. Could someone else try this and see? ./run_sys_tests --skip-compare --skip-generate -t ERS_D_Ld15.f45_f45_mg37.I2000Clm50FatesRs.izumi_nag.clm-FatesColdTwoStream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cime_config/testdefs/testmods_dirs/clm/FatesColdSeedDisp/shell_commands
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few suggestions in the code. Also:
- Ensure this works for various users on Izumi.
cime_config/testdefs/testmods_dirs/clm/FatesColdSeedDisp/shell_commands
Outdated
Show resolved
Hide resolved
I'm seeing a failure trying this as well with the following:
Looking at the fates directory structure, the |
That is indeed the same error I was seeing, but I don't think it's |
I'm actually getting a similar error on Derecho, too. From
Tests in |
The issue is that |
@samsrabin thanks for the work on this. Note, that fates/parameter_files is under the FATES external so adding a binary_files subdirectory would require a PR to FATES. And I think for git, you have to have at least a README file in the directory for it to show up when you check it out... |
Wait, @ekluzek, even if the new directory is canonically in FATES, won't the checkout be unclean once the parameter file is generated? |
Actually… the checkout looks clean. |
@samsrabin yes exactly. But, it's good that you showed that's the case. It's good to confirm. |
That directory needs to be added in fates, sorry for that, I'll get it added to the next FATES PR. |
Improvements and fix for fates-auto-params
That merge commit I just did was to resolve conflicts introduced in my PR. They were only related to run_sys_tests.py and its testing. They're now resolved, and |
clm_aux on derecho, ok with exception:
FAIL DAE_C2_D_Lh12.f10_f10_mg37.I2000Clm50BgcCrop.derecho_intel.clm-DA_multidrv RUN time=304
|
That dang DAE test! Try resubmitting it. |
I tried the izumi test that was failing before and it works for me, so I checked that item off, which puts this in a ready to merge mode. |
This test fails create case: ERS_D_Ld15.f45_f45_mg37.I2000Clm50FatesRs.izumi_nag.clm-FatesColdTwoStream
However, i'm able to load scipy manually when I run python. Also, this test passed when I ran it stand-alone.. ie, this test passed:
|
DAE_C2_D_Lh12.f10_f10_mg37.I2000Clm50BgcCrop.derecho_intel.clm-DA_multidrv also still fails after re-submitting |
Since this isn't critical to come in now, we will plan on delaying this to fix the conda env issue on izumi (I think #2385 will fix this). @samsrabin also has some analysis that shows that there is a race condition for the DAE test that sometime results in a file being gzipped before something else needs I don't think the DAE issue should hold this one up, but that is another good thing to have come in. |
Undoing approval until it's actually ready
Getting this prioritized came up in discussion around NGEET/fates#1236. @ckoven suggested that we may want to also move towards building from the xml patch file. |
@ekluzek will start a document on this and shared it with interested parties by our Nov 6 meeting. |
Description of changes
This enables the automatic building of the fates parameter file binary for all tests. This calls ncgen from the shell_commands script in the Fates/ testdef folder, to operate on the fates default file that is version controlled.
Specific notes
This implementation is incomplete. In order to get this to work for all tests, I had to place the newly built binaries in the a new folder in the fates source tree. The reason for this is because some of the tests are multi-phase (PEM, ERP, etc). Each of these phases needs access to either the same parameter file, or an exact copy of it. However, the shell_commands (as far as my test show) script is only called the first time, so both parts of the test need access to the same file. Unfortunately, the xml files in both parts of the tests, do not provide any file-paths that are common to phase (I looked pretty thoroughly but maybe missed something), located somewhere on the scratch partition. For instance, they both have different cases, which makes it tough for us to locate the parameter file on the second test, if it has a different case as the first. I also tried using CIME_OUTPUT_ROOT, the sharedlib build location.
There is an xml entry in env_test.xml that is TEST_ARGV. This holds the root folder for the current test environment, and the id of the specific test currently run. With these two bits of information we could place a binary file that is accessible to all phases of a test. However, this information does not seem to be available via xml query at the time we run the shell_commands script.
Another location that might be better than the source, at least for the time being would be to put all these files in the CIME_OUTPUT_ROOT, which is usually just the scratch folder where all cases and tests go. Each parameter file could have the test name and hash in it, to prevent redundancy. The downside is that the root scratch folder starts to fill up.
Contributors other than yourself, if any:
@ekluzek @glemieux @adrifoster
Are answers expected to change (and if so in what way)?
Any User Interface Changes (namelist or namelist defaults changes)?
Testing performed, if any: