Make TC Analysis parallel #631

forsyth2 · 2024-10-15T21:18:21Z

Issue resolution

May close Fix shared library error in tc_analysis #180. If the errors from Fix shared library error in tc_analysis #180 persist, I will try merging the zppy implementation of make --closedcontourcmd resolution specific e3sm_diags#824 first, and run the tests again, seeing if they are gone then.
Resolves the e3sm_diags piece of Improve carryover dependency handling #623, thus closing [Bug]: Issues with tc_analysis in zppy #622.

Select one: This pull request is...

a bug fix: increment the patch version
a small improvement: increment the minor version
an incompatible (non-backwards compatible) API change: increment the major version

1. Does this do what we want it to do?

Objectives:

Remove sequential-only restriction on tc_analysis task.
Remove sequential-only restriction on the e3sm_diags task when tc_analysis is in sets.
Change min-case tests of tc_analysis to use v2, since data in v3 is unavailable at the moment (Add support for multiple time series datasets via glob and fix enso_diags set e3sm_diags#866 (comment))

Required:

Product Management: I have confirmed with the stakeholders that the objectives above are correct and complete.
Discussed these design decisions with @chengzhuzhang
Testing: I have added or modified at least one "min-case" configuration file to test this change. Every objective above is represented in at least one cfg.
Testing: I have considered likely and/or severe edge cases and have included them in testing.

2. Are the implementation details accurate & efficient?

Required:

Logic: I have visually inspected the entire pull request myself.
Logic: I have left GitHub comments highlighting important pieces of code logic. I have had these code blocks reviewed by at least one other team member.
Left comments, no block here requires review.

3. Is this well documented?

Required:

Documentation: by looking at the docs, a new user could easily understand the functionality introduced by this pull request.
The vast majority of zppy's user documentation is via https://docs.e3sm.org/zppy/_build/html/main/parameters.html, which directs to https://github.com/E3SM-Project/zppy/blob/main/zppy/templates/default.ini, which doesn't appear to make any references to only being able to run tc_analysis sequentially.

4. Is this code clean?

Required:

Readability: The code is as simple as possible and well-commented, such that a new team member could understand what's happening.
Pre-commit checks: All the pre-commits checks have passed.

If applicable:

Software architecture: I have discussed relevant trade-offs in design decisions with at least one other team member. It is unlikely that this pull request will increase tech debt.
Discussed these design decisions with @chengzhuzhang

forsyth2 · 2024-10-15T22:47:15Z

Unit tests pass (https://github.com/E3SM-Project/zppy/actions/runs/11355423070/job/31584722994?pr=631).

I didn't run into any of the issues from #180. Those errors were intermittent, so I do have some concern they just didn't happen to happen this time. That said, it's very possible underlying things have changed since those bugs were first noticed. For reference, those bugs were:

GenerateConnectivityFile: error while loading shared libraries: libnetcdf.so.11: cannot open shared object file: No such file or directory previously occurred when multiple year sets of tc_analysis were run simultaneously. This was tested here with test_min_case_e3sm_diags_tc_analysis_parallel_chrysalis.cfg.
socket.gaierror: [Errno -2] Name or service not known previously occurred in E3SM Diags when running the tc_analysis set in parallel. This was also tested here with test_min_case_e3sm_diags_tc_analysis_parallel_chrysalis.cfg.

Full testing details

# zppy dev environment
mamba clean --all
mamba env create -f conda/dev.yml -n zppy_dev_make-tc-parallel-20241015
conda activate zppy_dev_make-tc-parallel-20241015
pre-commit run --all-files
pip install .
python -u -m unittest tests/test_*.py

# e3sm_diags dev environment
cd ../e3sm_diags
git checkout main
git fetch upstream
git reset --hard upstream/main
git log # Should match https://github.com/E3SM-Project/e3sm_diags/commits/main
mamba clean --all
mamba env create -f conda-env/dev.yml -n e3sm_diags_20241015
conda activate e3sm_diags_20241015
pip install .

cd ../zppy
conda activate zppy_dev_make-tc-parallel-20241015
pip install .

zppy -c tests/integration/generated/test_min_case_tc_analysis_simultaneous_1_chrysalis.cfg
zppy -c tests/integration/generated/test_min_case_tc_analysis_simultaneous_2_chrysalis.cfg 
cd /lcrc/group/e3sm/ac.forsyth2/zppy_min_case_tc_analysis_simultaneous_1_output/test-631-v2/v2.LR.historical_0201/post/scripts/
grep -v "OK" *status
# No errors
cd /lcrc/group/e3sm/ac.forsyth2/zppy_min_case_tc_analysis_simultaneous_2_output/test-631-v2/v2.LR.historical_0201/post/scripts/
grep -v "OK" *status
# No errors

zppy -c tests/integration/generated/test_min_case_e3sm_diags_tc_analysis_chrysalis.cfg
cd /lcrc/group/e3sm/ac.forsyth2/zppy_min_case_e3sm_diags_tc_analysis_output/test-631-v2/v2.LR.historical_0201/post/scripts
grep -v "OK" *status
# No errors
# https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.forsyth2/zppy_min_case_e3sm_diags_tc_analysis_www/test-631-v2/v2.LR.historical_0201/e3sm_diags/atm_monthly_180x360_aave_tc_analysis/model_vs_obs_1985-1986/viewer/tc_analysis/index.html has plots

zppy -c tests/integration/generated/test_min_case_e3sm_diags_tc_analysis_mvm_1_chrysalis.cfg
# Wait to complete
cd /lcrc/group/e3sm/ac.forsyth2/zppy_min_case_e3sm_diags_tc_analysis_mvm_1_output/test-631-v2/v2.LR.historical_0201/post/scripts/
grep -v "OK" *status
# No errors
cd -
zppy -c tests/integration/generated/test_min_case_e3sm_diags_tc_analysis_mvm_2_chrysalis.cfg
cd /lcrc/group/e3sm/ac.forsyth2/zppy_min_case_e3sm_diags_tc_analysis_mvm_2_output/test-631-v2/v2.LR.historical_0201/post/scripts/
grep -v "OK" *status
# No errors
# https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.forsyth2/zppy_min_case_e3sm_diags_tc_analysis_mvm_2_www/test-631-v2/v2.LR.historical_0201/e3sm_diags/atm_monthly_180x360_aave_mvm/model_vs_model_1995-1996/viewer/tc_analysis/index.html has plots

zppy -c tests/integration/generated/test_min_case_e3sm_diags_tc_analysis_parallel_chrysalis.cfg
cd /lcrc/group/e3sm/ac.forsyth2/zppy_min_case_e3sm_diags_tc_analysis_parallel_output/test-631-v2/v2.LR.historical_0201/post/scripts/
grep -v "OK" *status
# No errors
# https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.forsyth2/zppy_min_case_e3sm_diags_tc_analysis_parallel_www/test-631-v2/v2.LR.historical_0201/e3sm_diags/atm_monthly_180x360_aave_tc_analysis/model_vs_obs_1985-1986/viewer/tc_analysis/index.html has plots
# https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.forsyth2/zppy_min_case_e3sm_diags_tc_analysis_parallel_www/test-631-v2/v2.LR.historical_0201/e3sm_diags/atm_monthly_180x360_aave_tc_analysis/model_vs_obs_1987-1988/viewer/tc_analysis/index.html has plots

forsyth2

This should resolve: #180, #622. However, #623 will still need to be done in some form to account for dependency handling in other tasks. Furthermore, we still need to implement the equivalent changes of E3SM-Project/e3sm_diags#824.

forsyth2 · 2024-10-15T22:54:13Z

tests/integration/template_min_case_e3sm_diags_tc_analysis.cfg

@@ -1,20 +1,20 @@
 [default]
-case = "#expand case_name#"
+case = "#expand case_name_v2#"


Any test using tc_analysis now uses the v2 input data because of E3SM-Project/e3sm_diags#866 (comment)

forsyth2 · 2024-10-15T22:55:13Z

tests/integration/template_min_case_e3sm_diags_tc_analysis_parallel.cfg

@@ -0,0 +1,36 @@
+[default]


New min-case test where E3SM Diags runs in parallel to generate tc_analysis plots for two year sets.

forsyth2 · 2024-10-15T22:56:08Z

zppy/e3sm_diags.py

    for c in tasks:
+        dependencies: List[str] = []


The relevant changes from #623. We want to reset the dependencies list at the beginning of each task (i.e., nothing should be carried over).

forsyth2 · 2024-10-15T22:56:26Z

zppy/e3sm_diags.py

-
-                    # Due to a `socket.gaierror: [Errno -2] Name or service not known` error when running e3sm_diags with tc_analysis
-                    # on multiple year_sets, if tc_analysis is in sets, then e3sm_diags should be run sequentially.
-                    if "tc_analysis" in c["sets"]:


No longer running e3sm_diags sequentially

forsyth2 · 2024-10-15T22:56:37Z

zppy/tc_analysis.py

    for c in tasks:
+
+        dependencies: List[str] = []


Relevant change from #623

forsyth2 · 2024-10-15T22:56:49Z

zppy/tc_analysis.py

-                    # Note that this line should still be executed even if jobid == -1
-                    # The later tc_analysis tasks still depend on this task (and thus will also fail).
-                    # Add to the dependency list
-                    dependencies.append(status_file)


No longer running tc_analysis sequentially.

chengzhuzhang · 2024-10-15T23:37:11Z

@forsyth2 thank you for working on the PR and the comments. I visually checked and it looks good. I'm happy to review any upcoming TC analysis related PRs, so feel free to tag me.

forsyth2 · 2024-10-16T00:15:13Z

Thanks @chengzhuzhang!

forsyth2 added the semver: small improvement Small improvement (will increment patch version) label Oct 15, 2024

forsyth2 self-assigned this Oct 15, 2024

forsyth2 marked this pull request as ready for review October 15, 2024 22:48

forsyth2 commented Oct 15, 2024

View reviewed changes

Make TC Analysis parallel

a70db5e

forsyth2 force-pushed the make-tc-parallel branch from 3cb5659 to a70db5e Compare October 15, 2024 23:03

forsyth2 merged commit d5c8005 into main Oct 15, 2024
4 checks passed

forsyth2 deleted the make-tc-parallel branch October 15, 2024 23:03

forsyth2 mentioned this pull request Oct 15, 2024

[Bug]: Issues with tc_analysis in zppy #622

Closed

forsyth2 mentioned this pull request Oct 16, 2024

Improve carryover dependency handling #623

Merged

11 tasks

zhangshixuan1987 pushed a commit to zhangshixuan1987/zppy that referenced this pull request Nov 1, 2024

Make TC Analysis parallel (E3SM-Project#631)

3b60b4f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make TC Analysis parallel #631

Make TC Analysis parallel #631

forsyth2 commented Oct 15, 2024 •

edited

Loading

forsyth2 commented Oct 15, 2024

forsyth2 left a comment

forsyth2 Oct 15, 2024

forsyth2 Oct 15, 2024

forsyth2 Oct 15, 2024

forsyth2 Oct 15, 2024

forsyth2 Oct 15, 2024

forsyth2 Oct 15, 2024

chengzhuzhang commented Oct 15, 2024

forsyth2 commented Oct 16, 2024

Make TC Analysis parallel #631

Make TC Analysis parallel #631

Conversation

forsyth2 commented Oct 15, 2024 • edited Loading

Issue resolution

1. Does this do what we want it to do?

2. Are the implementation details accurate & efficient?

3. Is this well documented?

4. Is this code clean?

forsyth2 commented Oct 15, 2024

forsyth2 left a comment

Choose a reason for hiding this comment

forsyth2 Oct 15, 2024

Choose a reason for hiding this comment

forsyth2 Oct 15, 2024

Choose a reason for hiding this comment

forsyth2 Oct 15, 2024

Choose a reason for hiding this comment

forsyth2 Oct 15, 2024

Choose a reason for hiding this comment

forsyth2 Oct 15, 2024

Choose a reason for hiding this comment

forsyth2 Oct 15, 2024

Choose a reason for hiding this comment

chengzhuzhang commented Oct 15, 2024

forsyth2 commented Oct 16, 2024

forsyth2 commented Oct 15, 2024 •

edited

Loading