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

Adding cache test WDL and GitHub Action #66

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

tefirman
Copy link
Collaborator

@tefirman tefirman commented Jan 16, 2025

Description

  • Call-caching is only triggered during multiple calls of the same workflow
  • Adding a new GitHub action to call a very simple workflow twice
  • Custom Cromwell configuration is necessary to enable call-caching
  • Action checks the Cromwell logs of the second run to ensure call-caching was performed
  • Third run uses slightly different inputs to ensure that call-caching is not used in that case
  • Action again checks Cromwell logs to confirm this for the third run.

Related Issues

Testing

  • Executed GitHub Action on this branch multiple times, works as expected.

@tefirman tefirman linked an issue Jan 16, 2025 that may be closed by this pull request
@tefirman
Copy link
Collaborator Author

Added @sitapriyamoorthi for review of the Cromwell/WDL aspects.
Added @sckott for review of the GitHub Action yml.
Tagging @seankross for visibility.

If we feel like this shouldn't be its own GitHub Action, we can definitely explore other options, just let me know.

@tefirman tefirman added unit test Adding or modifying a unit test infrastructure Infrastructure fix to execute WDL GitHub Actions labels Jan 16, 2025
Copy link
Collaborator

@sckott sckott left a comment

Choose a reason for hiding this comment

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

looking great


- name: Download Cromwell
run: |
wget https://github.com/broadinstitute/cromwell/releases/download/86/cromwell-86.jar
Copy link
Collaborator

Choose a reason for hiding this comment

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

use cromwell-87 here and in other places in this file - per discussion in slack about using the same version proof is using

fi

# Verify second run was significantly faster
if [ ${{ steps.second-run.outputs.second_duration }} -gt $(( ${{ steps.first-run.outputs.first_duration }} / 2 )) ]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

In your experience thus far is the timing variable? That is, is this math safe enough that even with variable load on the clusters that this will succeed? Same comment for next if block


## Version
- WDL 1.0
- Cromwell 86
Copy link
Collaborator

Choose a reason for hiding this comment

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

86 -> 87

exit 1
fi

echo "Cache validation passed!"
Copy link
Collaborator

Choose a reason for hiding this comment

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

should there be an exit 0 here? or does it succeed without that explicitly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Infrastructure fix to execute WDL GitHub Actions unit test Adding or modifying a unit test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Caching with identical inputs Cache invalidation on environmental changes
2 participants