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

Use GitHub Actions & Pytest #45

Merged
merged 8 commits into from
Jun 16, 2021
Merged

Use GitHub Actions & Pytest #45

merged 8 commits into from
Jun 16, 2021

Conversation

phschiele
Copy link
Collaborator

@phschiele phschiele commented May 26, 2021

Hi @akshayka !
this PR addresses #43 and #44. It also switches from nose to pytest, I hope that's fine with you.

Some remaining open points:

  • GitHub Actions do not seem to be enabled
  • test_threading fails under ubuntu & Python 3.6 (under nose and pytest, relative tolerance is 1e-7, maximum relative difference is 1e-5)
  • Potentially one could specify lower versions for scipy and numpy during the install, but the scipy=1.3 numpy=1.16 combo from cvxpy did not work.
  • I have commented out some conditions to see if everything works as expected, needs to be reverted before merge
  • The secrets need to be added to the repo

@akshayka akshayka requested a review from sbarratt June 8, 2021 04:47
@akshayka
Copy link
Member

akshayka commented Jun 8, 2021

Thanks @phschiele!

  • The repository settings say that all Github Actions are enabled. What kind of error are you running into?
  • I'm fine with decreasing the tolerance so that test_threading passes.
  • Can you clarify what error you ran when specifying the scipy and numpy versions?
  • I have added the PYPI secrets to the repository.

@phschiele
Copy link
Collaborator Author

phschiele commented Jun 8, 2021

@akshayka

  • if it's not the settings, then I am not quite sure why the action is not running. Perhaps adding the first action from a forked repo PR does not trigger them yet? As you can see here, the checks run in my fork. We can check this if you add the "cleanup" action posted below to .github/workflows/build.yml.
  • Regarding numpy/scipy versions: Initially, I tried with numpy=1.15 scipy=1.1.0 as per setup.py. This raised conda dependency conflicts. I then tried numpy=1.16 scipy=1.3, but it did not work yet for Python 3.9, again due to dependency conflicts. In the last commit, I have fixed the versions in alignment with cvxpy.
name: build

on:
    pull_request:
    push:
        branches:
            - master
        tags:
          - '*'

jobs:
  cleanup-runs:
    runs-on: ubuntu-latest
    steps:
    - uses: rokroskar/workflow-run-cleanup-action@master
      env:
        GITHUB_TOKEN: "${{ secrets.GITHUB_TOKEN }}"
    if: "!startsWith(github.ref, 'refs/tags/') && github.ref != 'refs/heads/master'"

@sbarratt
Copy link
Collaborator

sbarratt commented Jun 8, 2021

Will this run on PRs?

@phschiele
Copy link
Collaborator Author

@sbarratt Yes, the idea is to run the build incl. tests on every PR. Building wheels and deploying to PYPI will only be done whenever a new tag is created.

Copy link
Member

@akshayka akshayka left a comment

Choose a reason for hiding this comment

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

LGTM, modulo the workflows not running. Will look into that ...

@akshayka
Copy link
Member

akshayka commented Jun 9, 2021

@akshayka

  • if it's not the settings, then I am not quite sure why the action is not running. Perhaps adding the first action from a forked repo PR does not trigger them yet?

That appears to be the case:

https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

However, the documentation says that I should see an option to approve your workflow and run it on this PR, which I do not. (I approved the PR to see if the option would show up.) All I see is the following

image

These links might be helpful:

WordPress/gutenberg#17324 (comment)

WordPress/gutenberg#26876

@phschiele phschiele mentioned this pull request Jun 9, 2021
@phschiele
Copy link
Collaborator Author

@akshayka The GH Actions are running (and passing) now.

@akshayka
Copy link
Member

Looks good, thanks @phschiele!

@akshayka akshayka merged commit e17162d into cvxgrp:master Jun 16, 2021
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.

3 participants