`verrou` into Workflow #937

Malmahrouqi3 · 2025-07-12T15:29:19Z

User description

Description

Concerning (#650),
Intended as standalone floating-point error checker.

Debugging info,
The current Valgrind setup ought to be sufficient, so debugging locally (nektos/act extension) or on the CI are all viable.

To-dos,

Proper Valgrind commands to generate the most amount of errors.
Save and process all reports such that non-empty error summaries are displayed.

Ref verrou docs

PR Type

Other

Description

Remove all GitHub Actions workflow files
Add new Verrou floating-point compliance workflow
Delete HPC cluster-specific scripts (Frontier, Phoenix)
Replace comprehensive CI/CD pipeline with single workflow

Diagram Walkthrough

flowchart LR
  A["Old CI/CD Pipeline"] --> B["Complete Removal"]
  B --> C["New Verrou Workflow"]
  C --> D["Floating-Point Testing"]

File Walkthrough

Relevant files

Miscellaneous

20 files

bench.sh `Remove Frontier benchmarking script`	+0/-16
build.sh `Remove Frontier build script`	+0/-17
submit-bench.sh `Remove Frontier benchmark submission script`	+0/-54
submit.sh `Remove Frontier job submission script`	+0/-55
test.sh `Remove Frontier testing script`	+0/-10
bench.sh `Remove Phoenix benchmarking script`	+0/-28
submit-bench.sh `Remove Phoenix benchmark submission script`	+0/-107
submit.sh `Remove Phoenix job submission script`	+0/-100
test.sh `Remove Phoenix testing script`	+0/-30
bench.yml `Remove benchmark workflow`	+0/-111
cleanliness.yml `Remove code cleanliness workflow`	+0/-127
coverage.yml `Remove coverage testing workflow`	+0/-48
docs.yml `Remove documentation workflow`	+0/-76
formatting.yml `Remove formatting workflow`	+0/-19
line-count.yml `Remove line counting workflow`	+0/-54
lint-source.yml `Remove source linting workflow`	+0/-55
lint-toolchain.yml `Remove toolchain linting workflow`	+0/-17
pmd.yml `Remove PMD source analysis workflow`	+0/-131
spelling.yml `Remove spell checking workflow`	+0/-17
test.yml `Remove comprehensive test suite workflow`	+0/-132

Tests

1 files

verrou.yml `Add Verrou floating-point compliance workflow`	+39/-0

qodo-merge-pro · 2025-07-12T15:29:43Z

PR Reviewer Guide 🔍

(Review updated until commit `97ff4df`)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 650 - Partially compliant Compliant requirements: • Use Valgrind's verrou tool to check for poorly conditioned float operations • Implement floating-point error checking capability Non-compliant requirements: • Identify source of randomly failing tests when compiled differently Requires further human verification: • Verify that the verrou tool actually detects floating-point issues in the codebase • Confirm that the workflow produces meaningful error reports • Test that the setup works correctly in CI environment
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Incomplete Implementation The verrou command runs with --dry-run flag which prevents actual execution of tests, and there's no error processing or report generation as mentioned in the PR description todos valgrind --tool=verrou ./mfc.sh test -a --dry-run Missing Error Handling The Check Compliance step only echoes a message without actually processing verrou output or checking for floating-point errors - name: Check Compliance run: \| echo "Checking floating point compliance..." Destructive Changes All existing CI/CD workflows were deleted and replaced with a single incomplete workflow, removing comprehensive testing infrastructure name: Floating-Point Compliance on: [pull_request, push] jobs: Floating-Point_Compliance: runs-on: ubuntu-latest steps: - name: Clone PR uses: actions/checkout@v4 - name: Verrou Setup run: \| # get recent release of valgrind from GitHub API VERROU_VERSION=$(curl -s https://api.github.com/repos/edf-hpc/verrou/releases/latest \| grep -oP '"tag_name": "\K(.)(?=")') VERROU_NUMBER=${VERROU_VERSION#v} wget -P pr https://github.com/edf-hpc/verrou/releases/download/${VERROU_VERSION}/valgrind-3.23.0_verrou-${VERROU_NUMBER}.tar.gz cd pr tar -xzf valgrind-3.23.0_verrou-${VERROU_NUMBER}.tar.gz cd valgrind-3.23.0+verrou-${VERROU_NUMBER} ./autogen.sh ./configure --prefix=$HOME/.local make make install cd ../.. rm -rf pr/valgrind- - name: Run Tests run: \| sudo apt update -y sudo apt install -y cmake gcc g++ python3 python3-dev hdf5-tools \ libfftw3-dev libhdf5-dev openmpi-bin libopenmpi-dev valgrind --tool=verrou ./mfc.sh test -a --dry-run - name: Check Compliance run: \| echo "Checking floating point compliance..."

Copilot

Pull Request Overview

Adds a new GitHub Actions workflow to run floating-point compliance checks with Valgrind’s verrou tool.

Introduces .github/workflows/verrou.yml for triggering on PRs and pushes
Downloads, builds, and installs the latest Valgrind release
Executes tests under verrou and provides a placeholder for compliance reporting

Comments suppressed due to low confidence (4)

.github/workflows/verrou.yml:24

The step sources /usr/local/env.sh which is not generated by the Valgrind install; this will likely fail. Instead, export the path (e.g., export PATH=/usr/local/bin:$PATH) or source the correct environment file to ensure Valgrind is on the PATH.

          source /usr/local/env.sh

.github/workflows/verrou.yml:27

Using --dry-run prevents actual test execution under Valgrind, so no floating-point errors will be detected. Remove --dry-run to execute the tests for real.

        run: valgrind --tool=verrou ./mfc.sh test -a --dry-run

.github/workflows/verrou.yml:6

[nitpick] The job identifier self is ambiguous; rename it to something descriptive like floating_point_compliance or verrou_check for clarity.

  self:

.github/workflows/verrou.yml:31

This step only echoes a message and does not actually parse or summarize the verrou output. Consider adding commands to collect, report, and fail the workflow when errors are found.

          echo "Checking floating point compliance..."

qodo-merge-pro · 2025-07-12T15:30:21Z

PR Code Suggestions ✨

Latest suggestions up to 97ff4df
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Use locally installed valgrind binary The `valgrind` command needs to use the locally installed version from `$HOME/.local/bin`. The current command will use the system valgrind which doesn't have the verrou tool. .github/workflows/verrou.yml [29-35] - name: Run Tests run: \| sudo apt update -y sudo apt install -y cmake gcc g++ python3 python3-dev hdf5-tools \ libfftw3-dev libhdf5-dev openmpi-bin libopenmpi-dev - valgrind --tool=verrou ./mfc.sh test -a --dry-run + $HOME/.local/bin/valgrind --tool=verrou ./mfc.sh test -a --dry-run Suggestion importance[1-10]: 9 __ Why: The `valgrind` command on line 35 would use the system's version, not the custom-built one with the `verrou` tool installed in a previous step, causing the workflow to fail its objective.	High
General	Implement actual compliance checking logic The compliance check step only prints a message but doesn't actually analyze the verrou output. Add logic to parse verrou results and determine if floating-point issues were detected. .github/workflows/verrou.yml [37-39] - name: Check Compliance run: \| echo "Checking floating point compliance..." + # Add actual compliance checking logic here + # For example: check verrou output files or exit codes Suggestion importance[1-10]: 9 __ Why: This suggestion correctly points out that the `Check Compliance` step is a placeholder and does not check the output of `verrou`, rendering the entire workflow ineffective at detecting floating-point issues.	High
Update

Previous suggestions

✅ Suggestions up to commit 7bab395

Category	Suggestion	Impact
Possible issue	✅ ~~Fix environment variable persistence~~ Suggestion Impact: The suggestion was implemented by removing the 'source /usr/local/env.sh' line entirely, which addresses the persistence issue mentioned in the suggestion code diff: - source /usr/local/env.sh The `source` command only affects the current shell session and won't persist to subsequent workflow steps. Use `echo` to add environment variables to `$GITHUB_ENV` for persistence across steps. .github/workflows/verrou.yml [24] -source /usr/local/env.sh +echo "PATH=/usr/local/bin:$PATH" >> $GITHUB_ENV +echo "LD_LIBRARY_PATH=/usr/local/lib:$LD_LIBRARY_PATH" >> $GITHUB_ENV Suggestion importance[1-10]: 9 __ Why: This is a critical fix, as `source` only affects the current shell step, and environment variables will not persist to subsequent steps, likely causing the `valgrind` command to fail.	High
Possible issue	Add error handling for API calls Add error handling for the curl command to prevent workflow failure if the GitHub API is unavailable or returns unexpected data. Consider adding a fallback version or retry mechanism. .github/workflows/verrou.yml [15] -VALGRIND_VERSION=$(curl -s https://api.github.com/repos/LouisBrunner/valgrind/releases/latest \| grep -oP '"tag_name": "\K(.)(?=")') +VALGRIND_VERSION=$(curl -s https://api.github.com/repos/LouisBrunner/valgrind/releases/latest \| grep -oP '"tag_name": "\K(.)(?=")') \|\| { echo "Failed to fetch latest version"; exit 1; } Suggestion importance[1-10]: 7 __ Why: This suggestion correctly points out that the `curl` and `grep` pipeline could fail, making the workflow more robust by adding error handling and preventing cryptic failures in later steps.	Medium
General	Implement actual compliance checking This step only prints a message without performing any actual compliance checking. Either implement the actual verification logic or remove this placeholder step. .github/workflows/verrou.yml [30-31] run: \| - echo "Checking floating point compliance..." + echo "Floating point compliance check completed" + # Add actual verification commands here Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies that the step is a placeholder, which is likely intentional. While valid, it points out incompleteness rather than a flaw in the existing code.	Low

Malmahrouqi3 · 2025-07-13T19:17:21Z

Status Update: Valgrind builds nicely and PR is awaiting (#938) to merge in to replicate the results of (#650).

sbryngelson · 2025-07-13T22:59:18Z

I assume you know this but the use of verrou in our case is very specific (we are looking for quite specific problems involving dangerous floating point operations that could cause round-off error problems). This is what this 'linter' of sorts should be looking for.

premature commit and puhsing for testing

7bab395

Copilot AI review requested due to automatic review settings July 12, 2025 15:29

Malmahrouqi3 requested a review from sbryngelson as a code owner July 12, 2025 15:29

qodo-merge-pro bot added the Review effort 2/5 label Jul 12, 2025

Copilot AI reviewed Jul 12, 2025

View reviewed changes

Malmahrouqi3 and others added 5 commits July 12, 2025 11:31

Merge branch 'master' into verrou

e33e89b

few changes

3eac407

simplified ./configure

7d3eddb

improved Verrou setup

db69006

still improving ./configure

8254cda

Added MFC dependencies

97ff4df

sbryngelson marked this pull request as draft July 14, 2025 07:02

Malmahrouqi3 marked this pull request as ready for review July 17, 2025 23:41

qodo-merge-pro bot added Review effort 4/5 and removed Review effort 2/5 labels Jul 17, 2025

Merge remote-tracking branch 'upstream/master' into verrou

f0c2eb9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

`verrou` into Workflow #937

`verrou` into Workflow #937

Malmahrouqi3 commented Jul 12, 2025 •

edited by qodo-merge-pro bot

Loading

Uh oh!

qodo-merge-pro bot commented Jul 12, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

qodo-merge-pro bot commented Jul 12, 2025 •

edited

Loading

Uh oh!

Malmahrouqi3 commented Jul 13, 2025

Uh oh!

sbryngelson commented Jul 13, 2025

Uh oh!

Uh oh!

verrou into Workflow #937

Are you sure you want to change the base?

verrou into Workflow #937

Conversation

Malmahrouqi3 commented Jul 12, 2025 • edited by qodo-merge-pro bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Description

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

qodo-merge-pro bot commented Jul 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 97ff4df)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

qodo-merge-pro bot commented Jul 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

Malmahrouqi3 commented Jul 13, 2025

Uh oh!

sbryngelson commented Jul 13, 2025

Uh oh!

Uh oh!

`verrou` into Workflow #937

`verrou` into Workflow #937

Malmahrouqi3 commented Jul 12, 2025 •

edited by qodo-merge-pro bot

Loading

qodo-merge-pro bot commented Jul 12, 2025 •

edited

Loading

(Review updated until commit `97ff4df`)

qodo-merge-pro bot commented Jul 12, 2025 •

edited

Loading