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

(WIP) Test batch_processing.sh in CI #3196

Closed
wants to merge 13 commits into from

Conversation

joshuacwnewton
Copy link
Member

Just a quick WIP to test:

  • Running batch_processing using CI
  • Uploading the resulting csv files using the upload-artifact GH action

Don't review, etc. I'll write a proper description when ready. :)

@joshuacwnewton joshuacwnewton force-pushed the jn/2888-test-batch_processing.sh branch 11 times, most recently from 25c58d7 to a0d9b84 Compare January 25, 2021 00:26
@joshuacwnewton
Copy link
Member Author

joshuacwnewton commented Jan 25, 2021

After some finagling, I've gotten the upload-artifact action to upload the 5 csv files produced by batch_processing.sh.

Now, in theory, I should be able to fetch these using the download-artifact action so that I can make a comparison in a test. :)

@joshuacwnewton joshuacwnewton linked an issue Jan 25, 2021 that may be closed by this pull request
3 tasks
Also, re-enable a check that only uploads new results on merge. It
was disabled only to upload the very first set of results.
@joshuacwnewton
Copy link
Member Author

Welp, that's disappointing. ☹️

Downloading files: You can only download artifacts that were uploaded during the same workflow run. When you download a file, you can reference it by name.

That conflicts with what I expected, given this sentence on the same page:

For example, you can use artifacts to save your build and test output after a workflow run has ended.

I'll have to find another way. :(

@joshuacwnewton
Copy link
Member Author

joshuacwnewton commented Jan 25, 2021

The strange thing to me is, the retention period for artifacts is 90 days. Why would they store something for 90 days if you can't reuse it outside of a single workflow run?

@joshuacwnewton
Copy link
Member Author

joshuacwnewton commented Jan 25, 2021

Interesting...

joshua@XPS-15-9560:~$ curl   -H "Accept: application/vnd.github.v3+json"   https://api.github.com/repos/neuropoly/spinalcordtoolbox/actions/artifacts
{
  "total_count": 1,
  "artifacts": [
    {
      "id": 36987543,
      "node_id": "MDg6QXJ0aWZhY3QzNjk4NzU0Mw==",
      "name": "batch-processing-results",
      "size_in_bytes": 6316,
      "url": "https://api.github.com/repos/neuropoly/spinalcordtoolbox/actions/artifacts/36987543",
      "archive_download_url": "https://api.github.com/repos/neuropoly/spinalcordtoolbox/actions/artifacts/36987543/zip",
      "expired": false,
      "created_at": "2021-01-25T00:52:39Z",
      "updated_at": "2021-01-25T00:52:39Z",
      "expires_at": "2021-04-25T00:52:35Z"
    }
  ]
}

So, despite download-artifact seemingly not working in my CI test, the csv files still get stored, and it looks like you can access them via the REST API. Weird. Why does download-artifact have a limitation, then?

No matter. Looks like there's still hope!

@joshuacwnewton
Copy link
Member Author

joshuacwnewton commented Jan 25, 2021

Alrighty, figured out how to download the artifact using curl:

curl -u "$USER:$ACCESS_TOKEN" -H "Accept: application/vnd.github.v3+json" -L https://api.github.com/repos/neuropoly/spinalcordtoolbox/actions/artifacts/36987543/zip --output test.zip

Screenshot from 2021-01-24 22-09-14

The only quirk is the $USER:$ACCESS_TOKEN part. The documentation says "Anyone with read access to the repository can use this endpoint", but I'm hoping that the GITHUB_TOKEN secret can be substituted instead. This seems reasonable, given that they even include a REST API example.

Other than that, the steps are quite simple:

  • Fetch the list of artifacts using a GET request
  • Filter the response to find the most recent artifact
  • Download that artifact using another GET request
  • Use the resulting CSV files in a test

Value comparison logic will come in a future commit.
@joshuacwnewton joshuacwnewton force-pushed the jn/2888-test-batch_processing.sh branch 4 times, most recently from 117dcf5 to ec3c485 Compare January 25, 2021 17:08
@joshuacwnewton
Copy link
Member Author

Alrighty! I've done all the experimenting I wanted to do, and everything looks good to me! I'm going to close this, then open a separate PR with all of the commits cleaned up + an in-depth description. :)

@joshuacwnewton
Copy link
Member Author

Just kidding -- there's one more test I want to do. There are a few workarounds in actions/runner#241 and pytest-dev/pytest#7443 to enable color output in pytest. I think this would be really nice for improving the readability of test results.

@joshuacwnewton
Copy link
Member Author

Splendid! Pretty colour: https://github.com/neuropoly/spinalcordtoolbox/runs/1764465154?check_suite_focus=true

Now I'm closing this. Expected a much neater PR soon.

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

1 participant