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

Update TC Analysis template #632

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Update TC Analysis template #632

wants to merge 2 commits into from

Conversation

forsyth2
Copy link
Collaborator

Issue resolution

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:

  • Objective 1
  • Objective 2
  • ...
  • Objective n

Required:

  • Product Management: I have confirmed with the stakeholders that the objectives above are correct and complete.
  • 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.

If applicable:

  • Testing: this pull request introduces an important feature or bug fix that we must test often. I have updated the weekly-test configuration files, not just a "min-case" one.
  • Testing: this pull request adds at least one new possible parameter to the cfg. I have tested using this parameter with and without any other parameter that may interact with it.

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.

If applicable:

  • Dependencies: This pull request introduces a new dependency. I have discussed this requirement with at least one other team member. The dependency is noted in zppy/conda, not just an import statement.

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.

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.

@forsyth2 forsyth2 self-assigned this Oct 16, 2024
@forsyth2
Copy link
Collaborator Author

forsyth2 commented Oct 16, 2024

TODO:

  • Remove scratch parameter elsewhere it appears. Will need to stay in default.ini for backwards compatibility.
  • Test the tc_analysis min-case cfgs (currently using v2 data)
  • Switch those min-case cfgs back to using v3 data and test again (or have v2 and v3 versions of those min-case tests)

@forsyth2
Copy link
Collaborator Author

@chengzhuzhang The relevant code changes are in zppy/templates/tc_analysis.bash. Could you please review those to make sure they match up with E3SM-Project/e3sm_diags#824 as intended?

I tested all the min-case cfg files on v2 data and everything was fine. However, I ran into some issues on v3. min_case_e3sm_diags_tc_analysis_parallel and min_case_e3sm_diags_tc_analysis_mvm_2 resulted in ERROR (1) for the tc_analysis job. ERROR (1) means the dat file is empty. grep -in error <.o file> doesn't return anything in either case. I'm not sure why this would only happen on some of the runs.

If you'd like to try this out yourself:

Testing directions
# Check out the branch
git fetch upstream
git checkout -b tc-analysis-updates upstream/tc-analysis-updates

# Set up e3sm diags dev environment
cd <e3sm_diags directory>
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_<date>
conda activate e3sm_diags_<date>
pip install .

# Set up zppy dev environment
cd <zppy directory>
mamba clean --all
mamba env create -f conda/dev.yml -n zppy_dev_pr632
conda activate zppy_dev_pr632
pip install .

# This block is only if you make further changes to the code
pre-commit run --all-files
pip install .
python -u -m unittest tests/test_*.py # Run unit tests

# Set up the testing files
emacs tests/integration/utils.py # set `UNIQUE_ID = "test-pr632"` and under `get_chrysalis_expansions`: `"diags_environment_commands": "source <path to conda.sh>; conda activate e3sm_diags_<date>",`
python tests/integration/utils.py # Actually generate the testing files

# Run zppy. These usually take 5-10 minutes to run in full.
zppy -c tests/integration/generated/test_min_case_e3sm_diags_tc_analysis_parallel_chrysalis.cfg
zppy -c tests/integration/generated/test_min_case_e3sm_diags_tc_analysis_mvm_1_chrysalis.cfg
# Wait for _1 to finish and then run:
zppy -c tests/integration/generated/test_min_case_e3sm_diags_tc_analysis_mvm_2_chrysalis.cfg

# Check results
cd /lcrc/group/e3sm/<username>/zppy_min_case_e3sm_diags_tc_analysis_parallel_output/test-pr632/v3.LR.historical_0051/post/scripts/
grep -v "OK" *status
cd /lcrc/group/e3sm/<username>/zppy_min_case_e3sm_diags_tc_analysis_mvm_2_output/test-pr632/v3.LR.historical_0051/post/scripts/
grep -v "OK" *status

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.

1 participant