Skip to content
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

cmor tool improvements #205

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

ilaflott
Copy link
Member

@ilaflott ilaflott commented Oct 11, 2024

  • I ran my code
  • I tried to make my code readable
  • I tried to comment my code
  • I wrote a new test, if applicable
  • I wrote new instructions/documentation, if applicable
  • I ran pytest and inspected it's output
  • I ran pylint and attempted to implement some of it's feedback

This PR...

  • overhauls the README.md with fre-cli oriented usage and removes outdated doc
  • refactors cmor_mixer.py heavily 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 routine
  • functions now have doc strings with argument descriptions in most places
  • better run-time feedback of functioning/IO/logic etc during runtime, hopefully not too much
  • better comments where possible
  • new test case where key:value pair in var_list argument differ
  • more test case cleanup/setup, more specific test case success conditions
  • new CLI/click.testing.CliRunner test for the new functionality of the fre.cmor module
  • change of the way the click module interfaces with the cmor run subtool module

@ilaflott ilaflott self-assigned this Oct 11, 2024
@ilaflott ilaflott linked an issue Oct 11, 2024 that may be closed by this pull request
…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
@ilaflott ilaflott marked this pull request as ready for review October 22, 2024 19:40
fre/cmor/README.md Outdated Show resolved Hide resolved
fre/cmor/README.md Outdated Show resolved Hide resolved
fre/cmor/README.md Outdated Show resolved Hide resolved
fre/cmor/frecmor.py Outdated Show resolved Hide resolved
fre/cmor/cmor_mixer.py Outdated Show resolved Hide resolved
fre/cmor/cmor_mixer.py Outdated Show resolved Hide resolved
fre/cmor/cmor_mixer.py Outdated Show resolved Hide resolved
fre/cmor/cmor_mixer.py Outdated Show resolved Hide resolved
fre/cmor/cmor_mixer.py Outdated Show resolved Hide resolved
fre/cmor/cmor_mixer.py Outdated Show resolved Hide resolved
fre/cmor/cmor_mixer.py Outdated Show resolved Hide resolved
Comment on lines +153 to +173
# 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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ===============================================================================

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another reason to leave it unresolved- i suspect this is cmor's fault...

fre/cmor/cmor_mixer.py Outdated Show resolved Hide resolved
@ilaflott
Copy link
Member Author

this will close #204

@ilaflott
Copy link
Member Author

ilaflott commented Oct 23, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve fre cmor run functionality
1 participant