cmor tool improvements #205
Conversation
…change test succeess condition to reflect WHAT WE WANT and NOT what is currently happening...
…cmor_mixer, refactor the old function netcdf_var, but most importantly, RENAMING THINGS for readability. push all input netCDF4.Dataset reading for that function as close to the start as possible in most cases. refactor vertical dimension logic. several new helper functions to enhance logic parsing of the bulk routines.
…s, and ignore incoming cmor module warnings thrown by pylint that we cannot prevent nor control
…what kinds of objects are passed to which function, etc. theres definitely some easy confusion regarding the key/value pair in varlist
…nto functions, delving into data movement finally a bit. additioanl test case for both the modular functionality of the python package, and the cli calls. probably not in a completley working state, but want to jot a note before i get even more ahead of myself.
…p command line options the same while adjusting python module argument names, switch from context forward to context invoke to map the arg names
…by click.echo of result.stderr to remind myself why thats commented out in the first place. not capd sep err
…hen trying to remove tmp dir or outdir...), the output files ARE still being recreated at least. better control over output directories now as well.
… when they cleave scope. netcdf4 datasets explicitly closed now too. removed useless cmor.close() call. remove outdated os.system calls and replace with subprocess which... cant quite believe im saying it, but is preferable to this way of making system calls. add absolutely dreadful absolute-path resolving right before rewrite_netcdf_var... but it does get cmor to do its temporary file processing in the right spot
| # VERY ANNOYING !!! FYI WARNING TODO | ||
| if Path(TMPDIR).exists(): | ||
| try: | ||
| shutil.rmtree(TMPDIR) | ||
| except OSError as exc: | ||
| print(f'WARNING: TMPDIR={TMPDIR} could not be removed.') | ||
| print( ' this does not matter that much, but is unfortunate.') | ||
| print( ' supicion: something the cmor module is using is not being closed') | ||
|
|
||
| #assert not Path(TMPDIR).exists() # VERY ANNOYING !!! FYI WARNING TODO | ||
|
|
||
| # VERY ANNOYING !!! FYI WARNING TODO | ||
| if Path(OUTDIR).exists(): | ||
| try: | ||
| shutil.rmtree(OUTDIR) | ||
| except OSError as exc: | ||
| print(f'WARNING: OUTDIR={OUTDIR} could not be removed.') | ||
| print( ' this does not matter that much, but is unfortunate.') | ||
| print( ' supicion: something the cmor module is using is not being closed') | ||
|
|
||
| #assert not Path(OUTDIR).exists() # VERY ANNOYING !!! FYI WARNING TODO |
There was a problem hiding this comment.
these cleanup steps throw some very annoying error messages if you don't let the exception go-
...
...
...
fre/cmor/tests/test_cmor_run_subtool.py::test_fre_cmor_run_subtool_case1_output_compare_metadata PASSED [ 55%]
fre/cmor/tests/test_cmor_run_subtool.py::test_setup_fre_cmor_run_subtool_case2 FAILED [ 66%]
======================================================================================== FAILURES ========================================================================================
_________________________________________________________________________ test_setup_fre_cmor_run_subtool_case2 __________________________________________________________________________
capfd = <_pytest.capture.CaptureFixture object at 0x2b0f0acad790>
def test_setup_fre_cmor_run_subtool_case2(capfd):
''' make a copy of the input file to the slightly different name.
checks for outputfile from prev pytest runs, removes it if it's present.
this routine also checks to make sure the desired input file is present'''
if Path(FULL_OUTPUTFILE).exists():
Path(FULL_OUTPUTFILE).unlink()
assert not Path(FULL_OUTPUTFILE).exists()
if Path(OUTDIR+'/CMIP6').exists():
shutil.rmtree(OUTDIR+'/CMIP6')
assert not Path(OUTDIR+'/CMIP6').exists()
# VERY ANNOYING !!! FYI WARNING TODO
if Path(TMPDIR).exists():
#try:
> shutil.rmtree(TMPDIR)
fre/cmor/tests/test_cmor_run_subtool.py:156:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../../conda/envs/fre_cli/lib/python3.9/shutil.py:734: in rmtree
_rmtree_safe_fd(fd, path, onerror)
../../conda/envs/fre_cli/lib/python3.9/shutil.py:667: in _rmtree_safe_fd
_rmtree_safe_fd(dirfd, fullname, onerror)
...
...
...
../../conda/envs/fre_cli/lib/python3.9/shutil.py:690: in _rmtree_safe_fd
onerror(os.unlink, fullname, sys.exc_info())
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
topfd = 28, path = 'fre/tests/test_files/outdir/tmp/CMIP6/CMIP6/ISMIP6/PCMDI/PCMDI-test-1-0/piControl-withism/r3i1p1f1/Omon/sos/gn/v20241022'
onerror = <function rmtree.<locals>.onerror at 0x2b0f0accbe50>
def _rmtree_safe_fd(topfd, path, onerror):
...
...
...
...
> os.unlink(entry.name, dir_fd=topfd)
E OSError: [Errno 16] Device or resource busy: '.nfs00000000a9f8735600000090'
../../conda/envs/fre_cli/lib/python3.9/shutil.py:688: OSError
================================================================================ short test summary info =================================================================================
FAILED fre/cmor/tests/test_cmor_run_subtool.py::test_setup_fre_cmor_run_subtool_case2 - OSError: [Errno 16] Device or resource busy: '.nfs00000000a9f8735600000090'
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
============================================================================== 1 failed, 5 passed in 2.65s ===============================================================================
There was a problem hiding this comment.
leaving this unresolved for now. the directory re-creation for the automatic test pipeline is a nice-to-have, not essential. as if you remove the directory by hand after erroring, and restart at this failed test, the test succeeds as expected.
largely that re-tests the directory creation that already succeeded in the prev test, so its even a tad redundant.
There was a problem hiding this comment.
another reason to leave it unresolved- i suspect this is cmor's fault...
|
this will close #204 |
… gimme the bugs and i squish em
…using the included test files are shown. dependency on external configs from PCMDI made clear
|
regarding meniton in NOAA-GFDL/CatalogBuilder#79 links in commit point to https://github.com/NOAA-GFDL/MDTF-diagnostics/tree/main/data/gfdl-cmor-tables note submodule dep: https://github.com/NOAA-GFDL/MDTF-diagnostics/tree/main/data/cmip6-cmor-tables comes from https://github.com/PCMDI/cmip6-cmor-tables |
ilaflott
added
bug
ceblanton
left a comment
ceblanton
left a comment
There was a problem hiding this comment.
Your code deserved a better review than what I did. I didn't look hard at the testing additions, but the rest looked great with the few comments I had. I don't think any of the comments are deal-breakers tho for taking this.
| print(f'(cmor_run_subtool) WARNING: local_var == {local_var} ' | ||
| f'!= {target_var} == target_var') | ||
| print(f'i am expecting {local_var} to be in the filename, and i expect the variable' | ||
| f' in that file to be {target_var}') |
There was a problem hiding this comment.
I think I'm following but disagree with the message here. Shouldn't it be:
I am expecting {local_var} to be in the filename, and I expect the variable in that file to be {local_var}. The output file will be named {target_var}.
There was a problem hiding this comment.
filenames are just filenames- they can be whatever people want! the variable in the filename doesnt have to match what is in that file.
I understood the var-list input to be key/value pairs where the key could be named var or varV2 or varFIX, etc so the model runner in mind has some flexibility. The value of that key should always be the variable that the MIP table wants/expects, and the cmor module sets the output filename based on that table
|
@ceblanton has good feedback here, most of which is actually not straightforward to address, or involves some creative decision making. given that this PR is gigantic but functional, i'm inclined to push that feedback forward for now, and spin those up into issues and/or dev efforts on the next PR #218 |
2 participants
This PR...
README.mdwithfre-clioriented usage and removes outdated doccmor_mixer.pyheavily to be more readable, have more modularity to it to clearly illustrate who-does-what and why, with an emphasis on clearly exposing data movement inherent in the routinekey:valuepair invar_listargument differclick.testing.CliRunnertest for the new functionality of thefre.cmormodule