Adds a time-slice capability to Omega IO and IOStream #189
Conversation
Adds a capability to read/write multiple time slices of an unlimited time dimension to a single file. This modifies just the base IO layer - IOStream support will follow in another commit. The base IO unit test and documentation have been modified to test and document the new functionality.
adds the ability to write multiple time slices to a single file in IOStreams.
|
Passes unit tests on Chrys/Intel and Frontier Cray cpu and gpu |
There was a problem hiding this comment.
@philipwjones , this is great! This is what we need. Thanks a lot for your dedicated work on this, and I'm sorry for the delay in my review.
-
I have been testing this PR, and it performed as expected with the unit tests. However, I encountered an error while running the manufactured solution test. I will provide details on how to reproduce the error.
-
Should the time dimension and variable name
timebe changed toTimeto match Omega's naming conventions?
| TimeUnits FileUnits = TimeUnitsFromString(FileUnitStr); | ||
|
|
||
| // Various error checks | ||
| if (Err1 == 0 or Err2 == 0) { |
There was a problem hiding this comment.
It may be helpful to include a log statement specifying the frequency (Freq and FreqUnits) and file frequency (FileFreq and FileFreqUnits) utilized by Omega.
There was a problem hiding this comment.
In general, we need to echo options to the log file, but I need to make some changes to the logging facility as part of the error handler so will make the changes then.
|
Here is the omega.log (bottom half)And here is an instruction to reproduce the error on Frontier (cpu): Error reproduceWhen I commented out |
|
@hyungyukang Thanks, I'll look into the manufactured solution case, but it looks like it thinks the file should already exist and is trying to read dims for consistency. So it probably has to do with assumptions about whether a file was written at the alarm boundary, esp when a simulation starts on a boundary. Regarding time vs Time, there is no way to add a std name to dimensions so the dimension has to keep time as the name for CF compliance unless we want to translate it later. So I left the time field name consistent with the dimension name, though I could keep Time with time as a std name in that case. I'll let @xylar weigh in on the preference - don't know whether we're going to have a translation step at some point anyway as part of CMORizing. |
|
@philipwjones, sorry for the delay on this. I'll get to this as soon as I can. |
|
Quick update - there are indeed problems with the way I've computed the frame number in some specific cases (in addition to the bug above, there is one associated with calendar-based intervals like monthly and annual). These are limited to a small chunk of code that I should be able to fix pretty quickly. |
I do feel strongly that the coordinate
Yes, I think that's still inevitable but we do want cf-compliant output to the extent possible even so. |
There was a problem hiding this comment.
I tried to run in this directory with the following omega.yml:
$ pwd
/lcrc/group/e3sm/ac.xylar/polaris_0.5/chrysalis/test_20250116/cosine_bell_io_time_slice/ocean/spherical/icos/cosine_bell/convergence_both/with_viz/forward/480km_1440s
$ cat omega.yml
Omega:
TimeIntegration:
CalendarType: No Leap
TimeStepper: RungeKutta4
TimeStep: 0000_00:24:00.000
StartTime: 0001-01-01_00:00:00
StopTime: none
RunDuration: 0024_00:00:00.000
Dimension:
NVertLevels: 1
Decomp:
HaloWidth: 3
DecompMethod: MetisKWay
State:
NTimeLevels: 2
Advection:
FluxThicknessType: Center
FluxTracerType: Center
Tendencies:
ThicknessFluxTendencyEnable: false
PVTendencyEnable: false
KETendencyEnable: false
SSHTendencyEnable: false
VelDiffTendencyEnable: false
ViscDel2: 1.0e3
VelHyperDiffTendencyEnable: false
ViscDel4: 1.2e11
TracerHorzAdvTendencyEnable: true
TracerDiffTendencyEnable: false
EddyDiff2: 10.0
TracerHyperDiffTendencyEnable: false
EddyDiff4: 0.0
UseCustomTendency: false
ManufacturedSolutionTendency: false
Tracers:
Base: [Temperature, Salinity]
Debug: [Debug1, Debug2, Debug3]
ManufacturedSolution:
WavelengthX: 5.0e6
WavelengthY: 4.33013e6
Amplitude: 1.0
IOStreams:
# InitialState should only be used when starting from scratch.
# For restart runs, the frequency units should be changed from
# "OnStartup" to "never" so that the initial state file is not read.
InitialState:
UsePointerFile: false
Filename: init.nc
Mode: read
Precision: double
Freq: 1
FreqUnits: OnStartup
UseStartEnd: false
Contents:
- State
- Tracers
History:
UsePointerFile: false
Filename: output.nc
Mode: write
IfExists: append
Precision: double
Freq: 2073600
FreqUnits: seconds
UseStartEnd: false
Contents:
- Tracers
- LayerThickness
- NormalVelocity
I am seeing these errors:
PIO: WARNING: Opening file (output.nc) with iotype=3 (PIO_IOTYPE_NETCDF4C) failed (ierr=2, No such file or directory). Retrying with iotype=PIO_IOTYPE_NETCDF
...
PIO: FATAL ERROR: Aborting... FATAL ERROR: No such file or directory (file = output.nc) (/home/ac.xylar/e3sm_work/omega/omega/io-time-slice/externals/scorpio/src/clib/pioc_support.c: 5090)
It seems like maybe the file is getting opened in append mode even if it doesn't exist, and that PIO is unhappy with that? Or am I doing something weird?
| - Append if you want to append (eg multiple time slices) to the existing | ||
| file (this option is not currently supported). | ||
| file |
There was a problem hiding this comment.
@philipwjones, could you clarify (here or elsewhere) a bit more about what happens when you choose append? In particular, if the the model goes back in time (e.g. as a result of a restart), will time entries that already exist in the file be skipped or overwritten (or, heaven forbid, appended!) when Omega attempts to rewrite the same time entry?
In MPAS-Ocean, the clobber model refers to specific time entries, not to files. So the equivalent of replace would mean overwriting time entries as they are reached, and append would mean skipping existing time entries and only adding new ones. I am not saying that this is what Omega should do, just that we want to be clear about what Omega does do. At the moment, it seems like Omega likely does only one or the other of the MPAS-Ocean behaviors. If that is overwriting time entries even in append mode, I think that's okay. If someone changes how the model runs, the output will get replaced with the updated version. But if time entries get skipped in append mode, that might be an unexpected behavior for users fiddling around with the yaml file. They could probably get the intended behavior by deleting files, though. So again, just a question of documenting what the behavior is.
There was a problem hiding this comment.
Currently, append is just an indicator that the file may already exist and that we should write to the existing file if it exists and if not, create a new one. It was really only meant for multi-slice files (for single-slice files, I think we want to replace typically). The intent for multi-slice files was to overwrite any time slices that already existed in the file if it's re-running the same interval. I will clarify in the docs. As I was debugging Hyun's problem, I found that I was only computing the frame/slice correctly for some frequency options (intervals that didn't depend on calendar like seconds, minutes, etc) so trying to fix that now for the calendar intervals.
There was a problem hiding this comment.
Okay, sounds good. That's the behavior I would like us to have.
| - **FileFreq:** An optional entry for including multiple time slices in a | ||
| single file (only supported for output presently) | ||
| - **FreqUnits:** A field that, combined with the integer frequency, | ||
| determines the frequency of new files when multiple time slices are | ||
| included in a file. The file frequency must be less than or equal to | ||
| the data frequency. Acceptable values include all of the frequency values | ||
| listed above except the one-time values (eg OnStartup/Shutdown or AtTime). | ||
| The filename template should reflect the frequency and the time used for | ||
| the file will correspond to the template at the time the file is first | ||
| opened. |
There was a problem hiding this comment.
What should these be set to if you just want all time entries to go to a single file? Should they be omitted? Or set to a very large frequency?
There was a problem hiding this comment.
Does Omega write contents to these files on startup (before any time steps have run)? Do we need an option for specifying whether to write on startup or not?
There was a problem hiding this comment.
@xylar yes, if you want all entries in a single file, you'll need a file frequency that's very large. And no, we currently don't have a write on startup option, though you could kludge it by defining a separate stream with freq OnStartup that writes to the same file. Can you remind me what the use case is for writing on startup? Would you just need contents and metadata? Or would you actually want a full write?
There was a problem hiding this comment.
In MPAS-Ocean, it has been pretty standard to write out history files that include the initial state. This is particularly important for diagnostic variables that aren't part of an initial condition or restart file.
But we can cross that bridge when we come to it. I think there may be a few places in Polaris where we assume that the first output for test cases is the initial condition. We'll have to read the initial condition instead, but that's easy.
There was a problem hiding this comment.
@xylar yes, if you want all entries in a single file, you'll need a file frequency that's very large.
Is the fact that I didn't have FileFreq and FileFreqUnits perhaps why I got the errors I reported above? I can try adding those and running again.
There was a problem hiding this comment.
Possibly, but there may be other bugs too. Worth a try.
And on the startup thing, it's not a huge deal to add since I already have the OnStartup flag internally. But it'll probably require the explicit flag and then not using it as freq units - requiring a bit of change in interpretation for one-time read/writes. Maybe it's clearer that way anyway.
There was a problem hiding this comment.
Don't feel like you have to include that change here. It might be something we need down the road so it sounds good to keep it in mind.
|
Yes to following CF conventions in output which uses "time". |
| @@ -114,6 +116,16 @@ a template can be: | |||
| - Hours for a frequency every Freq hours (*not* Freq times per hour) | |||
| - Minutes for a frequency every Freq minutes (*not* Freq times per minute) | |||
| - Seconds for a frequency every Freq seconds (*not* Freq times per seconds) | |||
| - **FileFreq:** An optional entry for including multiple time slices in a | |||
| single file (only supported for output presently) | |||
| - **FreqUnits:** A field that, combined with the integer frequency, | |||
There was a problem hiding this comment.
| - **FreqUnits:** A field that, combined with the integer frequency, | |
| - **FileFreqUnits:** A field that, combined with the integer frequency, |
|
@philipwjones, I'm still getting the My test is in: if you need a reproducer. |
The append option needs to know whether a file exists or not to use either the create or open function.
This change allows developers to specify that a field (eg time) needs to retain their full precision even when included in an IOStream that requests reduced precision.
- fixed frame calculation for multi-slice files so that it works for
calendar intervals like months or years
- fixes issues when overwriting existing frames or files in append mode
|
Ok @xylar and @hyungyukang I believe I have fixed all the issues that you reported and this should be ready to review again. The prior version was not correctly opening and over-writing files and slices when in append mode, so I have fixed those. And the prior version was not computing the frames in a multi-slice file correctly for all time intervals. That has now been fixed. |
|
@philipwjones , thanks a lot. I just did some quick tests with the manufactured solution test, and now it is working properly! I am going to test more under several circumstances, but I have a quick question: The time-slice capability only works with I believe it would be beneficial for it to support |
|
@hyungyukang Time slices only work with append, otherwise you would lose all prior time slices when the file was replaced. Having said that, you shouldn't need to manually remove existing files. It should either replace files (under the replace option) or over-write time slices within an existing file (under the append option). I tested that by running Ctest twice without clearing the directory and it worked fine for me. What is your setup that's causing an error? |
@philipwjones , I'm running the manufactured solution test. You can retrieve my setup by following this (building processes are the same): |
There was a problem hiding this comment.
@philipwjones, I really appreciate you working so hard on this an iterating with us.
In my testing with Polaris on Chrysalis, things are working as expected. I needed to add the following to history streams that use a single output file (output.nc):
IfExists: append
# effectively never
FileFreq: 9999
FileFreqUnits: years
When I do that, I can successfully output either every 10 years or every 1 year in a 10-year run, which the expected results. (I tried switching from outputting every 10 years to every 1 year without deleting the file and got a a critical error, which was very easy to understand.)
|
I will note that I saw a transient error: The |
|
@hyungyukang In your case, the issue is that you're (correctly) using a pointer file for restarts. This is fine for normal restarts, but if you're trying to re-run an interval, you have to edit/delete the pointer file or else it will try to extend the run from the last restart written. @xylar Yes, the behavior trying to switch output intervals is expected - if it finds a different interval, it doesn't know what it should do so bails with an error. The other error you encountered means that it couldn't find the time dimension while trying to read and if the file was invalid, then something might be amiss there. |
|
Actually @hyungyukang , maybe I'm misunderstanding what you're running. Was your second run supposed to extend from restart? In that case, you do have to modify the input yml file from an initial state read to a restart read (as noted in the comments in the yml file). |
|
@hyungyukang Sorry, being stupid this morning. Ignore the previous comments... trying to reproduce now. |
@philipwjones , no problem! Thank you for taking a look! 😃 |
fixed a bug in which string metadata was being read but had not been allocated with a large enough size and needed a call to get the length first
|
@hyungyukang There was indeed a bug - I had added a metadata read check to see if metadata already existed with a proper value, but for the string read, the string was not allocated with the proper length. Should be fixed now - worked for me anyway @xylar It appears there is an issue with the conda environment for PR tests - see the error from the CI test. This PR satisfied the pre-commit checks that I ran manually so should be ok. |
|
@philipwjones, yeah, there's a known issue causing these intermittent failures. Hopefully, it will be fixed soon. A rerun nearly always takes care of it. |
There was a problem hiding this comment.
@philipwjones , I really appreciate your hard work on this. I have tested this PR with the manufactured solution test under various conditions by changing I/O related options in omega.yml. In all scenarios, I/O works as expected. I was also able to visualize SSH of the manufactured solution test using an output file containing 96 time levels. Thanks again!
|
Thanks @xylar and @hyungyukang for the testing. Glad it's all finally working. Wish I could say this is the last time I need to touch streams... |
With these changes, users can write multiple time slices of a field to a given file. This is specified through the stream input YAML configuration by adding a file frequency in addition to the data frequency. Currently this only applies to output streams. While input multi-frame reads are supported at the base IO level, I am waiting for forcing designs to determine how best to support multiple slices in an input file. Unit tests for reading/writing multiple slices from a file have been added for the base IO level and all unit tests pass. I have modified the default YAML config to write 10-day high-frequency output for two months in two monthly files and verified that correct times have been written for the time field. The user and developer docs have been updated to describe how to configure and use this feature.
Checklist