Split raiwidgets/e2e-notebooks test pipeline in two #1478
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…olbox into romanlutz/split_pipeline
Codecov Report
@@ Coverage Diff @@
## main #1478 +/- ##
=======================================
Coverage 87.56% 87.56%
=======================================
Files 108 108
Lines 5081 5081
=======================================
Hits 4449 4449
Misses 632 632
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
gaugup
reviewed
Jun 9, 2022
2 similar comments
xuke444
approved these changes
Jun 10, 2022
…olbox into romanlutz/split_pipeline
imatiach-msft
approved these changes
Jun 10, 2022
1 similar comment
gaugup
reviewed
Jun 10, 2022
…olbox into romanlutz/split_pipeline
gaugup
approved these changes
Jun 14, 2022
…olbox into romanlutz/split_pipeline
vinuthakaranth
approved these changes
Jun 14, 2022
…olbox into romanlutz/split_pipeline
3 tasks
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The existing pipeline
.github\workflows\Ci-raiwigets-python-typescript.ymlserves two distinct purposes:They are in one pipeline since they both require
yarn install,yarn buildall, and the subsequent installation ofraiwidgets. However, this pipeline has become the most time-consuming of all of our pipelines and it will only get worse in the future as we add more notebooks and more test cases. For that reason, this PR splits them into two and prunes unnecessary steps (e.g., installing torch for the pytest tests). The e2e-notebook pipeline isn't run on macos-latest which also removed a lot of custom parts. So far, this was just handled viaifstatements.Beyond that, the e2e-notebook pipeline is also broken down into each individual notebook using the matrix strategy. This required extending
e2e-widget.jsto take additional input params-nfor the notebook name and--skipgento skip notebook generation which considerably speeds up the local test iterations. In this new setup, the notebook names need to match the test folders so I renamed some of the test folders to match the notebook names.Furthermore, this now does
yarn installandyarn buildallin a single workflow, stores the output as an artifact and installsraiwidgetsusing this artifact in the individual workflows. This means we're running theyarn buildallpart only once instead ofntimes. It often takes up to 30 minutes for just that part, so the savings should be considerable.This change was originally part of #1462 but the PR got too big. In that PR, I'm adding e2e notebook tests for flighted versions of our dashboard. This alone doubles the test suite and made it run out of memory on the node that ran it. Splitting it up was the reasonable adjustment, but we felt like it makes sense to go a step further as we'll be adding more notebooks in the future (presumably).
Checklist