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

Github Actions CI for testing GMT master branch #485

Merged
merged 37 commits into from
Jul 22, 2020
Merged

Conversation

weiji14
Copy link
Member

@weiji14 weiji14 commented Jun 22, 2020

Description of proposed changes

Running Continuous Integration tests on the gmt-master branch. Based on code I had lying around on my computer from May 2020 (tested locally on https://github.com/nektos/act/), but updated with stuff from #475 et al.

This is similar to #462, but on Github Actions (instead of Azure Pipelines), and currently only on Ubuntu and macOS only (may try on Windows someday).

Fixes #345, Closes #462.

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to docstrings or tutorials.

@seisman
Copy link
Member

seisman commented Jun 22, 2020

I'm thinking if it's easier and more cross-platform if we build the gmt master branch via conda-build and then install it, i.e.,

  1. Clone the gmt-feedstock. Only the files in the recipe directory are needed
  2. Update the source section in the meta.yml file to use the master branch source from git
  3. Build the gmt package via conda build recipe -c conda-forge.
  4. Install the local conda package via conda install gmt-xxxx.tar.gz.

@weiji14
Copy link
Member Author

weiji14 commented Jun 22, 2020

That's a smart idea. But we might as well make gmt-nightly builds a thing and pull that (and users who want the latest bugfix can benefit).

The Rapids AI organization has a good documented example, see e.g. cudf https://anaconda.org/rapidsai-nightly/cudf/files and blog post at https://medium.com/rapids-ai/in-the-heat-of-the-nightlies-6ed3be57ac64. I seem to recall they designed the build so it only happens if there's been commits, so no compute time is wasted. I'll open an issue for that later.

@seisman
Copy link
Member

seisman commented Jun 22, 2020

Do you mean a separate gmt-nightly feedstock?

We actually have a devel branch in the gmt-feedstock, but I'm not sure if it's a good idea to have a nightly release, since the packages (Linux, macOS and Windows) would be almost 200 MB for each release.

Found this one conda-forge/cfep#3, but haven't read it yet.

@weiji14
Copy link
Member Author

weiji14 commented Jun 22, 2020

I've started a new thread at conda-forge/gmt-feedstock#95. We can continue that part of the discussion there.

@weiji14 weiji14 force-pushed the gmt-master-gh-actions branch from 58cd99b to 3f136bb Compare June 29, 2020 04:39
@weiji14 weiji14 force-pushed the gmt-master-gh-actions branch from 12f5b90 to 575baa7 Compare July 12, 2020 10:29
@weiji14 weiji14 changed the title WIP: Github Actions CI for testing GMT master branch Github Actions CI for testing GMT master branch Jul 16, 2020
@weiji14 weiji14 mentioned this pull request Jul 18, 2020
5 tasks
@seisman
Copy link
Member

seisman commented Jul 18, 2020

I think we should move the "testing GMT master branch" into a separate file. Sometimes it's impossible to make all the tests for the current release and master branch. Then we will always see a failing badge in the README file.

@vercel vercel bot temporarily deployed to Preview July 22, 2020 08:30 Inactive
Also run the tests only on PRs that are ready for review or have a review requested (i.e. non-draft PRs). Based on https://github.saobby.my.eu.orgmunity/t/dont-run-actions-on-draft-pull-requests/16817/6.
@weiji14 weiji14 force-pushed the gmt-master-gh-actions branch from eff332b to 19c38d0 Compare July 22, 2020 08:31
Comment on lines +16 to +20
:alt: GitHub Actions Tests status
:target: https://github.com/GenericMappingTools/pygmt/actions?query=workflow%3ATests
.. image:: https://github.com/GenericMappingTools/pygmt/workflows/GMT%20Master%20Tests/badge.svg
:alt: GitHub Actions GMT Master Tests status
:target: https://github.com/GenericMappingTools/pygmt/actions?query=workflow%3A"GMT+Master+Tests"
Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should move the "testing GMT master branch" into a separate file. Sometimes it's impossible to make all the tests for the current release and master branch. Then we will always see a failing badge in the README file.

Ok, I've moved it into a separate file and added a separate badge. It would be nice if we could have separate badges for different jobs in a workflow but it doesn't look to be possible yet according to https://github.saobby.my.eu.orgmunity/t/separate-workflow-badges-when-using-matrix-testing-possible/16708.

Comment on lines +9 to +10
pull_request:
types: [review_requested, ready_for_review]
Copy link
Member Author

Choose a reason for hiding this comment

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

Found this nice way to limit the GMT Master Tests to non-draft PRs (see https://github.saobby.my.eu.orgmunity/t/dont-run-actions-on-draft-pull-requests/16817/6 and also https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request). I think this strikes a nice balance between testing GMT master "on every PR commit" and testing "only on the PyGMT master branch". Let me know if this is okay.

As an aside, we could apply this to other expensive tests too (e.g. *coughs* Windows).

Copy link
Member

Choose a reason for hiding this comment

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

Looks a good idea. Do we still want to test GMT master when a PR is merged into master branch?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we still want to test GMT master when a PR is merged into master branch?

Up to you. I'd prefer to save on CI resources, and there's a daily cron job anyway.

Copy link
Member

Choose a reason for hiding this comment

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

OK. Daily cron job is enough.

Copy link
Member

Choose a reason for hiding this comment

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

I just made a new commit b911366 to this branch. The GMT master workflow is not triggered. I need to click the "Re-request review" icon to trigger the GMT master test.

I think It's good. The GMT master workflow only runs when:

  1. A draft PR is ready for review
  2. A review is request or re-request

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's a nice feature. We don't need to test this too often anyway so I'm happy with it 😄 Should I document this in MAINTENANCE.md?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please.

fail-fast: false
matrix:
python-version: [3.8]
os: [ubuntu-20.04, macOS-10.15]
Copy link
Member Author

@weiji14 weiji14 Jul 22, 2020

Choose a reason for hiding this comment

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

Would love to test on Windows here, but I think we might have to do conda-forge/gmt-feedstock#95 first 😉

@weiji14 weiji14 marked this pull request as ready for review July 22, 2020 20:26
@seisman seisman self-requested a review July 22, 2020 21:32
Comment on lines +52 to +55
1. `ci_tests.yaml` (Style Checks, Tests on Linux/macOS/Windows)

This is ran on every commit on the *master* and Pull Request branches.
It is also scheduled to run daily on the *master* branch.
Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I've reorganized the Continuous Integration section a bit since it was getting complicated. You can insert a comment here about 'skipping ci-tests' later in #534.

@weiji14 weiji14 merged commit bdc51d7 into master Jul 22, 2020
@weiji14 weiji14 deleted the gmt-master-gh-actions branch July 22, 2020 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test pygmt with GMT master branch
2 participants