-
Notifications
You must be signed in to change notification settings - Fork 33
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
Update DevOps tooling #551
Conversation
85c6100
to
e98273d
Compare
"Programming Language :: Python :: 3", | ||
"Programming Language :: Python :: 3.8", | ||
"Programming Language :: Python :: 3.9", | ||
"Programming Language :: Python :: 3.10", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently, the package can be built on Python 3.10 based on CI/CD.
I opted not to update our Anaconda environments to 3.10 just yet though.
@@ -82,7 +82,7 @@ def compare_images( | |||
expected_png = Image.open(path_to_expected_png).convert("RGB") | |||
diff = ImageChops.difference(actual_png, expected_png) | |||
|
|||
diff_dir = "image_check_failures" | |||
diff_dir = f"{TEST_ROOT_PATH}image_check_failures" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The image_check_failures/
dir was being placed in the root of the repo. .gitignore
wasn't ignoring it as a result.
Hi @forsyth2, I'm hoping to get this PR merged before our next release. If you have the time, it would be great if you can help me fix the failing tests (https://github.com/E3SM-Project/e3sm_diags/runs/4806697230?check_suite_focus=true) and review the PR afterwards.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me.
As for the test failures, they appear to because of minor plotting changes (probably from the new package versions). Do you want me to update the expected images (https://e3sm-project.github.io/e3sm_diags/_build/html/master/dev_guide/testing.html)?
For tests/complete_run.py
, it looks like there are many expected images to update as well. Also, arm_diags/armdiags-convection-onset-twpc3.png
isn't being created for some reason.
Thanks for the link to the instructions. I haven't updated expected images before, but I'll give a shot. |
f52b3a2
to
473386d
Compare
473386d
to
6f93fcc
Compare
@@ -159,7 +159,7 @@ def precst(precc, precl): | |||
|
|||
def tauxy(taux, tauy): | |||
"""tauxy = (taux^2 + tauy^2)sqrt""" | |||
var = (taux ** 2 + tauy ** 2) ** 0.5 | |||
var = (taux**2 + tauy**2) ** 0.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest version of black
(22.1.0) now fixes unnecessary spaces between **
Hi @forsyth2 and @chengzhuzhang, this PR is nearly complete, but the test suite is failing (specifically for enso diags). I validated that the changes in this PR aren't a factor by running the test suite with the latest stable release (v2.6.1) of Please advise on what I should do here, since I usually don't handle test suite failures related to image diffs. Should I update the expected images? Related log lines:
|
That's almost certainly what happened
Let me investigate. I'll let you know |
@tomvothecoder Well I have tests passing on
|
@forsyth2 Thanks a lot for checking it out.
I followed these steps and the tests pass also on my end.
I would create a clean environment using Note, the env is now called |
I thought that's what I was doing with |
Ahh I misread your command and thought It's very odd that I will debug this and keep you posted. |
I have a tool I use when You could modify the tool to use |
Thanks for providing that tool @xylar, it looks really useful. In my case, I took an alternate approach by building the production env with the latest stable version of I still need to figure out the root cause of the conflict, but at least things are working again for now. @forsyth2, you can try building the dev env using |
@tomvothecoder Ok, I got 7 test failures that appear to be caused by the 3 image failures at https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.forsyth2/e3sm_diags_development/image_check_failures_20220224/. The only noteworthy diff I see is that So, I think it's ok to just update the expected images as described in https://e3sm-project.github.io/e3sm_diags/_build/html/master/dev_guide/testing.html. Do you want me to do that? |
Thanks for checking @forsyth2! That would be awesome if you can handle updating the expected images since I'll be out tomorrow. |
@tomvothecoder Ok the build workflows are passing now. For reference, here are the steps I ran:
To do:
|
@forsyth2 Great, thanks for taking care of that Ryan. Happy to see the build is passing again! |
Anaconda: - Remove unused `meta.yaml` (packages are now released through conda-forge feedstock) - Remove `meta.yaml` from `tbump.toml` - Rename `conda/` dir to `conda-env/` - Rename `e3sm_diags_env_dev.yml` to `dev.yml` and `e3sm_diags_env.yml` to `prod.yml` - Update dependencies to the latest versions - Update name and prefix of envs - Remove dev dependences in `prod.yml` - Add `ci.yml` for a minimal CI/CD conda env to speed up GH Actions - Only contains the minimum dependencies required to build the package and run the test suite - Add `docs.yml` for minimal CI/CD docs env to speed up GH Actions - Only contains the minimum dependencies required to build the documentation CI/CD: - Add Python matrix testing to `build_workflow.yml` - Use `ci.yml` instead of `dev.yml` when building conda env `setup.py`: - Update `setup()` contents with supported Python versions `setup.cfg` and `pre-commit-config.yaml` - Update dependencies to the latest versions Docs: - Update `docs/source/install.rst` with latest conda env updates
a8d67ac
to
ddd5908
Compare
FYI @forsyth2 and @chengzhuzhang, you should follow the "What to do after this is merged" instructions in this PR. |
Thank you for the heads-up! |
@tomvothecoder Thanks, I updated my environments and also did the update-README step I listed above. |
This PR updates the DevOps tooling to optimize build time, remove deprecated files, and update dependencies to the latest versions.
Summary of Changes
Conda environments:
conda/
dir toconda-env/
e3sm_diags_env_dev.yml
todev.yml
ande3sm_diags_env.yml
toprod.yml
e3sm_diags_env_dev
ande3sm_diags_env_prod
toe3sm_diags_dev
ande3sm_diags_prod
prod.yml
(installing latest stable version already includes these dependencies)ci.yml
for a minimal CI/CD conda env to speed up GH Actionsdocs.yml
for a minimal CI/CD docs env to speed up GH ActionsConda recipe:
meta.yaml
setup.py:
setup()
contents with supported Python versionssetup.cfg and pre-commit-config.yaml:
CI/CD:
build_workflow.yml
docs.yml
) inpublish-docs
stepWhat to do after this is merged
master
conda env create -f conda-env/dev.yml
conda env create -f conda-env/prod.yml
e3sm_diags_dev
ande3sm_diags_prod
envs are created