Fixes #2 Caching does not work#4
Conversation
* Initial plan * Extract environment setup into standalone composite GitHub Action Co-authored-by: Yuri05 <25061876+Yuri05@users.noreply.github.com> * Update action.yml * Update create-evaluation_reports.yml * Update create-qualification_reports.yml * Update action.yml * Update action.yml * Update .github/workflows/create-evaluation_reports.yml Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update .github/workflows/create-qualification_reports.yml Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update .github/workflows/create-evaluation_reports.yml Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update action.yml * Update create-evaluation_reports.yml * Update create-qualification_reports.yml * Update action.yml --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Yuri05 <25061876+Yuri05@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Update action.yml * Update action.yml * Update action.yml
📝 WalkthroughWalkthroughIntroduces a new composite GitHub Action that consolidates R, PK-Sim, and QualificationRunner environment setup with caching logic, then simplifies two workflows to use this action instead of their previous multi-step setup sequences. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.github/actions/setup-qualification-environment/action.yml (3)
8-11: Hardcodedtools.csvpath reduces reusability.The
tools-pathis hardcoded totools.csvin both the cache key computation (line 16) and the remote action call (line 60), but this composite action doesn't declare anyinputs. If you plan to move this to the shared Workflows repo, consider adding atools-pathinput with a default oftools.csvnow to ease that migration.Example: declare an input
name: 'Setup Qualification Environment' description: 'Sets up the qualification environment (R, PK-Sim, QualificationRunner) with caching support' +inputs: + tools-path: + description: 'Path to the tools.csv file' + required: false + default: 'tools.csv' + runs: using: composite steps:Then replace
tools.csvreferences with${{ inputs.tools-path }}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/setup-qualification-environment/action.yml around lines 8 - 11, Add a new action input named tools-path (default "tools.csv") to the action.yml inputs section and update all hardcoded references to "tools.csv" (used in the cache key computation and the remote action call) to use the input expression ${{ inputs.tools-path }} instead; ensure the input is documented in the action metadata so callers can override it and verify both the cache key generation and the remote action invocation now reference the new tools-path input rather than the literal filename.
13-17: Cache key includesR_LIBS_USERpath — consider whether this adds value.
R_LIBS_USERresolves to${RUNNER_TEMP}\Library, which is a fixed path on GitHub-hostedwindows-2022runners (typicallyD:\a\_temp\Library). Including it in the cache key means the key is stable as long as GitHub doesn't change the runner's temp path. If the intent is to differentiate by runner type, consider using the runner OS label explicitly instead, which is more readable and intentional:echo "CACHE_KEY=ospsuite-windows-2022-${{ hashFiles('tools.csv') }}" >> $GITHUB_ENVThat said, the current approach works — this is just a readability suggestion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/setup-qualification-environment/action.yml around lines 13 - 17, The CACHE_KEY currently embeds env.R_LIBS_USER which expands to a runner-specific temp path; update the action.yml step that sets CACHE_KEY so it uses an explicit, readable runner identifier (e.g., the OS label) instead of ${ env.R_LIBS_USER }—locate the lines that write CACHE_KEY (the echo assigning CACHE_KEY=ospsuite-${{ env.R_LIBS_USER }}-${{ hashFiles('tools.csv') }}) and replace the env.R_LIBS_USER portion with a fixed runner token like the OS label (or another intentional discriminator) while keeping the hashFiles('tools.csv') part intact.
55-60: Naming overlap with the remote action may cause confusion.The local composite action (
.github/actions/setup-qualification-environment) has the same directory name as the remote action it delegates to on cache miss (Open-Systems-Pharmacology/Workflows/.github/actions/setup-qualification-environment@main). While this works correctly because they're in different repos, it makes it harder for maintainers to tell them apart at a glance. The PR description notes this is "intended to be moved later to the Workflows repo" — consider adding a comment here clarifying the relationship (e.g., "delegates to the upstream action that performs the actual setup").Also, if the remote action at
Workflowsrepo ever adds its own caching layer, this would result in double-caching. Worth keeping in mind during the planned migration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/setup-qualification-environment/action.yml around lines 55 - 60, The composite action step with id "qualification-environment" currently uses the remote action "Open-Systems-Pharmacology/Workflows/.github/actions/setup-qualification-environment@main" which shares the same directory name as the local composite action and can confuse maintainers; update the step to include a short inline comment clarifying that this local composite delegates to the upstream action (e.g., "delegates to upstream Workflows action that performs actual setup") and optionally rename the local action directory or the step id to avoid name collision; also add a TODO note near the if-cache logic warning about potential double-caching if the upstream action later implements caching.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/actions/setup-qualification-environment/action.yml:
- Around line 8-11: Add a new action input named tools-path (default
"tools.csv") to the action.yml inputs section and update all hardcoded
references to "tools.csv" (used in the cache key computation and the remote
action call) to use the input expression ${{ inputs.tools-path }} instead;
ensure the input is documented in the action metadata so callers can override it
and verify both the cache key generation and the remote action invocation now
reference the new tools-path input rather than the literal filename.
- Around line 13-17: The CACHE_KEY currently embeds env.R_LIBS_USER which
expands to a runner-specific temp path; update the action.yml step that sets
CACHE_KEY so it uses an explicit, readable runner identifier (e.g., the OS
label) instead of ${ env.R_LIBS_USER }—locate the lines that write CACHE_KEY
(the echo assigning CACHE_KEY=ospsuite-${{ env.R_LIBS_USER }}-${{
hashFiles('tools.csv') }}) and replace the env.R_LIBS_USER portion with a fixed
runner token like the OS label (or another intentional discriminator) while
keeping the hashFiles('tools.csv') part intact.
- Around line 55-60: The composite action step with id
"qualification-environment" currently uses the remote action
"Open-Systems-Pharmacology/Workflows/.github/actions/setup-qualification-environment@main"
which shares the same directory name as the local composite action and can
confuse maintainers; update the step to include a short inline comment
clarifying that this local composite delegates to the upstream action (e.g.,
"delegates to upstream Workflows action that performs actual setup") and
optionally rename the local action directory or the step id to avoid name
collision; also add a TODO note near the if-cache logic warning about potential
double-caching if the upstream action later implements caching.
| # Save the prepared environment so subsequent runs can skip setup | ||
| - name: Save setup cache | ||
| if: steps.cache-setup.outputs.cache-hit != 'true' | ||
| continue-on-error: true | ||
| uses: actions/cache/save@v5 | ||
| with: | ||
| key: ${{ env.CACHE_KEY }} | ||
| path: | | ||
| ${{ env.R_LIBS_USER }} | ||
| ${{ github.workspace }}/PK-Sim | ||
| ${{ github.workspace }}/QualificationRunner |
There was a problem hiding this comment.
When executing a workflow with >= 2 models (resp. >= 2 qualifications) for the first time from a branch (so that no cache exists yet) - each of these parallel jobs will try to save the cache with the same cache key.
N-1 jobs will not be able to do this because the cache was already created by the 1st job. Which is OK and can be safely ignored. So adding continue-on-error: true here.
The job will then produce a warning like below and continue:
| - name: Configure git, set cache key | ||
| run: | | ||
| git config --global core.longpaths true | ||
| echo "CACHE_KEY=ospsuite-${{ env.R_LIBS_USER }}-${{ hashFiles('tools.csv') }}" >> $GITHUB_ENV |
There was a problem hiding this comment.
Defined the variable R_LIBS_USER in the previous step and not here, because the value of the environment GHA variable can be evaluated only in the next GHA step (pretty weird)
2 participants
Extracted the common "Setup OSP Infrastructure" part from the 2 workflows (should be moved later to the Workflows repo).
Fixes #2 Caching does not work.
Now when executed for the 1st time from a new branch (or after changing of tools.csv configuration) - a new cache is created and used in the subsequent runs from this branch (saves 6-7 minutes per qualification run).
Summary by CodeRabbit