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

Preparation work for Supporting matrix context in output keys #2477

Merged
merged 1 commit into from Mar 10, 2023

Conversation

ajaykn
Copy link
Contributor

@ajaykn ajaykn commented Mar 9, 2023

Please note that this is not supported yet, and Server side changes are still in-progess

we will release a changelog when supported end to end.

Currently outputs for matrix jobs are represented by a single output and only the last successful matrix job is used to populate the output. This prevents users from being able to target output from a specific job and can lead to unpredictable results since the matrix jobs don't run in a guaranteed order.

So as part of the improvements, we want to support
Allow the matrix context to be used to define the output variable name. This allows users to make the output completely deterministic if they choose to include all dimensions of the matrix in their key.

outputs:
  ${{ matrix.os }}_${{ matrix.arch }}_result: ${{ steps.step1.outputs.test }}

Example using all dimensions in output key:

jobs:
  job1:
    runs-on: ubuntu-latest
    strategy:
      matrix:
        os: ['linux', 'windows','macos']
        arch: ['x86','x64']
    outputs:
      ${{ matrix.os }}_${{ matrix.arch }}_result: ${{ steps.step1.outputs.test }}
    steps:
      - id: step1
        run: "test=${{ matrix.os }} - ${{ matrix.arch }}" >> $GITHUB_OUTPUT
  job2:
    runs-on: ubuntu-latest
    needs: job1
    steps:
      - run: echo ${{ needs.job1.outputs["linux_x86_result"] }}

Please note that: Server side changes are still in-progess.

@ajaykn ajaykn requested a review from a team as a code owner March 9, 2023 02:31
@0x53A
Copy link

0x53A commented Mar 13, 2023

hallelujah, this is great news! Thank you.

Please note that: Server side changes are still in-progess

is there a public issue we can watch?

@srosell-ut
Copy link

srosell-ut commented Mar 14, 2023

Hey @ajaykn 🖖 !
this is just what we were looking for but although it seems to be merged and deployed in the release v2.303.0 it seems to not work as the example shows. We've tried and we get the following error:

image

The workflow is not valid. .github/workflows/test.yaml (Line: 16, Col: 7): Unrecognized named-value: 'matrix'. Located at position 1 within expression: matrix.os .github/workflows/test.yaml (Line: 24, Col: 14): Unexpected symbol: '"linux_x86_result"'. Located at position 20 within expression: needs.job1.outputs["linux_x86_result"]

We just copy/paste you example as it was not working in our use case to see if we where doing something wrong but we have the same error.

Are we missing something? 🤔 I guess it is still something missing due to the side-note:

Please note that: Server side changes are still in-progess

Thanks for the work 🙇

@SvenStaehs
Copy link

SvenStaehs commented Mar 15, 2023

Love it, just started getting stuck on this issue. To my untrained eye, the changes look like they only affect job outputs, not workflow outputs (for reusable workflows where the workflows are called with a matrix), is that a separate change? Because trying the same syntax on those gives me Unexpected symbol: '"build-image-${{' trying to dereference in the workflow that needs this as an input.

Edit: I re-checked the example and noticed it uses a hard-coded string to resolve the output, not something like echo ${{ needs.job1.outputs["${{ matrix.os }}_${{ matrix.arch }}_result"] }}, which is what I need: My job2 runs on the same matrix as job1 and the respective second jobs are supposed to use the outputs of the respective first jobs that ran for the same matrix values... So maybe this implementation doesn't really cover my specific use-case completely?

@griseau
Copy link

griseau commented Mar 15, 2023

Such a great improvement ! Do you have any ETA for this ? So that I know if I need to make a workaround or can just wait !
Thanks a lot

@caio-eiq
Copy link

caio-eiq commented Mar 22, 2023

@ajaykn is this already working on v2.303.0?
I am running into a similar error as mentioned by @srosell-uz

@ajaykn
Copy link
Contributor Author

ajaykn commented Mar 23, 2023

@srosell-uz , @caio-eiq

Please note that there are still few more changes from server side.
We will release a changelog / blog post once we support end to end.

@griseau
Currently under internal testing and will be released in another 2-3 weeks.

@SvenStaehs

To my untrained eye, the changes look like they only affect job outputs, not workflow outputs (for reusable workflows where the workflows are called with a matrix), is that a separate change?

We wil share more in blog post once the feature is completely baked.
To give you initial idea, for reusable wf outputs, we were thinking to support via inputs context.

caller workflow
job1:
    strategy:
      matrix:
        os: ['linux', 'windows','macos']
        arch: ['x86','x64']
    uses: ./.github/workflows/called1.yml
    with:
      os: ${{ matrix.os }}
      arch: ${{ matrix.arch }} 
job2:
    runs-on: ubuntu-latest
    needs: job1
    steps:
      - run: echo ${{ needs.job1.outputs["linux_x86_result"] }}
called workflow
on:
  workflow_call:
    inputs:
      os:
        type: string
      arch:
        type: string
    outputs:
      ${{ inputs.os }}_${{ inputs.arch }}_result:
        value: ${{ jobs.job1.outputs.result}}

Edit: I re-checked the example and noticed it uses a hard-coded string to resolve the output, not something like echo ${{ needs.job1.outputs["${{ matrix.os }}_${{ matrix.arch }}_result"] }}, which is what I need: My job2 runs on the same matrix as job1 and the respective second jobs are supposed to use the outputs of the respective first jobs that ran for the same matrix values... So maybe this implementation doesn't really cover my specific use-case completely?

yes currently, we will not be supporting expressions inside an expression like ${{ needs.job1.outputs["${{ matrix.os }}_${{ matrix.arch }}_result"] }}

@ajaykn ajaykn changed the title Support matrix context in output keys Preparation work for Supporting matrix context in output keys Mar 23, 2023
@ChristopherHX
Copy link
Contributor

expressions inside an expression is not required to access composed output keys.

${{ needs.job1.outputs["${{ matrix.os }}_${{ matrix.arch }}_result"] }}

This can be rewritten to ${{ needs.job1.outputs[format('{0}_{1}_result', matrix.os, matrix.arch)] }}, only downside is that syntax is harder to write (and read) for workflow authors.

@brianjmurrell
Copy link

Could we get an update when the server-side bits have been completed and are live so that the change described here can work please?

(Note I am not asking for an ETA, just a note here when it's complete so that we know we can start using this change.)

@nicholasbergesen
Copy link
Contributor

Hi all, unfortunately the team working on this feature has been laid off. I have no information on whether or not the work will be continued by another team or if the work has stopped completely. Its quite sad as it was very close to completion.

cc @mph4

@brianjmurrell
Copy link

This is a real pity as this is a pretty serious feature gap. Not being able to get matrix per intersection outputs seriously impacts the usability of the matrix feature.

@qc00
Copy link

qc00 commented Apr 14, 2023

There's no design documentation linked, so I am not sure what alternative syntax was considered, but templating in the field name seems to be a bit limiting because the user of the outputs will then have to enumerate all the needed field names somehow.

This will either lead to a hard-coded, manual expansion of the matrix combinations, or some convoluted code to reuse the matrix definition.

Why not allow nested objects so we can use the existing [object filter] feature?

For example:

outputs:
  my_output:
    key: ${{matrix.os}}
    nested:
      key: ${{matrix.arch}}
      value: ...

# Usage:
${{jobs.x.outputs.my_output.*.*}}
${{jobs.x.outputs.my_output.linux.*}}
${{jobs.x.outputs.my_output.*.x64}}

The last two use cases are almost impossible with the current syntax

@MatthiasValvekens
Copy link

This will either lead to a hard-coded, manual expansion of the matrix combinations, or some convoluted code to reuse the matrix definition.

Wouldn't that be solvable with strategy.job-index as a unique identifier?

nikola-jokic pushed a commit to nikola-jokic/runner that referenced this pull request May 12, 2023
@andrewstec
Copy link

Can we please get an update on this? I understand the layoffs have occurred. Is there a PM/team who still has this in the product roadmap or is this indefinitely scrapped?

@RafPe
Copy link

RafPe commented May 30, 2023

Having this feature would make dynamic CICD with github actions a 💥 If there is any help we as community could contribute please let us know!

@patrickmoore-nc
Copy link

Just investigated parameterising a few similar jobs using matrix and I am in total disbelief this functionality wasn't provided from the get-go. It's something that every user of the matrix feature would obviously want.

@Zermond
Copy link

Zermond commented Jul 26, 2023

Does it work in cloud runner? Because I got error just like srosell-ut

@RafPe
Copy link

RafPe commented Jul 27, 2023

It will not work until Github pushes backend changes to support this 😢

@cdata
Copy link

cdata commented Aug 12, 2023

This change landed almost 6 months ago and there isn't any way to track progress on whatever other changes are blocking it. Can we get an update on the outlook for this feature? Or, perhaps, a reference to another issue to follow?

@RafPe
Copy link

RafPe commented Oct 19, 2023

I have taken a try to get the attention on Github by sharing a callout on LinkedIn platform! https://www.linkedin.com/posts/raftech_preparation-work-for-supporting-matrix-context-activity-7120734515211161600-12jD

Lets try to get their attention to this in every possible way. As automation/platform engineer this is a killer feature which would enable us to automate beyond what we got atm!

@elviskahoro
Copy link

We should loop in some of GitHub's developer advocates, especially given GitHub Universe starts tomorrow. Will try to get back with a list of folks

@shapirus
Copy link

is there a public issue we can watch?

I second this question.

@kishaningithub
Copy link

Given the importance of this i could not stop myself from posting this (meme below)

image

bennibbelink added a commit to cyclus/cyclus that referenced this pull request Jan 28, 2024
would have to jump through hoops to pass digest output between jobs given we have a testing matrix.
Github is still working on implementing the functionality - actions/runner#2477
bennibbelink added a commit to cyclus/cyclus that referenced this pull request Feb 7, 2024
would have to jump through hoops to pass digest output between jobs given we have a testing matrix.
Github is still working on implementing the functionality - actions/runner#2477
@zjplab
Copy link

zjplab commented Mar 5, 2024

Any new update so far on this topic?

@shinebayar-g
Copy link

No we're working on AI.

@RDhar
Copy link

RDhar commented Apr 4, 2024

While it's a bit niche, worth mentioning how Microsoft's other tool, Playwright (the E2E test runner w/ browser automation) handles the challenge of merging reports from multiple parallel tests.

Since they have native support for sharding (scaling parallel test execution by running tests on multiple machines simultaneously), they've shared a GH Actions example which leverages actions/upload-artifact and actions/download-artifact as follows:

jobs:
  run-tests:
    runs-on: ubuntu-latest
    strategy:
      fail-fast: false
      matrix:
        shardIndex: [1, 2, 3, 4]
        shardTotal: [4]

    steps:
    - name: Run Playwright tests
      run: npx playwright test --shard=${{ matrix.shardIndex }}/${{ matrix.shardTotal }}

    - name: Upload blob report to GitHub Actions Artifacts
      if: always()
      uses: actions/upload-artifact@v4
      with:
        name: blob-report-${{ matrix.shardIndex }}
        path: blob-report
        retention-days: 1

  merge-reports:
    # Merge reports after run-tests, even if some shards have failed
    if: always()
    needs: [run-tests]
    runs-on: ubuntu-latest

    steps:
    - uses: actions/checkout@v4
    - uses: actions/setup-node@v4
      with:
        node-version: 18
    - name: Install dependencies
      run: npm ci

    - name: Download blob reports from GitHub Actions Artifacts
      uses: actions/download-artifact@v4
      with:
        path: all-blob-reports
        pattern: blob-report-*
        merge-multiple: true

    - name: Merge into HTML Report
      run: npx playwright merge-reports --reporter html ./all-blob-reports 

    - name: Upload HTML report
      uses: actions/upload-artifact@v4
      with:
        name: html-report--attempt-${{ github.run_attempt }}
        path: playwright-report
        retention-days: 14

While the added HTML-report related steps are specific for Playwright's use-case, the merge-multiple: true line is the crux here for pulling together the partial blob reports together, as required.

It's not exactly straightforward by a long-shot, but it is an "officially" recommended method for merging outputs together cohesively from prior jobs running matrix strategy.

image

@SvenStaehs
Copy link

well that's a bit off-topic. Uploading multiple files to a single artifact was originally possible and was only broken with release v4 of upload-artifacts. So what they do with merge-multiple: true in that extra job is more like a workaround for this new restriction. Good to know this exists, so thanks for sharing.

But what we wanted to see from this PR was not about artifacts but about outputs: A job that runs in matrix and produces output variables -- e.g. the digest of the docker image it just created -- is currently useless (unless in specific circumstances where you don't care which job's output you consume because they're all going to be the same). So in the same way the upload-artifacts action accepts the input name: blob-report-${{ matrix.shardIndex }}, we wanted to be able to define a job-level output with matrix value in its name:

outputs:
  ${{ matrix.os }}_${{ matrix.arch }}_result: ${{ steps.step1.outputs.test }}

It has been suggested to use artifacts as a workaround for this: Instead of using output variables, write the values into a file that is then exposed as artifact. The file will have the matrix values in its name so you can load that file specifically and use the values it defines (could be a .env file). So this isn't entirely unrelated, but only as a matter of workarounds for workaround 😉

@RDhar
Copy link

RDhar commented Apr 5, 2024

I'm with you, @SvenStaehs, this is an (inconvenient) workaround that barely serves the purpose, at least until GH Actions actually begins to support multiple outputs from matrix jobs like this PR suggests it should.

Between this and passing secrets between jobs, I'm hoping for a lot from the Actions team! 🤞

rgildein added a commit to canonical/solutions-engineering-automation that referenced this pull request Apr 10, 2024
Since outputs do not support matrix strategy id deffinition (see [1])
I used artifact to store resuls and then collect them.

---
[1]: actions/runner#2477
rhigman added a commit to rhigman/thoth-dissemination that referenced this pull request Apr 17, 2024
rhigman added a commit to thoth-pub/thoth-dissemination that referenced this pull request May 7, 2024
* Use test version of Docker image which prints a value

* Replace uses: docker with run: docker

* Test passing output between jobs

* Fix: set `outputs` for first (passing) job

* Attempt to use matrix to reference outputs

* Can't nest actions variables references

* Test actions/runner/pull/2477 syntax (though it seems to be uncompleted)

* Try alternative syntax

* Can't use wildcard

* Can't reference full outputs mapping directly - try JSON

* Test whether variables can be used as output name at all

* Try using artifacts to pass matrix outputs between jobs

* Path is optional for download-artifact

* Only run secondary job for the platforms which we expect to return location info

* Output variable syntax no longer needed

* Not all platforms will produce output - let subsequent job handle it

* No relevant output should be created on failure

* Trailing spaces

* Misplaced comma

* No need for echo - echo will always succeed and overwrite actual disseminator exit code

* Tidy up

* No newline at end of file

* No need to append, just write

* We only expect output to be non-empty for the specified set of platforms
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet