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

Update CI #113

Merged
merged 13 commits into from
Jun 17, 2020
Merged

Update CI #113

merged 13 commits into from
Jun 17, 2020

Conversation

devmotion
Copy link
Member

This PR changes the Julia versions that we test on Travis to the stable, LTS, and nightly version (1.4 is currently not tested on Travis). Moreover, I removed the AppVeyor configuration and switched Github Actions instead for testing on Windows.

Additionally, the Github Actions configuration was incorrect - instead of Julia 1.0 and 1.3 (as one might assume by looking at the configuration files), tests used Julia 1.4 and 1.3. I changed this to stable and LTS Julia versions. However, given that the tests (in particular the Zygote tests) take a lot of time, maybe one would want to test only the stable Julia version?

Moreover, the CompatHelper action is updated (there's no need to install Julia anymore explicitly, CompatHelper takes care of that). If CompatHelper should also trigger the Github CI tests, one has to add an additional key, see https://github.com/bcbi/CompatHelper.jl#12-set-up-the-ssh-deploy-key-optional (I don't have the permission for it and I am not sure if we maybe only want to run the Travis tests).

Additionally, to save resources I reduced the frequency of TagBot and CompatHelper to once per day, as suggested in their respective example configurations.

@devmotion
Copy link
Member Author

A compromise could also be to test Zygote only with the stable Julia version (since Zygote highly recommends Julia >= 1.3) but test ReverseDiff, ForwardDiff, and Tracker with both LTS and stable Julia versions.

@yebai
Copy link
Member

yebai commented Jun 16, 2020

A compromise could also be to test Zygote only with the stable Julia version (since Zygote highly recommends Julia >= 1.3) but test ReverseDiff, ForwardDiff, and Tracker with both LTS and stable Julia versions.

Sounds sensible.

@codecov
Copy link

codecov bot commented Jun 16, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@c19c9a9). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #113   +/-   ##
=========================================
  Coverage          ?   57.22%           
=========================================
  Files             ?       23           
  Lines             ?     1169           
  Branches          ?        0           
=========================================
  Hits              ?      669           
  Misses            ?      500           
  Partials          ?        0           

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 c19c9a9...0f5b57d. Read the comment docs.

@yebai
Copy link
Member

yebai commented Jun 16, 2020

Moreover, the CompatHelper action is updated (there's no need to install Julia anymore explicitly, CompatHelper takes care of that). If CompatHelper should also trigger the Github CI tests, one has to add an additional key, see https://github.com/bcbi/CompatHelper.jl#12-set-up-the-ssh-deploy-key-optional (I don't have the permission for it and I am not sure if we maybe only want to run the Travis tests).

@devmotion I just made you an admin user of Bijectors.jl. Maybe give it another try?

@yebai
Copy link
Member

yebai commented Jun 16, 2020

It seems failing tests are due to insufficient RAM of CI machines.

LLVM ERROR: out of memory
ERROR: Package Bijectors errored during testing

https://github.com/TuringLang/Bijectors.jl/pull/113/checks?check_run_id=777374944#step:5:1667

It's slightly surprising that testing Bijectors would consume more RAM than what CI machines provide. Perhaps this can be resolved by breaking down the CI script into multiple steps, and run each CI script using a fresh Julia session. This would allow LLVM to release some memory.

@devmotion
Copy link
Member Author

Yes, this should definitely not happen. I think the AD test setup might be problematic, in particular the heavy use of eval. That seems surprising and "non-standard", and might provoke issues similar to JuliaLang/julia#30653 (just a speculation).

@devmotion
Copy link
Member Author

I replaced the whole eval machinery with just regular functions and simplified the test setup, but I'm wondering if it would be better to make these changes in a separate PR. Maybe a workaround for now would be to just not run the tests on 32bit systems (since it seems the RAM problems only occur there)?

@yebai yebai merged commit a72665d into master Jun 17, 2020
@delete-merged-branch delete-merged-branch bot deleted the devmotion-patch-1 branch June 17, 2020 12:03
@yebai
Copy link
Member

yebai commented Jun 17, 2020

I replaced the whole eval machinery with just regular functions and simplified the test setup, but I'm wondering if it would be better to make these changes in a separate PR. Maybe a workaround for now would be to just not run the tests on 32bit systems (since it seems the RAM problems only occur there)?

yes, a separate PR sounds sensible.

@devmotion devmotion mentioned this pull request Jun 17, 2020
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.

2 participants