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

Simplify conda environments #198

Merged
merged 2 commits into from
Jul 19, 2023
Merged

Simplify conda environments #198

merged 2 commits into from
Jul 19, 2023

Conversation

tomvothecoder
Copy link
Collaborator

@tomvothecoder tomvothecoder commented Jul 19, 2023

  • Loosen up ci.yml and dev.yml constraints to improve upgradeability
  • Remove conda-env/readthedocs.yml and use ci.yml instead in readthedocs.yml config
  • Update GitHub Actions build workflow to use mamba for building environment with proper caching
  • Fixes e2c v1.10.0rc1 with cmor 3.7.2 environment fails to build #197

- Loosen up constraints to improve upgradeability
- Remove `readthedocs.yml`
@tomvothecoder
Copy link
Collaborator Author

tomvothecoder commented Jul 19, 2023

Hi @TonyB9000, I'm tagging you for review so you can familiarize yourself with the changes. You can adopt a similar approach when you open up a PR to simplify the datasm conda-env yaml files.

Please approve once you are good to go.

@@ -35,32 +35,61 @@ jobs:
- if: ${{ steps.skip_check.outputs.should_skip != 'true' }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can ignore the changes in this file. This is purely DevOps work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh. DevOps work. (complete mystery).

@@ -4,14 +4,14 @@ fail_fast: true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ignore the changes in this file as well.

Comment on lines +1 to 28
# Conda e3sm_to_cmip CI/CD environment (used in GH Actions).
name: e3sm_to_cmip_ci
channels:
- conda-forge
- defaults
dependencies:
# Base
# ==================
- python>=3.9
- pip
- cmor>=3.6.0
# Base - required for building the repository as an Anaconda package
# ====================
- python >=3.9
- cdms2=3.1.5
- cdutil=8.2.1
- cmor >=3.7.0
- dask
- nco
- nco >=5.1.4
- netcdf4
- numpy
- numpy >=1.23.0 # This version of numpy includes support for Python 3.11.
- pyyaml
- tqdm
- xarray
# Required for phalf and pfull handlers.
- cdms2=3.1.5
- cdutil=8.2.1
- xarray >=2022.02.0 # This version of Xarray drops support for Python 3.8.
# Testing
# ==================
- pytest
- pytest-cov
# Documentation
# =================
- sphinx
- sphinx_rtd_theme
prefix: /opt/miniconda3/envs/e3sm_to_cmip_ci
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

File of interest

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no problem with these changes. But I have no good idea how you knew to make them, nor how to apply anything similar to the datasm proc/pub.yml specs.

Copy link
Collaborator Author

@tomvothecoder tomvothecoder Jul 19, 2023

Choose a reason for hiding this comment

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

Reasoning behind constraints:

  • python - we want to support 3.9 or higher
  • cdms2 and cdutil - these are the latest versions and we need them for pfull/phalf handlers. We don't want old versions because there are probably bugs in them and dependency compatibility issues
  • nco - I presumed we want 5.1.4 or greater based on our past experience working with this package in e2c (this will pick up the latest version)
  • numpy - there is a comment in the yml file
  • xarray - there is a comment in the yml file

Copy link
Contributor

Choose a reason for hiding this comment

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

@tomvothecoder Thanks Tom, I've already (sort of) modified my datasm proc.yml based on this guidance. There is some overlap. Yet to test datasm (will wait until e3sm_to_cmip testing is completed.)

Comment on lines +7 to +19
# Base - required for building the repository as an Anaconda package
# ====================
- python >=3.9
- cdms2=3.1.5
- cdutil=8.2.1
- cmor >=3.7.0
- dask
- nco >=5.1.4
- netcdf4
- numpy >=1.23.0 # This version of numpy includes support for Python 3.11.
- pyyaml
- tqdm
- xarray >=2022.02.0 # This version of Xarray drops support for Python 3.8.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

File of interest

Copy link
Contributor

Choose a reason for hiding this comment

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

See previous comment.

@TonyB9000 TonyB9000 merged commit f5fd403 into master Jul 19, 2023
2 checks passed
@TonyB9000 TonyB9000 deleted the devops/simplify-conda-envs branch July 19, 2023 17:42
@TonyB9000
Copy link
Contributor

I merged. I did a pull in my repo that brought down the changes to master.

(base) -bash-4.2$ git pull
Updating 96373bc..f5fd403
Fast-forward
 .github/workflows/build_workflow.yml | 57 +++++++++++++++++++++++++++++++++++++++++++--------------
 .pre-commit-config.yaml              |  8 ++++----
 conda-env/ci.yml                     | 26 ++++++++++++++------------
 conda-env/dev.yml                    | 46 +++++++++++++++++++++++-----------------------
 conda-env/readthedocs.yml            | 27 ---------------------------
 readthedocs.yml                      |  2 +-
 6 files changed, 85 insertions(+), 81 deletions(-)
 delete mode 100644 conda-env/readthedocs.yml

I issued mamba env create -n e2c_loose_e2c1100rc1 -f conda-env/dev.yml (no complaints)

I issued mamba activate e2c_loose_e2c1100rc1

I issued pip install . (e3sm_to_cmip)

I issued e3sm_to_cmip --version

(e2c_loose_e2c1100rc1) -bash-4.2$ e3sm_to_cmip --version
Traceback (most recent call last):
  File "/home/bartoletti1/mambaforge/envs/e2c_loose_e2c1100rc1/bin/e3sm_to_cmip", line 5, in <module>
    from e3sm_to_cmip.__main__ import main
  File "/home/bartoletti1/mambaforge/envs/e2c_loose_e2c1100rc1/lib/python3.10/site-packages/e3sm_to_cmip/__main__.py", line 48, in <module>
    np.warnings.filterwarnings("ignore")  # type: ignore
  File "/home/bartoletti1/mambaforge/envs/e2c_loose_e2c1100rc1/lib/python3.10/site-packages/numpy/__init__.py", line 322, in __getattr__
    raise AttributeError("module {!r} has no attribute "
AttributeError: module 'numpy' has no attribute 'warnings'. Did you mean: 'hanning'?

Not sure how to proceed from here.

@xylar
Copy link
Contributor

xylar commented Jul 19, 2023

@TonyB9000, presumably numpy.warnings was deprecated and then removed from numpy. I think the easiest is to remove these lines:
https://github.com/E3SM-Project/e3sm_to_cmip/blob/master/e3sm_to_cmip/__main__.py#L47-L48

If some warning is happening that you want to suppress, you will have to find another way.

@TonyB9000
Copy link
Contributor

@xylar Thanks Xylar. I hope to add an option to suppress all of the "progress bar" output - in batch mode these just create MB of non-print chars in log files.

@xylar
Copy link
Contributor

xylar commented Jul 19, 2023

Sounds good but that's an entirely unrelated issue

@TonyB9000
Copy link
Contributor

I can now get "e3sm_to_cmip --version" to report e3sm_to_cmip 1.10.0rc1, but the "--info" mode was throwing errors: in the
self._setup_dirs_with_paths(), which is called BEFORE checking for self._run_info_mode() which does not need those paths (hence, os.path stuff was complaining of "NoneType" issues.)

Fixed now, I think. I am getting the expected

2023-07-19 20:32:14,857_857:INFO:run:--------------------------------------
2023-07-19 20:32:14,857_857:INFO:run:| Running E3SM to CMIP in Info Mode
2023-07-19 20:32:14,857_857:INFO:run:--------------------------------------
[{'CMIP6 Name': 'pr',
  'CMIP6 Table': 'CMIP6_day.json',
  'CMIP6 Units': 'kg m-2 s-1',
  'E3SM Variables': 'PRECT'},
 {'CMIP6 Name': 'pr',
  'CMIP6 Table': 'CMIP6_Amon.json',
  'CMIP6 Units': 'kg m-2 s-1',
  'E3SM Variables': 'PRECC, PRECL'}]

If it works with mode 3 (checking for variable support in supplied data-path), I'll push this. But should be in another issue somewhere (not #198). I'll use #197 I think.

@tomvothecoder
Copy link
Collaborator Author

@TonyB9000, presumably numpy.warnings was deprecated and then removed from numpy. I think the easiest is to remove these lines: master/e3sm_to_cmip/main.py#L47-L48

If some warning is happening that you want to suppress, you will have to find another way.

Thanks Xylar. I added a FIXME line there 5 months ago according to the git blame. It looks like this has caught up now as an actual issue (#196).

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

Successfully merging this pull request may close these issues.

e2c v1.10.0rc1 with cmor 3.7.2 environment fails to build
3 participants