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

Add coverage test and test against qutip released & master #31

Merged
merged 4 commits into from
May 24, 2021

Conversation

BoxiLi
Copy link
Member

@BoxiLi BoxiLi commented May 13, 2021

Update the GitHub action so that qutip-qip is tested against both the released version of qutip and against the master branch.
Add action for Coveralls.

Tests will also be much faster (except for the master branch) because we can use distributed wheels from qutip4.6 instead of building it ourselves.

.github/workflows/test.yml Outdated Show resolved Hide resolved
Co-authored-by: Nathan Shammah <nathan.shammah@gmail.com>
@BoxiLi BoxiLi requested a review from nathanshammah May 14, 2021 11:27
Comment on lines +36 to +37
# TODO: add dev.major and minimal supported qutip version v4.6
qutip-version: ['master']
Copy link
Member

Choose a reason for hiding this comment

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

You can add the minimal version right now if you like, right? You can run the test against the v4.6.0 tag, which is guaranteed to be static, or the qutip-4.6.X branch which will always point to the latest release in the 4.6 line. The pip command is something like pip install git+https://github.com/qutip/qutip.git@[branch or tag].

Copy link
Member

@jakelishman jakelishman May 14, 2021

Choose a reason for hiding this comment

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

Also, you should look into caching the build dependency - that'll speed up your tests no end, since from your end, the QuTiP master branch won't update that often: https://docs.github.com/en/actions/guides/caching-dependencies-to-speed-up-workflows. You'd basically do git rev-parse qutip/refs/heads/master, and use that (with a known prefix) as your cache key for the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

You can add the minimal version right now if you like, right?

Oh, yes, I was just being lazy because right now as the official qutip is 4.6. I think if I want to test qutipv4.60 I can just pip install qutip=4.6.0 right? This is a downstream package.

Caching the build dependency is a good idea, thanks!

Copy link
Member

@jakelishman jakelishman May 14, 2021

Choose a reason for hiding this comment

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

The official is 4.6.1 right now. You could pip install 'qutip>=4.6,<4.7' to get the latest released 4.6 version, which is nice because then you're testing against the true released package, however it means you need a special switch in your CI to decide whether to pip install from a PyPI target or a git target - the alternative is targetting specific versions from the qutip/qutip GitHub repo, and using dependency caching to ensure you need to rebuild at the absolute most once a week (GitHub empties cache files that haven't been touched in 7 days, so if there's at least one test run a week it'll never need rebuilding).

I think either would in general be fine - the CI switching between PyPI and GitHub is probably a little neater. You could implement it by having a sigil on your version specifier, so @master means a git ref and anything other than @ means PyPI. That way you could use GitHub Actions' if conditions on jobs to selectively run the one you want:

strategy:
    matrix:
        qutip-version: ['>=4.6,<4.7', '@master', '@dev.major']

steps:
    ...
    install-pypi:
        name: Install QuTiP from PyPI
        if: ${{ ! startsWith( matrix.qutip-version, '@') }}
        run: pip install 'qutip${{ matrix.qutip-version }}'

    install-git:
        name: Install QuTiP from GitHub
        if: ${{ startsWith( matrix.qutip-version, '@') }}
        run: pip install 'git+https://github.com/qutip/qutip.git${{ matrix.qutip-version }}'

Looks pretty neat to me? (caveat: I didn't test the syntax at all)

Copy link
Member

@nathanshammah nathanshammah left a comment

Choose a reason for hiding this comment

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

Thanks @BoxiLi and @jakelishman. Looks good to me, there are some conflicts though.

@BoxiLi BoxiLi merged commit 29be1b7 into qutip:master May 24, 2021
@BoxiLi
Copy link
Member Author

BoxiLi commented May 24, 2021

Okay, there is a mistake in the test workflow. It slipped through because if the syntax is wrong, no tests will run at all and one sees no red crosses. One needs to be extra careful when changing the workflows.

@BoxiLi BoxiLi mentioned this pull request May 24, 2021
BoxiLi added a commit that referenced this pull request May 24, 2021
There was a syntax error in the test workflow added in #31, fix it.
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