Initial implementation #1
Conversation
There was a problem hiding this comment.
@xylar @chengzhuzhang I have more work to do, but I think I have the main idea of the refactor captured in this pull request and the corresponding E3SM-Project/zppy#642.
I should stress I haven't run this code yet. I still need to get the environments sorted out to be able to call zppy-interfaces from zppy. That is, while I think the main idea is captured here, there is still polishing/testing work left. However, I wanted to tag you here to allow for any further design input early on.
Specifically, my remaining action items:
- Create a
condadirectory and setup the yaml/toml files necessary to produce a dev environment. - Use
zppyto generate stand-alonetsandmpas_analysisoutput. - Run
global-time-seriesinterface in the dev environment on that output. - Run the
global_time_seriestask in zppy (i.e., make surezppycan correctly callzppy-interfaces). - Create a
tests/directory and add some integration tests.zppyshould simply test that it can invokezppy-interfaces global-time-series.zppy-interfaceshowever should test that Global Time Series plots actually generate correctly. As for unit tests, Create global time series Viewers zppy#616 will do that once it's ported to this repo. - Add pre-commit checks
- Add documentation for this repo/package.
zppy-interfaces/__main__.py
Outdated
| if args.interface == "global_time_series": | ||
| global_time_series.global_time_series() |
There was a problem hiding this comment.
I'm following zstash's design here: have one arg parser for the package as a whole (e.g., https://github.com/E3SM-Project/zstash/blob/main/zstash/main.py#L34) and one each for the individual utilities [commands in zstash or interfaces in this package] (e.g., https://github.com/E3SM-Project/zstash/blob/main/zstash/create.py#L103)
The result is that calling the global time series code would be done via:
zppy-interfaces global-time-series <args>
There was a problem hiding this comment.
@forsyth2, this seems like a bit of a clunky interface to me. Since the tools don't have much to unify them, I would just given them individual entry points. You could give them a unique prefix like zi-global-time-series if you like.
zstash is a coherent package with clear subcommands similar to git or conda so it makes sense to use this kind of an approach there but I think zppy-interfaces doesn't clearly benefit in the same way.
There was a problem hiding this comment.
This is obviously just my opinion. You can stick with the zppy-intefaces main command with subcommands if you like.
There was a problem hiding this comment.
@xylar If we do completely separate commands, would it make more sense to have distinct dependency requirement lists too? (As opposed to a single dev.yml for all of zppy-interfaces). I'm not sure if that was something we had decided on.
There was a problem hiding this comment.
I could see several utilities being useful to multiple interfaces -- for example, the OutputViewer class for Global Time Series that E3SM-Project/zppy#616 will introduce will also be useful for adding Viewers to the PMP task in E3SM-Project/zppy#640. It follows that it may be useful to have a single dev environment for all of zppy-interfaces.
There was a problem hiding this comment.
@xylar If we do completely separate commands, would it make more sense to have distinct dependency requirement lists too? (As opposed to a single
dev.ymlfor all ofzppy-interfaces). I'm not sure if that was something we had decided on.
No, I would just have a single set of dependencies that covers the full package (all commands and any public functions, etc.).
There was a problem hiding this comment.
I could see several utilities being useful to multiple interfaces -- for example, the
OutputViewerclass for Global Time Series that E3SM-Project/zppy#616 will introduce will also be useful for adding Viewers to the PMP task in E3SM-Project/zppy#640. It follows that it may be useful to have a single dev environment for all ofzppy-interfaces.
Yes, and there should be a place for a common framework that includes things like this.
|
|
||
| from typing import Dict | ||
|
|
||
| def main(): |
There was a problem hiding this comment.
This function actually replaces much of the code in the global_time_series.bash template, bringing us closer to the goal of the bash templates being purely for launching jobs (rather than doing setup work).
There was a problem hiding this comment.
Related to my comment above, I would just make this function the entry point for a zi-global-time-series command.
|
|
||
| def _get_args() -> argparse.Namespace: | ||
| # Parser | ||
| parser: argparse.ArgumentParser = argparse.ArgumentParser( |
There was a problem hiding this comment.
global_time_series requires a large number of parameters. A cfg like e3sm_diags has may prove more useful than a bunch of command line arguments. I.e., in https://github.com/E3SM-Project/zppy/blob/main/zppy/templates/e3sm_diags.bash:
command="srun -n 1 python -u e3sm.py -d e3sm_diags.cfg"
There was a problem hiding this comment.
There can be benefits in supporting both approaches. All flags could be optional but they would override config options if provided. Similarly, all user (or zppy-provided) config options could be optional but they would override defaults. This approach would be attractive for standalone runs but maybe could become confusing through zppy so maybe for that reason it's safer to use one or the other approach -- config options or command-line flags.
| "plots_ocn": args.plots_ocn, | ||
| "regions": args.regions, | ||
| } | ||
| coupled_global.coupled_global(parameters) |
There was a problem hiding this comment.
E3SM-Project/zppy#616 itself refactors the coupled_global.py code, in addition to adding a number of unit tests. So, that pull request's code will just be moved over to this repo after this pull request merges.
|
I made a few suggestions but this is looking great so far! you'll need some more "glue" like |
|
Thanks @xylar! |
| import unittest | ||
| from typing import Any, Dict, List | ||
|
|
||
| from zppy_interfaces.global_time_series.coupled_global import ( |
There was a problem hiding this comment.
Need to figure out how to import the code properly to the tests.
I've moved many of the tests and the modularized/refactored functions from E3SM-Project/zppy#616 directly into this pull request. Specifically I've moved the changes that are more general refactoring and not specific to Land support or Viewer generation.
| # TODO: Fails here when atmosphere_only="false" | ||
| set_var( | ||
| exp, "ocean", requested_variables.vars_ocn, valid_vars, invalid_vars, rgn | ||
| ) |
There was a problem hiding this comment.
Of the original 8 plots, I can generate the atmosphere plots, but if I try to generate the ocean plots too I get ValueError: Could not find any dimension coordinates to use to order the datasets for concatenation.
I got this error before and after adding the refactored functions of E3SM-Project/zppy#616, so I'm confident those changes didn't affect this.
There was a problem hiding this comment.
(I'm waiting for nodes to run zppy's main branch with this configuration to see if something broke there or if the problem is specific to this PR).
There was a problem hiding this comment.
Can you provide the full stack trace for the error? It presumably comes from a call to xarray.concat() and it may be that you need to explicitly specify dim='Time' or something like that. You may also need to have other arguments. That's where I'd need to see the specifics of the call. Ocean input files don't have a CF-compliant time coordinate, which causes issues. (Omega will fix this...)
| ############################################################################### | ||
|
|
||
|
|
||
| def main(parameters=None): |
There was a problem hiding this comment.
Naming and organization:
mypy gave me zppy-interfaces is not a valid Python package name. I changed the directory names to use underscores: zppy_interfaces and global_time_series, and then all the pre-commit checks passed.
3 further questions:
- Should this file be named
zppy_interfaces/global_time_series/global_time_series.py[current] orzppy_interfaces/global_time_series/__main__.py? - Is this function stylistically correctly named
main? - Should we organize interfaces together with their tests or with each other? I.e. which of these:
zppy_interfaces/global_time_seriesandtests/global_time_series[current implementation, aside from dashes in name currently]global_time_series/srcandglobal_time_series/tests
There was a problem hiding this comment.
Correct, python cannot have dashes in any of its file names.
There was a problem hiding this comment.
Should this file be named zppy_interfaces/global_time_series/global_time_series.py [current] or zppy_interfaces/global_time_series/main.py?
It should be zppy_interfaces/global_time_series/__init__.py. __main__.py is just for the entry point main() and supporting private functions.
There was a problem hiding this comment.
- tests should be outside of the
zppy_interfacespackage so your first suggestion is far better.
There was a problem hiding this comment.
My mistake on 1. I took a closer look and everything you have does belong in __main__.py so leave it as you have it.
| coupled_global.coupled_global(parameters) | ||
|
|
||
|
|
||
| # TODO: replace command line arguments with _get_cfg_parameters, like https://github.com/E3SM-Project/e3sm_diags/blob/main/e3sm_diags/parser/core_parser.py#L809 |
There was a problem hiding this comment.
I do think a cfg would be cleaner, but I want to get this all working properly first before spending time on that.
conda/dev.yml
Outdated
| @@ -8,12 +8,12 @@ dependencies: | |||
| # ================= | |||
| - python >=3.9 | |||
| - pip | |||
| - matplotlib-base | |||
| - mpas_tools | |||
| - matplotlib-base # TODO: Find constraint | |||
There was a problem hiding this comment.
If you know nothing else, I would use the versions pinned in E3SM-Unified as the lower bounds and not have an upper bound. However, with matplotlib, it might be best to have an upper bound for safety, since there are frequent, breaking changes:
https://github.com/MPAS-Dev/geometric_features/blob/93f71ba01d0722de7bfb1b9ec3e9129b859b8963/feature_creation_scripts/ismip6_landice_basins/basin_masks_to_geojson_regions.py#L116
| - matplotlib-base # TODO: Find constraint | |
| - matplotlib-base >=3.8.2,<3.10 |
The current version is 3.9.2 so this would be allowed, but 3.10 is under development and we could test it out before officially loosening the constraints to allow it in a future release.
conda/dev.yml
Outdated
| - netcdf4 | ||
| - numpy >=2.0,<3.0 | ||
| - xarray >=2023.02.0 | ||
| - xcdat | ||
| - xcdat # TODO: Find constraint |
There was a problem hiding this comment.
| - xcdat # TODO: Find constraint | |
| - xcdat >=0.7.0,<1.0 |
Again, the lower bound is the version pinned in the last E3SM-Unified, the upper bound is the next major version, assuming that would likely have breaking changes.
@tomvothecoder, should this be tighter, e.g. 0.8.0? Often, minor versions have breaking changes for packages before the 1.0.0 release. Is that true of xcdat?
There was a problem hiding this comment.
xCDAT is becoming more stable with each new release. If there are breaking changes, we usually have deprecation warnings. However, I do think it is a good idea to use xcdat >=0.7.0,<1.0 until a 1.0 is officially released.
| @@ -0,0 +1,54 @@ | |||
| [flake8] | |||
There was a problem hiding this comment.
Most of this should move to pyproject.toml. See https://github.com/E3SM-Project/mache/blob/main/pyproject.toml
Frustratingly flake8 refuses to support this format so it has to move to a .flake8 file:
https://github.com/E3SM-Project/mache/blob/main/.flake8
There was a problem hiding this comment.
I moved to ruff for code formatting and linting in xCDAT, which removed the need for the setup.cfg file just for flake8. Reduces tool count and simplifies configs.
pyproject.toml
Outdated
| # This config file is required for Black. | ||
| # Unforunately, black does not support setup.cfg (refer to link). | ||
| # https://github.com/psf/black/issues/683#issuecomment-490236406 |
There was a problem hiding this comment.
This can be removed, because we don't want to use a setup.cfg anymore anyway.
pyproject.toml
Outdated
| "importlib_resources", | ||
| "jinja2", | ||
| "lxml", | ||
| "pyyaml", | ||
| "progressbar2", |
There was a problem hiding this comment.
These need to be the same as the base in dev.yml
There was a problem hiding this comment.
These are all the dependencies:
dependencies = [
"matplotlib-base >=3.8.2,<3.10",
"netcdf4",
"numpy >=2.0,<3.0",
"xarray >=2023.02.0",
"xcdat >=0.7.0,<1.0",
]
[project.optional-dependencies]
testing = [
"pytest",
"pytest-cov",
]
docs = [
"sphinx",
"sphinx_rtd_theme",
"sphinx-multiversion",
]
qa = [
"black=23.9.1",
"flake8=6.1.0",
"flake8-isort=6.1.0",
"isort=5.12.0",
"mypy=1.5.1",
"pre-commit >=3.0.0",
"types-PyYAML >=6.0.0",
]
dev = [
"tbump=6.9.0",
"ipykernel",
]There was a problem hiding this comment.
Hmm, changing this seems to break my pip install .: configuration error: "project.optional-dependencies.qa[0] must be pep508"
Stack trace
Processing /gpfs/fs1/home/ac.forsyth2/ez/zppy-interfaces
Installing build dependencies ... done
Getting requirements to build wheel ... error
error: subprocess-exited-with-error
× Getting requirements to build wheel did not run successfully.
│ exit code: 1
╰─> [53 lines of output]
configuration error: `project.optional-dependencies.qa[0]` must be pep508
DESCRIPTION:
Project dependency specification according to PEP 508
GIVEN VALUE:
"black=23.9.1"
OFFENDING RULE: 'format'
DEFINITION:
{
"$id": "#/definitions/dependency",
"title": "Dependency",
"type": "string",
"format": "pep508"
}
For more details about `format` see
https://validate-pyproject.readthedocs.io/en/latest/api/validate_pyproject.formats.html
Traceback (most recent call last):
File "/home/ac.forsyth2/miniconda3/envs/zppy-interfaces-dev-20241104/lib/python3.12/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 353, in <module>
main()
File "/home/ac.forsyth2/miniconda3/envs/zppy-interfaces-dev-20241104/lib/python3.12/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 335, in main
json_out['return_val'] = hook(**hook_input['kwargs'])
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/ac.forsyth2/miniconda3/envs/zppy-interfaces-dev-20241104/lib/python3.12/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 118, in get_requires_for_build_wheel
return hook(config_settings)
^^^^^^^^^^^^^^^^^^^^^
File "/tmp/pip-build-env-iekkrxzx/overlay/lib/python3.12/site-packages/setuptools/build_meta.py", line 333, in get_requires_for_build_wheel
return self._get_build_requires(config_settings, requirements=[])
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/tmp/pip-build-env-iekkrxzx/overlay/lib/python3.12/site-packages/setuptools/build_meta.py", line 303, in _get_build_requires
self.run_setup()
File "/tmp/pip-build-env-iekkrxzx/overlay/lib/python3.12/site-packages/setuptools/build_meta.py", line 319, in run_setup
exec(code, locals())
File "<string>", line 1, in <module>
File "/tmp/pip-build-env-iekkrxzx/overlay/lib/python3.12/site-packages/setuptools/__init__.py", line 117, in setup
return distutils.core.setup(**attrs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/tmp/pip-build-env-iekkrxzx/overlay/lib/python3.12/site-packages/setuptools/_distutils/core.py", line 157, in setup
dist.parse_config_files()
File "/tmp/pip-build-env-iekkrxzx/overlay/lib/python3.12/site-packages/setuptools/dist.py", line 655, in parse_config_files
pyprojecttoml.apply_configuration(self, filename, ignore_option_errors)
File "/tmp/pip-build-env-iekkrxzx/overlay/lib/python3.12/site-packages/setuptools/config/pyprojecttoml.py", line 71, in apply_configuration
config = read_configuration(filepath, True, ignore_option_errors, dist)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/tmp/pip-build-env-iekkrxzx/overlay/lib/python3.12/site-packages/setuptools/config/pyprojecttoml.py", line 139, in read_configuration
validate(subset, filepath)
File "/tmp/pip-build-env-iekkrxzx/overlay/lib/python3.12/site-packages/setuptools/config/pyprojecttoml.py", line 60, in validate
raise ValueError(f"{error}\n{summary}") from None
ValueError: invalid pyproject.toml config: `project.optional-dependencies.qa[0]`.
configuration error: `project.optional-dependencies.qa[0]` must be pep508
[end of output]
note: This error originates from a subprocess, and is likely not a problem with pip.
error: subprocess-exited-with-error
× Getting requirements to build wheel did not run successfully.
│ exit code: 1
╰─> See above for output.
note: This error originates from a subprocess, and is likely not a problem with pip.
There was a problem hiding this comment.
If that doesn't work comment out qa for now.
There was a problem hiding this comment.
@forsyth2 I think it might be mad that the black version is being compared with a single equals sign instead of a double equals. We don't have a comparison like that in mache so I haven't needed to format it for the toml file yet, but that makes sense. Could you change the last two lists to these and see how it works?
qa = [
"black==23.9.1",
"flake8==6.1.0",
"flake8-isort==6.1.0",
"isort==5.12.0",
"mypy==1.5.1",
"pre-commit >=3.0.0",
"types-PyYAML >=6.0.0",
]
dev = [
"tbump==6.9.0",
"ipykernel",
]
| ] | ||
|
|
||
| dependencies = [ | ||
| "matplotlib-base >=3.8.2,<3.10", |
There was a problem hiding this comment.
This toml update seems to be causing an issue.
$ pip install .
Processing /gpfs/fs1/home/ac.forsyth2/ez/zppy-interfaces
Installing build dependencies ... done
Getting requirements to build wheel ... done
Preparing metadata (pyproject.toml) ... done
INFO: pip is looking at multiple versions of zppy-interfaces to determine which version is compatible with other requirements. This could take a while.
ERROR: Could not find a version that satisfies the requirement matplotlib-base<3.10,>=3.8.2 (from zppy-interfaces) (from versions: none)
ERROR: No matching distribution found for matplotlib-base<3.10,>=3.8.2
There was a problem hiding this comment.
@altheaden Do you have an idea of what's going on here? No matter how I edit this line I can't seem to find something that pip will take.
There was a problem hiding this comment.
It then gives a similar error about xcdat if I just remove matplotlib-base entirely.
There was a problem hiding this comment.
@forsyth2 Sorry. You should change this to just matplotlib without base here. The matplotlib-base package doesn't exist on PyPI (packages listed in the pyproject.toml), just on conda-forge (the packages listed in dev.yml).
There was a problem hiding this comment.
But do leave matplotlib-base in the dev.yml
There was a problem hiding this comment.
| "matplotlib-base >=3.8.2,<3.10", | |
| "matplotlib >=3.8.2,<3.10", |
|
|
||
|
|
||
| # TODO: replace command line arguments with _get_cfg_parameters, like https://github.com/E3SM-Project/e3sm_diags/blob/main/e3sm_diags/parser/core_parser.py#L809 | ||
| def _get_args() -> Parameters: |
There was a problem hiding this comment.
The zppy side doesn't seem to be passing in arguments correctly.
The bash template, when filled out, shows:
zi-global-time-series --use_ocn True --global_ts_dir /lcrc/group/e3sm/ac.forsyth2/zppy_min_case_global_time_series_\
original_8_output/test-642-working-env-20241105/v3.LR.historical_0051/post/scripts/global_time_series_1985-1995_dir\
--input /lcrc/group/e3sm2/ac.wlin/E3SMv3/v3.LR.historical_0051 --input_subdir archive/ocn/hist --moc_file mocTimeS\
eries_1985-1995.nc --case_dir /lcrc/group/e3sm/ac.forsyth2/zppy_min_case_global_time_series_original_8_output/test-\
642-working-env-20241105/v3.LR.historical_0051 --experiment_name v3.LR.historical_0051 --figstr v3.LR.historical_00\
51 --color Blue --ts_num_years 5 --plots_original net_toa_flux_restom,global_surface_air_temperature,toa_radiation,\
net_atm_energy_imbalance,change_ohc,max_moc,change_sea_level,net_atm_water_imbalance --atmosphere_only False --plot\
s_atm None --plots_ice None --plots_lnd None --plots_ocn None --regions glb,n,s --results_dir ${results_dir} --star\
t_yr 1985 --end_yr 1995
But /lcrc/group/e3sm/ac.forsyth2/zppy_min_case_global_time_series_original_8_output/test-642-working-env-20241105/v3.LR.historical_0051/post/scripts/global_time_series_1985-1995.o622622 shows:
zi-global-time-series: error: the following arguments are required: end_yr
If I try to enter end_yr manually on the zppy_interfaces side, it then raises an error about start_yr. So clearly the arguments aren't getting passed in properly.
There was a problem hiding this comment.
It looks like you have a lot of \ line dividers in the middle of expressions. I think that is going to cause giant problems. You can only have \ between flags, not in the middle of them.
| # TODO: run this print statement here and in zppy main, and compare. | ||
| # TODO: double check we have the same xcdat version in the 2 cases. | ||
| # TODO: can we open these 3 folders from a tiny script? | ||
| # TODO: put in file names manually rather than using `*`? | ||
| print(f"error is here: {directory}*.nc") |
There was a problem hiding this comment.
I still need to debug why the ocean plots are getting stuck here but not in pre-split zppy.
There was a problem hiding this comment.
$ conda activate zppy-interfaces-dev-20241104
$ cd /lcrc/group/e3sm/ac.forsyth2/zppy_min_case_global_time_series_setup_only_output/test-642-2024-1105/v3.LR.historical_0051/post/ocn/glb/ts/monthly/5yr
$ ls
mocTimeSeries_1985-1995.nc mpaso.glb.198501-198912.nc mpaso.glb.199001-199412.nc
$ python
>>> import xcdat
>>> xcdat.open_mfdataset("*.nc", center_times=True)
# ValueError: Could not find any dimension coordinates to use to order the datasets for concatenation
$ source /lcrc/soft/climate/e3sm-unified/load_latest_e3sm_unified_chrysalis.sh
$ cd /lcrc/group/e3sm/ac.forsyth2/zppy_min_case_global_time_series_original_8_output/test-main-20241105/v3.LR.historical_0051/post/ocn/glb/ts/monthly/5yr/
$ ls
mocTimeSeries_1985-1995.nc mpaso.glb.198501-198912.nc mpaso.glb.199001-199412.nc
$ python
>>> import xcdat
>>> xcdat.open_mfdataset("*.nc", center_times=True)
# Successful
There was a problem hiding this comment.
Respectively:
>>> xcdat.__version__
'0.6.0'
>>> xcdat.__version__
'0.7.0'
Looks like it's the xcdat version.
There was a problem hiding this comment.
Since we're setting "xcdat >=0.7.0,<1.0",, this problem should get resolved.
There was a problem hiding this comment.
FYI I will be releasing xCDAT v0.7.3 soon which sets numpy >=2.0.0. I will let you know soon so you can set that as the min version here.
There was a problem hiding this comment.
Ah, yes, the specs here are probably currently unsovlable. @forsyth2, you probably got xcdat 0.6.0 before because that's before @tomvothecoder put an upper bound on numpy, so it can technically be installed with numpy 2.0 even though it won't actually work. (We could patch things on conda-forge to add the missing upper bound on numpy but probably not worth it.)
| "netcdf4", | ||
| "numpy >=2.0,<3.0", | ||
| "xarray >=2023.02.0", | ||
| "xcdat >=0.7.0,<1.0", |
There was a problem hiding this comment.
| "xcdat >=0.7.0,<1.0", | |
| "xcdat >=0.7.3,<1.0", |
xcdat v0.7.3 is now available on conda-forge with the same numpy 2.0 constraint: https://anaconda.org/conda-forge/xcdat
| - netcdf4 | ||
| - numpy >=2.0,<3.0 | ||
| - xarray >=2023.02.0 | ||
| - xcdat >=0.7.0,<1.0 |
There was a problem hiding this comment.
| - xcdat >=0.7.0,<1.0 | |
| - xcdat >=0.7.3,<1.0 |
xcdat v0.7.3 is now available on conda-forge with the same numpy 2.0 constraint: https://anaconda.org/conda-forge/xcdat
|
@forsyth2, haven't heard anything further on this. Has this just had to move to the back burner or are you stuck anywhere that @altheaden, @tomvothecoder or I could help with? |
@xylar Yes, competing priorities at the moment unfortunately. Hoping to get back to this sometime this week. |
Initial implementation for
zppy-interfaces. Resolves discussion at E3SM-Project/zppy#641. Implements E3SM-Project/zppy#398.This pull request should be reviewed in conjunction with E3SM-Project/zppy#642.