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

Draft Github CI integrations #484

Merged
merged 7 commits into from
Nov 9, 2022
Merged

Conversation

fsaad
Copy link
Collaborator

@fsaad fsaad commented Oct 31, 2022

Related #483

@fsaad fsaad force-pushed the 20221031-fsaad-github-actions branch 3 times, most recently from 43ee178 to 22f807a Compare October 31, 2022 20:32
@fsaad fsaad changed the title Draft Gtihub CI integrations Draft Github CI integrations Oct 31, 2022
@fsaad fsaad force-pushed the 20221031-fsaad-github-actions branch from 22f807a to 366bb26 Compare October 31, 2022 20:38
@fsaad fsaad force-pushed the 20221031-fsaad-github-actions branch from 366bb26 to 9fb1cb0 Compare October 31, 2022 21:17
@fsaad fsaad requested a review from femtomc October 31, 2022 21:28
@fsaad
Copy link
Collaborator Author

fsaad commented Oct 31, 2022

The ContinuousIntegration actions appears to work correctly.

While the Documentation action works within this PR, I still have not verified that it will correctly push the documentation to gen.dev. I'm not sure that there is a way to easily test this behavior outside of merging this PR into master.

runs-on: ${{ matrix.os }}
strategy:
matrix:
julia-version: ['1.3', 'nightly']
Copy link
Contributor

Choose a reason for hiding this comment

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

1.3 is quite old -- most packages in the ecosystem have dropped support for 1.3. Let's test the latest LTS instead 1.6.7.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am happy to bump it, but also not sure why the current Travis file tests against 1.3, maybe it is related to how Gen is configured in the official Julia Registry?

https://github.com/JuliaRegistries/General/blob/01717d7c358686ddbbe8ce40d159b6452b870f52/G/Gen/Compat.toml#L33-L35

Copy link
Member

Choose a reason for hiding this comment

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

It's not about the Julia Registry, it's that the current version of Gen (v0.4) still officially supports Julia 1.3 according to our Project.toml file: https://github.com/probcomp/Gen.jl/blob/master/Project.toml

We could decide to drop support for Julia 1.3 and only support Julia 1.6 onwards, but I think that should be a separate change, and in the meantime I think our CI tests should still test on the versions we're claiming to support!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, let's keep 1.3 for now and handle the upgrade to 1.6 in a future PR.

Copy link
Member

Choose a reason for hiding this comment

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

So it looks for the Travis file, we are currently testing 1.3, 1.6, 1 (latest stable), and nightly -- I think we should do the same for the GitHub CI action.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Testing against fewer versions has the advantage that it is faster to get the status checks (where each version can take several minutes) and saves computational resources, especially when the status checks are run against each commit in a PR (which is something we should disable).

For this PR I'm happy to test against the same versions of Travis, and then we can do a separate PR for deciding which versions CI should test against and formally support (e.g., "1.6" and "nightly").

@ztangent
Copy link
Member

ztangent commented Nov 9, 2022

It looks like the CI test for Julia 1.6 is failing on this test case due to some platform specific issue leading to different RNG seeds:

default proposal: Test Failed at /home/runner/work/Gen.jl/Gen.jl/test/inference/particle_filter.jl:169
  Expression: isapprox(expected_log_ml, actual_log_ml_est, atol = 0.02)
   Evaluated: isapprox(-4.87645083351704, -4.897679480149515; atol = 0.02)

In contrast a recent Travis job for Julia 1.6 passes that test just fine.

We should probably change the failing test to make them more error tolerant in a separate PR. But for the purposes of this PR, it looks like one difference is that previously, the (passing) Travis build was running on an x64 architecture:

Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: Intel(R) Xeon(R) CPU @ 2.80GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-11.0.1 (ORCJIT, cascadelake)

Whereas the failing GitHub CI action is running on a x86 architecture (note the value for WORD_SIZE):

Platform Info:
  OS: Linux (i686-pc-linux-gnu)
  CPU: Intel(R) Xeon(R) Platinum 8272CL CPU @ 2.60GHz
  WORD_SIZE: 32
  LIBM: libopenlibm
  LLVM: libLLVM-11.0.1 (ORCJIT, skylake-avx512)

To address this, I think we should try changing the GitHub action to run on x64 instead, and hope that this fixes the issue. It would also maintain consistency with the previous test suite.

@ztangent
Copy link
Member

ztangent commented Nov 9, 2022

Looks like the checks passed! Merging :)

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