Run profiling on comment-trigger, and cron

[willGraham01]

Updates the profiling workflow to:

• Run as a cron job once a week (Sundays at 00:00).
• Be triggered by comments on pull-requests.

Updates the run-on-comment workflow:

• Can be optionally passed a directory and tag to upload as an artifact after the run completes.

Comment Triggering

Profiling can be triggered by commenting on a PR with

/run profiling

provided the commenting user has the necessary permissions to trigger a workflow run.

Our current comment-triggered workflow was only capable of running shell commands, and for the profiling runs we needed to trigger a workflow (since running the profiling alone is not enough, as we need to export and upload the artifacts).

The solution suggested below is to adapt run-on-comment to allow export of an artifact. This means that, on comment-triggered runs we perform the profile-on-comment job, which passes the shell commands needed to trigger profiling to the run-on-comment workflow and requests an artifact to be produced with the profiling results. Conversely on workflows not triggered by comments, we can just run an alternative job (profile-on-dispatch) with the profiling commands and then upload an artifact with the same naming convention. The workflow will then check that at least one of profile-on-comment or profile-on-dispatch succeeded, and then push the results to the profiling repository and trigger a rebuild of the results website.

[matt-graham]

Making run-profiling.yaml define a reusable workflow seems like a good idea, but I wonder if we can somehow avoid the duplication of comment triggered workflow code by abstracting the resuable workflow in run-on-comment.yaml rather than partially copying and pasting.

I think we could either go the route of adding the option of uploading artifacts from a comment triggered workflow or allow optionally calling a composite action instead of running a shell command from the comment triggered workflow.

For the former I think the easiest route would be to factor out the upload-results workflow job in to a reusable workflow and then have two workflow files calling into the reusable workflows - comment-triggered-profiling.yaml with contents something like

on:
  issue_comment:
    types:
      - created
jobs:
  run-profiling:
    uses: ./.github/workflows/run-on-comment.yml
    with:
      runs-on: self-hosted
      keyword: profiling
      commands: |
          UNIQUE_ID=${GITHUB_EVENT_NAME}_${GITHUB_RUN_NUMBER}_${GITHUB_SHA}
          tox -vv -e profile -- --output_name ${UNIQUE_ID}
      description: Profiled run of model
      timeout-minutes: 8640
      application-organization: UCL
      artifact-tag: profiling
      artifact-path: profiling_results
      artifact-retention-days: 1
    secrets:
      application-id: ${{ secrets.COMMENT_BOT_APP_ID }}
      application-private-key: ${{ secrets.COMMENT_BOT_APP_PRIVATE_KEY }}
  upload-results:
    uses: ./github/workflows/upload-profiling-results.yml
    needs: run-profiling
    with:
      run-number: ${{ github.run-number }}
    secrets:
      profiling-repo-access-token: ${{ secrets.PROFILING_REPO_ACCESS }}

and scheduled-and-workflow-dispatch-profiling.yaml with contents something like

on:
  workflow_dispatch:
  schedule:
    - cron: 0 0 * * 6
jobs:
  run-profiling:
    runs-on: self-hosted
    steps:
      - name: Checkout repository
        uses: actions/checkout@v3
      - name: Run profiling
        run: |
          UNIQUE_ID=${GITHUB_EVENT_NAME}_${GITHUB_RUN_NUMBER}_${GITHUB_SHA}
          tox -vv -e profile -- --output_name ${UNIQUE_ID}
      - name: Save results as artifact
        uses: actions/upload-artifact@v3
        with:
          name: profiling_${{ github.run-number }}
          path: profiling_results/
          retention-days: 1
  upload-results:
    uses: ./github/workflows/upload-profiling-results.yml
    needs: run-profiling
    with:
      run-number: ${{ github.run-number }}
    secrets:
      profiling-repo-access-token: ${{ secrets.PROFILING_REPO_ACCESS }}

This would still duplicate the profiling run command and logic around naming of artifact, but would be less repetition overall.

Not sure that this is better than your proposed solution given extra complexity but thought I'd mention as something to discuss!

[willGraham01]

but I wonder if we can somehow avoid the duplication of comment triggered workflow code by abstracting the resuable workflow in run-on-comment.yaml rather than partially copying and pasting.

Yeah I'm definitely not going to defend that the copy/paste was the best design choice on my part. 😅

I think I like option 1: we give run-on-comment some additional (optional) inputs; artifact-name, artifact-path, etc, and an additional (optional) extra step right at the end that uploads the artifact. We can abstract the upload-profiling-results to another workflow as you say too.

Then we only actually need one workflow file for the profiling itself, unless you'd prefer to explicitly put comment-triggered and non-comment-triggered into their own separate workflow files:

name: Run Profiling

on:
  workflow_dispatch:
  # Allow profiling to be triggered by comments on pull requests
  # Trigger is /run profiling
  issue_comment:
    types:
      - created
  # Profile the model every Sunday at 00:00, 
  # on the HEAD of master
  schedule:
    - cron: 0 0 * * 6

jobs:
  profile-on-comment:
    uses: ./.github/workflows/run-on-comment.yml
    if: github.event_name == 'issue_comment'
    # what you have above, passing to run-on-comment.yml, 
    # with artifact upload option enabled
  
  profile-on-dispatch:
    if: github.event_name != 'issue_comment'
    # explicit tox call, followed by upload step
  
  upload-profiling-results:
    needs: [profile-on-dispatch or profile-on-comment] # TODO: check syntax!
    uses: ./.github/workflows/upload-results.yaml
    # suitable upload workflow that runs on ubuntu-latest, 
    # and fetches the artifact that was created by whichever job above was actually performed

In fact, I don't think we'd even to factor out upload-results into another workflow here, as we'd only be calling it from within this file (unless again, you think having separate workflows for the two profiling triggers would be better)?

[matt-graham]

Then we only actually need one workflow file for the profiling itself, unless you'd prefer to explicitly put comment-triggered and non-comment-triggered into their own separate workflow files:
...
In fact, I don't think we'd even to factor out upload-results into another workflow here, as we'd only be calling it from within this file (unless again, you think having separate workflows for the two profiling triggers would be better)?

Agree that what you propose here would be better but I think it's not possible for a step to depend on one of a set of previous steps rather than all of set of previous steps that is I don't think the

needs: [profile-on-dispatch or profile-on-comment] # TODO: check syntax!

syntax exists - it looks like we can only specify an intersection not a union.

We could potentially have two jobs which each depend on just one of the previous steps, but then we get back to duplicating code unless we factor out to a reusable workflow. It might be possible to do something similar to needs with an if conditional, but I'm not sure.

[willGraham01]

Agree that what you propose here would be better but I think it's not possible for a step to depend on one of a set of previous steps rather than all of set of previous steps

We might be able to get around this with a slight hack (I think brainglobe uses this hack for something similar but would need to double check) - we have an intermediate job/step that checks that at least one of profile-on-comment or profile-on-dispatch ran successfully. Something like

upload-profiling-results:
  if: |
    jobs.profile-on-comment.result == 'success' || jobs.profile-on-dispatch.result == 'success'
  uses: ./.github/workflows/upload-results.yaml
  # suitable upload workflow that runs on ubuntu-latest, 
  # and fetches the artifact that was created by whichever job above was actually performed

StackOverflow reference about doing this

Confirming this works in sandbox

[matt-graham]

Thanks @willGraham01 this is looking great. Have suggested a few possible minor naming changes and a small typo fix in one key but otherwise looks almost ready to merge.

[matt-graham]

Looks great - thanks for all your work on this @willGraham01 and for putting up with my overly long comments 😁. One small change needed I think to ensure all internal references to input names are consistent but otherwise looks good to merge.

[matt-graham]

@tamuri did you also want to review this? From my perspective this is good to merge.