Skip to content

Conversation

@Malmahrouqi3
Copy link
Collaborator

@Malmahrouqi3 Malmahrouqi3 commented Nov 10, 2025

User description

Description

Following on #1033,
This PR should be merged after pushing a finalized data.js (containing historic benchmark runs) to MFlowCode.github.io under documentation dir. Then #1033 will display whether bench_diff works as intended or not with the Self-Hosted Benchmark Tests (3 GT Phoenix, 1 ORNL Frontier).


PR Type

Enhancement


Description

  • Preserve existing benchmark data during documentation deployment

  • Prevent overwriting historical data.js in documentation repository

  • Ensure continuous benchmarking data persists across doc updates


Diagram Walkthrough

flowchart LR
  A["Docs workflow"] --> B["Clone documentation repo"]
  B --> C["Clear old files"]
  C --> D["Copy new docs"]
  D --> E["Stage all changes"]
  E --> F["Restore data.js"]
  F --> G["Commit and push"]
Loading

File Walkthrough

Relevant files
Enhancement
docs.yml
Restore benchmark data file during doc deployment               

.github/workflows/docs.yml

  • Added git restore documentation/data.js command after staging changes
  • Prevents historical benchmark data from being overwritten during doc
    deployment
  • Ensures data.js file is preserved from the remote repository
+1/-0     

Copilot AI review requested due to automatic review settings November 10, 2025 19:56
@qodo-merge-pro
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

The restore command assumes that a git-tracked 'documentation/data.js' exists in the target repo after removing everything with 'rm -rf ../www/*'. If the file isn't present or path differs, the restore will fail or silently leave it missing; consider restoring before wiping, or excluding the file from deletion.

if [ "$?" -ne "0" ]; then exit 0; fi
set -e
git config --global user.name  'MFC Action'
git config --global user.email '<>'
git clone "${{ secrets.DOC_PUSH_URL }}" ../www
rm -rf ../www/*
mv build/install/docs/mfc/* ../www/
git -C ../www add -A
git -C ../www restore documentation/data.js
git -C ../www commit -m "Docs @ ${GITHUB_SHA::7}" || true
git -C ../www push
Fragile Path

Hard-coding 'documentation/data.js' may break if the documentation structure changes; consider parameterizing the path or using a .gitignore/.gitattributes approach to persist history data.

if [ "$?" -ne "0" ]; then exit 0; fi
set -e
git config --global user.name  'MFC Action'
git config --global user.email '<>'
git clone "${{ secrets.DOC_PUSH_URL }}" ../www
rm -rf ../www/*
mv build/install/docs/mfc/* ../www/
git -C ../www add -A
git -C ../www restore documentation/data.js
git -C ../www commit -m "Docs @ ${GITHUB_SHA::7}" || true
git -C ../www push

Comment on lines 71 to +72
git -C ../www add -A
git -C ../www restore documentation/data.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Move the git restore command before git add -A to ensure changes to documentation/data.js are correctly discarded before being staged for commit. [possible issue, importance: 9]

Suggested change
git -C ../www add -A
git -C ../www restore documentation/data.js
git -C ../www restore documentation/data.js
git -C ../www add -A

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a safeguard to preserve historic benchmark data (documentation/data.js) during automated documentation deployments. It's a follow-up to #1033 that implements continuous benchmarking functionality.

Key Changes:

  • Adds a git restore command to prevent documentation/data.js from being overwritten during documentation builds

rm -rf ../www/*
mv build/install/docs/mfc/* ../www/
git -C ../www add -A
git -C ../www restore documentation/data.js
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

The git restore command will fail if documentation/data.js doesn't exist in the git history of the ../www repository (e.g., during initial setup or if the file hasn't been pushed yet). Since set -e is enabled on line 65, this will cause the workflow to fail.

Consider adding error handling to gracefully handle the case where the file doesn't exist:

git -C ../www restore documentation/data.js || true

Alternatively, check if the file exists in the git history before attempting to restore it:

git -C ../www ls-files --error-unmatch documentation/data.js &>/dev/null && git -C ../www restore documentation/data.js || true
Suggested change
git -C ../www restore documentation/data.js
git -C ../www restore documentation/data.js || true

Copilot uses AI. Check for mistakes.
git clone "${{ secrets.DOC_PUSH_URL }}" ../www
rm -rf ../www/*
mv build/install/docs/mfc/* ../www/
git -C ../www add -A
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider adding a comment to explain why documentation/data.js is being restored. Based on the PR description, this file contains historic benchmark data that should be preserved across documentation deployments. A brief comment would improve maintainability and help future developers understand this special handling.

Example:

# Preserve historic benchmark data from continuous benchmarking (see #1033)
git -C ../www restore documentation/data.js || true
Suggested change
git -C ../www add -A
git -C ../www add -A
# Preserve historic benchmark data from continuous benchmarking (see #1033)

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

1 participant