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

ci: refactor julia/r/conda tests - now ~25 min instead of ~50 min #1188

Merged
merged 14 commits into from
Oct 18, 2022

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Oct 9, 2022

Previously the CI system took ~50 minutes. The tests in the folders tests/conda, tests/julia, and tests/r all took longer than 25 minutes but are now reduced to about ~25 minutes or less.

There has been very little compromise on existing test coverage, and new test coverage has been gained against new language versions etc.

Change description

Test changes

Changes to tests/conda tests

After Before Note
py35-binder-dir binder-dir py37 Makes an old version the oldest version, and merges in the binder directory test
py310-requirements-file py38 requirements Makes a less old version the latest version, and merges in the requirements file test
py2 simple-py2 Renamed
r-postbuild-file r-unspecified default-env simple r4.0 r4.1 Merges in the postBuild file test (default-env), merges tests of modern R versions into the latest version (r4.0, r4.1), merges tests of the default Python version (simple)
r3.6-target-repo-dir-flag repo-path r3.6 Merges extra-args.yml file test with r3.6 pinning test
- channel-dep A deleted test, testing functionality part of mamba since 0.6 introduced more than two years ago

Changes to tests/r tests

After Before Note
r3.6-mran r3.6 Renamed
r4.0-mran mran, r4.0 Merges similar tests
r4.0-rspm r4.0-rspm, r4.1 Merges r4.0 and r4.1,
r-rspm-apt-file apt, simple Merges in the apt.txt file test
r-rspm-description-file description-file Renamed

Changes to tests/julia tests

After Before Note
project julia_version-default Merges in the requirements file test
project-1.0.2 julia_version-1.0.2 Renamed
require julialegacy_version-0.6.3 The default version for require is 0.6.4, and can be used to test the oldest version.
require-1-requirements-file pyplot-requirements julialegacy_version-1 julialegacy_version-1.0 julialegacy_version-1.1 Merged similar tests

Before

image

After

image

@consideRatio consideRatio added the ci label Oct 9, 2022
@consideRatio consideRatio marked this pull request as draft October 9, 2022 11:44
@consideRatio consideRatio force-pushed the pr/ci-speedup branch 2 times, most recently from 7573682 to 91631b8 Compare October 9, 2022 12:41
@consideRatio consideRatio changed the title ci: refactor julia/r/conda tests for ci speedup ci: refactor julia/r/conda tests for 50 -> 30 minute ci execution times Oct 9, 2022
@consideRatio consideRatio changed the title ci: refactor julia/r/conda tests for 50 -> 30 minute ci execution times ci: refactor julia/r/conda tests for 50->30 minutes ci execution times Oct 9, 2022
@consideRatio consideRatio force-pushed the pr/ci-speedup branch 2 times, most recently from 4743c24 to 51e3b8e Compare October 9, 2022 13:45
@manics
Copy link
Member

manics commented Oct 9, 2022

To help with review would you mind explaining the overall thoughts behind your changes? For example, what's your thinking behind bumping versions instead of leaving them as they are?

I think we should keep intermediate versions, for example an old bug was included installing the wrong version of R when 4.0 was requested #1077 so this should be kept as a regression test.

@consideRatio consideRatio force-pushed the pr/ci-speedup branch 2 times, most recently from 4adf61f to fde068b Compare October 9, 2022 15:31
@consideRatio consideRatio changed the title ci: refactor julia/r/conda tests for 50->30 minutes ci execution times ci: refactor julia/r/conda tests to go from ~50 to ~25 minutes test feedback Oct 9, 2022
@consideRatio
Copy link
Member Author

@manics I've updated the PR description with a declaration of the principles that has guided me when refactoring this! I think we still cover #1077 in the R buildback by testing both before (mran) and after (rspm) situations.

@consideRatio consideRatio changed the title ci: refactor julia/r/conda tests to go from ~50 to ~25 minutes test feedback ci: refactor julia/r/conda tests to go from ~50 to ~30 minutes test feedback Oct 9, 2022
@consideRatio consideRatio marked this pull request as ready for review October 9, 2022 16:03
@consideRatio consideRatio added the maintenance Under the hood fixes and improvements label Oct 9, 2022
@consideRatio consideRatio changed the title ci: refactor julia/r/conda tests to go from ~50 to ~30 minutes test feedback ci/maint: refactor julia/r/conda tests to go from ~50 to ~30 minutes test feedback Oct 9, 2022
@consideRatio consideRatio force-pushed the pr/ci-speedup branch 3 times, most recently from 8c8b7e2 to 31e7839 Compare October 9, 2022 19:16
@consideRatio consideRatio changed the title ci/maint: refactor julia/r/conda tests to go from ~50 to ~30 minutes test feedback ci/maint: refactor julia/r/conda tests to go from ~50 to ~25 minutes test feedback Oct 9, 2022
@consideRatio consideRatio changed the title ci/maint: refactor julia/r/conda tests to go from ~50 to ~25 minutes test feedback ci/maint: refactor julia/r/conda tests - now ~25 min instead of ~50 min Oct 9, 2022
@consideRatio consideRatio removed the maintenance Under the hood fixes and improvements label Oct 10, 2022
@consideRatio consideRatio changed the title ci/maint: refactor julia/r/conda tests - now ~25 min instead of ~50 min ci: refactor julia/r/conda tests - now ~25 min instead of ~50 min Oct 10, 2022
@consideRatio consideRatio force-pushed the pr/ci-speedup branch 2 times, most recently from 4ef5718 to dabd5b7 Compare October 10, 2022 01:20
@consideRatio consideRatio force-pushed the pr/ci-speedup branch 3 times, most recently from fa68da7 to be675e6 Compare October 10, 2022 08:21
@consideRatio
Copy link
Member Author

Sorry for the noise in this PR @manics @yuvipanda, with a lot of force pushes and too early transition from a draft PR to a ready for review PR.

At this point, it is ready for review though!

@manics
Copy link
Member

manics commented Oct 14, 2022

Thanks for the additional description- it makes sense, though I'm still struggling to review the actual changes. I'm also concerned we're going to loose some of the information on the purposes of individual tests, which is a problem for any future refactoring.

I noticed you added a few READMEs in some tests, what do you think about adding READMEs for each group of tests instead/as well, e.g.

tests/conda/README.md
tests/julia/README.md
tests/r/README.md

where each file describes how the subtests in that directory combines multiple factors, such that they cover all combinations?

@manics manics mentioned this pull request Oct 14, 2022
1 task
I misunderstood extra-args.yaml to be a file that was recognized by
repo2docker's CLI, but in reality it was just a file recognized by the
repo2docker test suite. Following that, I renamed this testing specific
file to test-extra-args.yaml to help others avoid making that mistake
and added comments to clarify that.

This commit also gathers the conda buildpacks separate readme files into
a single readme file.
@consideRatio
Copy link
Member Author

I'm still struggling to review the actual changes.

I'm sorry about this, if you have a suggestion on what I can do to help let me know. I have considered breaking apart this refactoring PR into pieces such as one for the conda buildpacks tests, and one for julia, and one for R. Doing that though would make common aspects need to be discussed in separate PRs, such as the README suggestion etc and I figure it wouldn't be very helpful.

I'm also concerned we're going to loose some of the information on the purposes of individual tests, which is a problem for any future refactoring.

Having worked with these files many hours now, I'm quite confident this is an improvement with regards to future refactoring work. The git history will be obfuscated by this refactor, but the documentation surrounding the tests will be accurate and updated. I've been surprised several times by un

I noticed you added a few READMEs in some tests, what do you think about adding READMEs for each group of tests instead/as well, e.g.

tests/conda/README.md
tests/julia/README.md
tests/r/README.md

where each file describes how the subtests in that directory combines multiple factors, such that they cover all combinations?

💯 implemented now!

Copy link
Member

@manics manics left a comment

Choose a reason for hiding this comment

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

Thanks for the additional README, it's a big help!

I'm happy with this now, @yuvipanda what do you think?

- Test setup of a R 3.6 environment by specifying `r-base=3.6` in
`environment.yml`.

- Test use of repo2docker with the `--target-repo-dir` flag.
Copy link
Member

Choose a reason for hiding this comment

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

Now that we're combining buildpack test cases I think we should avoid mixing in command line test cases too, as it adds another dimension. This can be left for a follow-up PR though.

@yuvipanda
Copy link
Collaborator

LET'S GOOOOO! THANK YOU SO MUCH @manics AND @consideRatio!

@yuvipanda yuvipanda merged commit 380f817 into jupyterhub:main Oct 18, 2022
@yuvipanda
Copy link
Collaborator

let it be known that I tried to upper case your usernames too ^ but github autocorrected them

manics added a commit to manics/repo2docker that referenced this pull request Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants