generalize table reading in p3_iso_c #2857
Conversation
mahf708
changed the title
mahf708
changed the title
mahf708
changed the title
Co-authored-by: Andrew M. Bradley <[email protected]>
mahf708
changed the title
mahf708
changed the title
mahf708
changed the title
|
@ambrad, this PR has devolved into a bit of a mess, sorry :( What do you think about it in its current state? I think this PR "generalizes" (very slightly) the table reading. Essentially, it allows for weirdly long pathnames. Moreover, the other unclear aspect is why the Any suggestions for testing or other improvements? Should I run the AT on this to see what it will return? I think it will be BFB, but who knows?! |
|
Note: since this is for specific, non-supported-machine type of thing, I am happy to close this PR and apply these changes as "patches" elsewhere. |
|
Looks fine. Definitely let the AT run all its tests before merging. |
There was a problem hiding this comment.
For the record, note that the change from calling append_precision to hardcoded string concatenation is because something in the old append_precision code path in the container environment Naser is working on is not working. A small reproducer does not have the problem, so something subtle is going wrong.
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: mahf708 |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
|
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: mahf708 |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
|
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
mahf708
changed the title
mahf708
changed the title
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: mahf708 |
|
NOTICE: The AutoTester has encountered an internal error (usually a Communications Timeout), testing will be restarted, previous tests may still be running but will be ignored by the AutoTester... |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: mahf708 |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
|
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: mahf708 |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
SCREAM_PullRequest_Autotester_Mappy # 5518 FAILED (click to see last 100 lines of console output)SCREAM_PullRequest_Autotester_Weaver # 5789 PASSED (click to see last 100 lines of console output) |
|
@jgfouca @tcclevenger @AaronDonahue and other p3 devs: It seems to me that we read p3 tables at init, but we never use them in the cxx code. Is that right? I see them explicitly computed in Maybe it's time we separate the f90 stuff in p3 that is needed for unit tests only, and put it somewhere else (maybe in p3/tests). Or, at the very least, avoid doing this step in CIME runs, since it's pointless? |
|
P3 definitely needs those tables. I think we decided it was simpler to read them in fortran and then pass the raw data over to the cxx side, but that data is used heavily all over p3. Just do a grep for |
|
I know we use the tables, but it looks like we are computing them in this function and this function, which are called during P3's initialize_impl. All of this makes me wonder if P3 itself needs to load those files in a CIME run... |
|
OK, my memory is starting to jog. I remember now that I did port the table reads from f90 to cxx. The second function you linked does file I/O, so the tables are still being read in cxx just like in f90 unless I misunderstood what you are saying. |
|
Ah, yes, I completely missed the ifstream part in that file. |
|
I'm not sure what our code is doing at this point, but here's a science perspective: it would be beneficial from a research standpoint if we could build as many of these tables as possible during the init phase of each run (or at least during the init phase of each startup rather than restart run). There are a lot of uncertain parameters in the creation of these tables and if we created the tables inline we could include tuning of those parameters in PPEs in a natural way. Another reason why it is good to calculate the tables inline is that it provides better provenance: if we just grab tables from some random place we have no way of knowing what they're doing. If those tables are calculated within our own code, we can always dive into them if needed. I'm not saying we need to make changes right now, just that if/when we can we should calculate these changes inline. Maybe we can get rid of the netcdf tables in the legendary time when DP is done and we abandon Fortran support? |
|
@PeterCaldwell , it's been so long that my memory is foggy, but I am pretty sure I remember that some of these tables were quite expensive (over 10 seconds) to create which is why we changed them to file reads. |
I wonder if they were expensive to build b/c we built them on host in F90 (I assume that's what we did). Maybe if we built them on device things would be better? Although, this would only matter for a GPU build... |
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: mahf708 |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
|
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
Two issues that appeared in cases where the produced shared object was patched to make the hard links softer.
writelogic in the append_precision wasn't working, so fixing that with a more hardcoded char concatIn both cases, the issues weren't reproducible in supported machines or supported builds (the issue is essentially when the shared objects are built on one machine and used on another, hence the need to change the hard links to softer links, so that the paths can be different on different machines)