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: Update/maintain test-install GA workflow #5082

Merged
merged 4 commits into from
Aug 18, 2021

Conversation

csadorf
Copy link
Contributor

@csadorf csadorf commented Aug 16, 2021

Related to: #5046 (comment)

  • MAINTAIN: Add 'check-yaml' to pre-commit hooks.
    Should catch issues with the definition of workflow files in the future.
  • Dependencies: Fix the pyzmq requirement specification for Python 3.9.
    I have verified that the resolve-pip-dependencies job (see below) fails without this change.
  • CI: Implement resolve-pip-dependencies job.
    Checks whether environments defined in the requirements/ files are resolvable.
  • CI: Separate the direct conda installation test into separate job.
    The test whether we can actually install aiida with conda install aiida-core ... is run as a dedicated job only after we checked that we can create a conda environment from the environment.yml file.
  • CI: Run install-with-conda job for all supported Python versions.
    To test whether conda install aiida-core ... actually works for all supported Python versions.

Checklist before merge:

  • Restore deleted GA workflows.
  • Re-enable test suite

@csadorf csadorf force-pushed the ci/update-test-install-workflow branch 7 times, most recently from 3a8eb2f to 42393d4 Compare August 16, 2021 12:11
@csadorf csadorf added dependencies Pull requests that update a dependency file topic/continuous-integration topic/testing labels Aug 16, 2021
@csadorf csadorf requested a review from ltalirz August 16, 2021 12:21
@csadorf
Copy link
Contributor Author

csadorf commented Aug 16, 2021

@ltalirz The PR is currently in draft mode, because I have deactivated all unrelated GA runs to save on time and resources. I would appreciate if you could review the PR in draft mode so that all necessary changes and fixes can be implemented in an efficient manner without accidentally merging it. Once the PR is accepted, I will do one final run with all workflows restored before merge.

I recommend to not squash this PR on merge.

@csadorf
Copy link
Contributor Author

csadorf commented Aug 18, 2021

@ltalirz Let me know if I can help with the review.

Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

thanks @csadorf , looks good to me.

My only comment regards the job dependencies: while I get that this makes sense from a perspective of saving resources, does it lead to a slowdown in practice?

I guess the jobs don't actually depend on each other to run successfully but rather if one job fails, then we know that the others will fail as well?

I'm ok with both approaches; just curious to know whether the total runtime differs significantly (maybe the effect is anyhow small).

.github/workflows/test-install.yml Outdated Show resolved Hide resolved
@csadorf
Copy link
Contributor Author

csadorf commented Aug 18, 2021

thanks @csadorf , looks good to me.

My only comment regards the job dependencies: while I get that this makes sense from a perspective of saving resources, does it lead to a slowdown in practice?

I guess the jobs don't actually depend on each other to run successfully but rather if one job fails, then we know that the others will fail as well?

I'm ok with both approaches; just curious to know whether the total runtime differs significantly (maybe the effect is anyhow small).

Yes, the idea was to save some resources, but it is true that some of those jobs do not actually depend on each other in the way that it is configured, but rather belong to a "topical unit". I guess it would be better practice to remove those dependencies and run everything maximal parallel. I will rewire the dependencies and request another review.

@csadorf csadorf force-pushed the ci/update-test-install-workflow branch 2 times, most recently from d56ce15 to 6f032d2 Compare August 18, 2021 10:19
@csadorf
Copy link
Contributor Author

csadorf commented Aug 18, 2021

@ltalirz Job dependencies are rewired. The runtime (without actually running the test suite) has been slightly reduced, but I guess this depends on how many workers GitHub makes available to us at a given time.

https://github.com/aiidateam/aiida-core/actions/runs/1142730871

@csadorf csadorf requested a review from ltalirz August 18, 2021 10:28
@ltalirz
Copy link
Member

ltalirz commented Aug 18, 2021

thanks @csadorf - up to you how to handle the dependency situation, both is fine with me.

Feel free to bring back the rest and merge without further review from my side

This job checks whether the environments defined in the requirements/
files are resolvable.
…ons.

Verifies that aiida can be installed with conda for all supported
Python versions.
@csadorf csadorf force-pushed the ci/update-test-install-workflow branch from 6f032d2 to e38d636 Compare August 18, 2021 10:53
@codecov
Copy link

codecov bot commented Aug 18, 2021

Codecov Report

Merging #5082 (e38d636) into develop (ccd5795) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #5082   +/-   ##
========================================
  Coverage    80.50%   80.50%           
========================================
  Files          531      531           
  Lines        37020    37020           
========================================
  Hits         29799    29799           
  Misses        7221     7221           
Flag Coverage Δ
django 74.99% <ø> (ø)
sqlalchemy 73.93% <ø> (-<0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ccd5795...e38d636. Read the comment docs.

@csadorf csadorf marked this pull request as ready for review August 18, 2021 11:39
@csadorf csadorf merged commit c5e0118 into develop Aug 18, 2021
@csadorf csadorf deleted the ci/update-test-install-workflow branch August 18, 2021 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file topic/continuous-integration topic/testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants