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

[1 pt] PR: Add CI - GitHub Actions Workflow file #1030

Merged
merged 13 commits into from
Nov 17, 2023
Merged

Conversation

robgpita
Copy link
Contributor

@robgpita robgpita commented Nov 8, 2023

This PR introduces the .github/workflows/lint_and_format.yaml file which serves as the first step in developing a Continuous Integration pipeline for this repository. The flake8-pyproject dependency is now used, as it works out of the box with the pre-commit GitHub Action in the GitHub Hosted Runner environment. In switching to this package, there were a couple of E721 which appeared. Modifications were made to the appropriate files to resolve the flake8 E721 errors. Also, updates to the unit_tests were necessary since Branch IDs have changed with the latest code.

Accomplishes the first step of incorporating CI from #1022

Closes #1034 : Upgrade to pyarrow version

The only difference in functionality is that instead of running pflake8 as a wrapper for flake8, one can call flake8 directly when specifying the flake8-pyproject as an additional_dependencies in .pre-commit-config.yaml to pick up the pyproject.toml configuration file.

Changes

  • .pre-commit-config.yaml: use flake8-pyproject package instead of pyproject-flake8.
  • Pipfile and Pipfile.lock: updated to use flake8-pyproject package instead of pyproject-flake8, upgrade pyarrow version.
  • data
    • /wbd/generate_pre_clip_fim_huc8.py: Add space between (-) operator line 134.
    • write_parquet_from_calib_pts.py: Add space between (-) operator line 234.
  • src
    • check_huc_inputs.py: Change == string to is str, remove import string
  • tools
    • eval_plots.py: Add space after comma in lines 207 & 208
    • generate_categorical_fim_mapping.py:
    • hash_compare.py: Add space after comma line 153.
    • inundate_mosaic_wrapper.py: Use is instead of == line 73.
    • inundation_wrapper_nwm_flows.py: Use is not instead of != line 76.
    • mosaic_inundation.py: Use is instead of == line 181.
  • unit_tests
    • clip_vectors_to_wbd_test.py: File moved to data/wbd directory, update import statement
    • filter_catchments_and_add_attributes_params.json: Update Branch ID
    • outputs_cleanup_params.json: Update Branch ID
    • split_flows_params.json: Update Branch ID
    • usgs_gage_crosswalk_params.json: Update Branch ID & update argument to gage_crosswalk.run_crosswalk
    • unit_tests/usgs_gage_crosswalk_test.py: Update params to gage_crosswalk.run_crosswalk

Additions

  • .github/workflows/
    • lint_and_format.yaml: Add GitHub Actions Workflow file for Continuous Integration environment.

Testing

fim_pipeline.sh was run successfully in generating the new unit test data.
hash_compare.py script was successful when comparing the unit test data generated with the dev branch, and with this branch. (After the unit tests were run, for consistency)

Issuer Checklist (For developer use)

You may update this checklist before and/or after creating the PR. If you're unsure about any of them, please ask, we're here to help! These items are what we are going to look for before merging your code.

  • Informative and human-readable title, using the format: [_pt] PR: <description>
  • Links are provided if this PR resolves an issue, or depends on another other PR
  • If submitting a PR to the dev branch (the default branch), you have a descriptive Feature Branch name using the format: dev-<description-of-change> (e.g. dev-revise-levee-masking)
  • Changes are limited to a single goal (no scope creep)
  • The feature branch you're submitting as a PR is up to date (merged) with the latest dev branch
  • pre-commit hooks were run locally
  • Any change in functionality is tested
  • Passes all unit tests locally (inside interactive Docker container, at /foss_fim/, run: pytest unit_tests/)
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • CHANGELOG updated with template version number, e.g. 4.x.x.x
  • Reviewers requested
  • Add yourself as an assignee in the PR as well as the FIM Technical Lead

Merge Checklist (For Technical Lead use only)

  • Update CHANGELOG with latest version number and merge date
  • Update the Citation.cff file to reflect the latest version number in the CHANGELOG
  • If applicable, update README with major alterations

@robgpita robgpita self-assigned this Nov 8, 2023
@robgpita robgpita added the CI/CD CI/CD - devOps related label Nov 8, 2023
@robgpita robgpita changed the title [1 pt]:PR Add CI - GitHub Actions Workflow file [1 pt] PR: Add CI - GitHub Actions Workflow file Nov 8, 2023
@robgpita robgpita marked this pull request as ready for review November 8, 2023 19:10
@robgpita robgpita added enhancement New feature or request testing Evaluation and testing related FIM4 labels Nov 8, 2023
@RobHanna-NOAA
Copy link
Contributor

Had to add a quick fix to src_adjust_ras2fim_rating.py in order to test it. I tested again the HUC of 12040101 which is a ras2fim HUC. In EFS, it also failed because the EFS version was way out of date. It was ras2fim pre rel_100 and now it is rel_100. Later we will upgrade it to rel_101 after rel_101 gets stabilized.

RobHanna-NOAA
RobHanna-NOAA previously approved these changes Nov 9, 2023
Copy link
Contributor

@RobHanna-NOAA RobHanna-NOAA left a comment

Choose a reason for hiding this comment

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

Did a number of tests in EC2's.

  • created a new docker container

  • ran pipeline with 12040101 has both alpha domain data and ras2fim calibration data.

  • found an error in both the code and EFS for the ras2fim rating curve and fixed it. Note; S3 edition of the ras2fim rating curve was ras2fim rel_100 which is good enough for now.

  • ran a quick synth test and eval test.

    • python3 ./foss_fim/tools/synthesize_test_cases.py -c DEV -l -v robh_cli_1 -jh 1 -jb 6 -m /outputs/robh_cli_1/robh_cli_1_metrics.csv -o -pcsv /data/previous_fim/fim_4_4_0_0/fim_4_4_0_0_metrics.csv
    • python3 foss_fim/tools/eval_plots.py -m /outputs/robh_cli_1/robh_cli_1_metrics.csv -w /outputs/robh_cli_1/eval/ -v fim_4_3_11_0 fim_4_3_12_0 fim_4_4_0_0
  • ran a unit test check == pytest /foss_fim/unit_tests. All but three passed.

  • within the container ran isort, black and flake8 (flake8 is the new command to be run now) and it all worked well.

  • Could not run pre-commit inside the container as the container does not have git.

  • did a git add, commit, push from outside the container without issue.

RobHanna-NOAA
RobHanna-NOAA previously approved these changes Nov 9, 2023
Copy link
Contributor

@RobHanna-NOAA RobHanna-NOAA left a comment

Choose a reason for hiding this comment

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

Looks great and ran well.
Had a bit of trouble initially with unit tests but easily got passed it and all unit tests ran well.

dependabot bot and others added 2 commits November 9, 2023 22:39
Bumps [pyarrow](https://github.com/apache/arrow) from 11.0.0 to 14.0.1.
- [Commits](apache/arrow@go/v11.0.0...apache-arrow-14.0.1)

---
updated-dependencies:
- dependency-name: pyarrow
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
RobHanna-NOAA
RobHanna-NOAA previously approved these changes Nov 13, 2023
@CarsonPruitt-NOAA CarsonPruitt-NOAA merged commit 89c7023 into dev Nov 17, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD CI/CD - devOps related enhancement New feature or request FIM4 testing Evaluation and testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants