Skip to content

Add github actions workflow for tests #4017

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

Closed
wants to merge 8 commits into from

Conversation

aseyboldt
Copy link
Member

@aseyboldt aseyboldt commented Jul 16, 2020

Right now we don't run our tests on windows automatically, which led to some problems there in the past.
This PR adds a github actions workflow that tests on linux and windows in addition to the tests on travis.
If we merge, those tests will be run for future PRs, so that we can evaluate github actions and see if we want to run it in addition or instead of travis.

Things to improve:

  • Run the linter as well as the tests
  • Use an environment file for the dependencies instead of manually installing them in the workflow file (see here)
  • Figure out the reporting of the coverage (if we want to replace travis)
  • Is the cache used properly (the .theano dir especially). The tests are somewhat slower on github actions compared to travis, this might be the reason.

There are some I think legitimate test failures on windows right now. One compiler issue on xeon CPUs is "fixed" by setting -march=cores2 in the tests, but some shape issues and the fact that np.float128 is not available on windows with the MS compiler remain.

Also, the windows tests take about twice as long to run, I'm not sure if this is due to higher IO overhead on the virtual machines or has some other reason.

CC @canyon289 @brandonwillard @junpenglao

@twiecki
Copy link
Member

twiecki commented Jul 27, 2020

Also, the windows tests take about twice as long to run, I'm not sure if this is due to higher IO overhead on the virtual machines or has some other reason.

People have reported much slower speeds on native windows before, I would imagine it's related to that.

Overall it would be great to get Windows testing here. What's missing? Seems like many tests are still failing.

@aseyboldt
Copy link
Member Author

Fixes for the windows issues are missing. I don't know what's going on there, they look like legitimate issues, not just test env stuff.
Also the "Things to improve" from the descriptions. I don't think those need to stop us from merging though.

@codecov
Copy link

codecov bot commented Aug 9, 2020

Codecov Report

Merging #4017 (5b000c1) into master (68d5201) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4017   +/-   ##
=======================================
  Coverage   88.19%   88.19%           
=======================================
  Files          89       89           
  Lines       14358    14358           
=======================================
  Hits        12663    12663           
  Misses       1695     1695           
Impacted Files Coverage Δ
pymc3/__init__.py 100.00% <ø> (ø)

@twiecki
Copy link
Member

twiecki commented Aug 10, 2020

@aseyboldt You mean the windows issues should not block us from merging? If so, I'm uneasy having a broken CI on master if we have no known path of fixing those issues.

@aseyboldt
Copy link
Member Author

@twiecki You are right, we should fix those issues before merging...
I'll try again tomorrow or possibly next week.

@michaelosthege
Copy link
Member

@aseyboldt can you resolve the conflicts?

And how much work is left to do to push this over the finish line?

@twiecki
Copy link
Member

twiecki commented Nov 24, 2020

I think the way forward here is to disable the windows builds for now, get this merged, and then figure windows out separately. @MarcoGorelli agreed to take this over - thanks Marco!

@michaelosthege
Copy link
Member

sorry about the intervention - the merge conflict was just about whitespace. But I still fucked it up - black wants an empty line behind that import 🙄

@MarcoGorelli
Copy link
Contributor

Do you have a strong preference for using conda? I just gave this a go using pip in my fork and it seems to work fine, see MarcoGorelli#2

@AlexAndorra
Copy link
Contributor

What do you mean by "using conda"?

@MarcoGorelli
Copy link
Contributor

In this PR, the action creates a conda environment:

    steps:
    - uses: actions/checkout@v2
    - name: Cache conda and theano
      uses: actions/cache@v2
      env:
        # Increase this value to reset cache
        CACHE_NUMBER: 0
      with:
        path: |
          ~/conda_pkgs_dir
          ~/.theano
        key: ${{ runner.os }}-${{ matrix.floatx }}-py${{ matrix.python-version }}-${{ env.CACHE_NUMBER }}
    - name: Install miniconda
      uses: goanpeca/setup-miniconda@v1
      with:
        auto-update-conda: true
        activate-environment: pymc3-dev
        python-version: ${{ matrix.python-version }}
        channels: conda-forge,defaults
        channel-priority: strict
    - name: Install Dependences
      shell: bash -l {0}
      run: |
        conda install --yes --only-deps pymc3
        conda install coverage pytest parameterized dill nose nose-parameterized python-graphviz pytest-cov

Maybe we could use conda for the Windows job, and pip for the Linux one, so both are tested - I'll take a closer look tomorrow

@twiecki
Copy link
Member

twiecki commented Nov 25, 2020

@MarcoGorelli I prefer conda because it's a) much faster to install and b) sets everything up correctly with MKL. Furthermore, it's the recommended way to install PyMC3. Why do you think pip would be preferable?

@MarcoGorelli
Copy link
Contributor

Why do you think pip would be preferable?

I hadn't realised conda was the recommended way to install, so pip just seemed simpler - I've gone with conda anyway

@MarcoGorelli
Copy link
Contributor

Thanks for having started this, it was really helpful - closing as the move to GHA been implemented in other PRs

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.

5 participants