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

Pin dependencies to a successful CI state #1107

Closed
wants to merge 11 commits into from
Closed

Pin dependencies to a successful CI state #1107

wants to merge 11 commits into from

Conversation

davidbrochart
Copy link
Contributor

No description provided.

@tridelat
Copy link
Member

tridelat commented Mar 5, 2020

Thanks @davidbrochart
Should I merge #1103 in master so that you can rebase this PR? Even if conda tests are not passing on #1103, hashdist tests are passing, meaning that it is supposedly safe.
Otherwise I will close #1103 and we can merge this one only

@davidbrochart
Copy link
Contributor Author

This PR is already rebased on #1103, so no need to merge right now.
Tests still fail on Linux. I pinned as much as I could to the package versions we have on OSX, where tests pass. So I don't know what's going on.
Could it be that we have some C code reading data from uninitialized memory? We could check the erroneous results are reproducible (if not, this is likely due to uninitialized memory).

@tridelat
Copy link
Member

tridelat commented Mar 5, 2020

This PR is already rebased on #1103, so no need to merge right now.
Tests still fail on Linux. I pinned as much as I could to the package versions we have on OSX, where tests pass. So I don't know what's going on.
Could it be that we have some C code reading data from uninitialized memory? We could check the erroneous results are reproducible (if not, this is likely due to uninitialized memory).

@davidbrochart I could reproduce the failure locally on Linux with the unpinned packages.
I think you already know, but OSX "test" passes on Travis CI because only the installation is checked, but actual tests are not activated.
I will look at the tests again on my local Linux machine with this branch.

@davidbrochart
Copy link
Contributor Author

No I didn't know, I should have looked at the CI! So what I did was useless.
Where can I look at a successful Linux CI?

@tridelat
Copy link
Member

tridelat commented Mar 5, 2020

No I didn't know, I should have looked at the CI! So what I did was useless.
Where can I look at a successful Linux CI?

@davidbrochart here is one that passed (and the last one that affected tests I think):
https://travis-ci.com/erdc/proteus/jobs/290783196

@codecov
Copy link

codecov bot commented Mar 5, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@5a87369). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1107   +/-   ##
=========================================
  Coverage          ?   51.43%           
=========================================
  Files             ?      534           
  Lines             ?   101882           
  Branches          ?        0           
=========================================
  Hits              ?    52407           
  Misses            ?    49475           
  Partials          ?        0
Impacted Files Coverage Δ
proteus/__init__.py 26.08% <100%> (ø)
proteus/tests/post_processing/test_bdm2_3d_mesh.py 82% <100%> (ø)
proteus/tests/post_processing/test_bdm2_mesh8.py 82% <100%> (ø)
...eus/tests/post_processing/test_bdm_3Dprojection.py 85.48% <100%> (ø)
proteus/tests/levelset/rotation/test_rotation2D.py 78.33% <100%> (ø)
...oteus/tests/post_processing/test_bdm_projection.py 87.67% <100%> (ø)
...orming_rans2p/test_cylinder2D_conforming_rans2p.py 84.48% <100%> (ø)
proteus/tests/TwoPhaseFlow/test_TwoPhaseFlow.py 82% <100%> (ø)
proteus/tests/SWEs/test_SWEs.py 97.36% <100%> (ø)

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 5a87369...74c1d2e. Read the comment docs.

@davidbrochart
Copy link
Contributor Author

@tridelat the CI passes on Linux with this last commit. It's a lot of pinning, somehow I have the feeling that the reason of the failure was related to python3.8 (it's pinned to 3.7 here).

@tridelat
Copy link
Member

tridelat commented Mar 5, 2020

@davidbrochart thanks! Interesting that python 3.8 might be what caused the issue.
pinning everything can look a bit cumbersome, but I think it could be preferable in our case to guarantee that results are reproducible.
The conda install on OSX doesn't pass however, it looks like it does not like the pinned gmsh version

@tridelat
Copy link
Member

tridelat commented Mar 5, 2020

@davidbrochart I'm going to go ahead and merge #1103, and then we can merge this PR once the OSX build is passing

@tridelat tridelat mentioned this pull request Mar 5, 2020
@davidbrochart
Copy link
Contributor Author

Closing this PR in favor of #1109, I had to delete my fork because I couldn't get rid of a conflict with the following file which seems to be on LFS now: proteus/tests/FSI/meshFloatingCylinder.geo

@davidbrochart
Copy link
Contributor Author

BTW @tridelat why don't we activate the tests on OSX?

@tridelat
Copy link
Member

tridelat commented Mar 6, 2020

@davidbrochart good question, I am not sure actually, and I am not a OSX user. It might have been that some of the tests were giving problems there, but it's probably worth checking if they now pass.
OSX build on Travis CI was introduced in #1083, where it just tests that a single simulation case can run, and #1068 added the commented out pytest line.

@davidbrochart
Copy link
Contributor Author

Trying out in #1110

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