-
Notifications
You must be signed in to change notification settings - Fork 40
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 Workflows, Fix Cycamore Build #548
Conversation
(@gonuke accidentally edited this instead of reply...) I've omitted the nosetests for now, I can uncomment them but there were some import errors, and I think it may be less effort to just convert to pytest. Thoughts about this? |
scratch that, I think its a problem with the environment. I'll try to get the python tests passing with nose before attempting a conversion to pytest |
We do want to move to pytest - we did on Cyclus. Nosetest is deprecated. |
Seeing an interesting segfault in the cycamore tests on the conda builds (apt builds pass all tests). You can replicate by simply running
I'm not sure why there are references to |
I haven't seen this before, but it appears to be related to the CBC solvers in coin. We are getting those from the conda-forge channel, so maybe they are built against something that lives there? correctly? Incorrectly? |
It might be interesting to see if we test the Coin CBC solver in the Cyclus kernel tests and consider how it may be different from this test in Cycamore? |
This has continued to stump me. Discussed it briefly at the hacker within meeting today and the consensus was that it is probably an issue with how that package is being built. I just opened an issue in the conda-forge feedstock repo, hopefully I get a response. |
The CBC tests get skipped if MILPS is disabled. This appears to be the default install option, so I don't think we ever test the CBC solver in our CI. Do you think this is a good time to tackle 1591 ? |
Adding code coverage should be high on the list, but it would be nice to have some build passing so we can use it for incoming PRs. We could temporarily back off the condo builds, for example. |
My current assessment of this issue: the problem is not with conda, but with the way that we operate the CBC Solver. I believe that we are operating the solver in an outdated way (using I propose that in this PR I modify the python tests so that if MILPS is not enabled then we only test greedy solves (as it is in cyclus), then CI should be in a working state here in Cycamore. Additionally, an issue should be opened in Cyclus regarding the use Cbc, since it is not in a working state in Cyclus. |
remove unneeded echo
Right now this does not include conda in the testing matrices, and all is passing on my fork. We could merge this now to get some CI in place, however if we wait to get #1613 merged in Cyclus, we can fill out the testing matrix in Cycamore and it will be more robust with respect to MILPS being enabled/disabled. I'll defer to you @gonuke to make the call |
Let's get the cyclus merges in first, but I can't tell how 1613 and 1613 for together? |
I thought that I would separate the changes into 2 PRs, but I instead I need to just replace #1613 entirely with #1614 (including all the changes). I will get that done today and close 1613 |
I manually re-ran the workflow in my fork. They now pull in the updated cyclus images with these COIN fixes, and all tests pass 👏👏 Marking this PR as ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bennibbelink - this really helps get a stable footing for continued development
Does this need to be staged as a branch on the upstream, at least for the build/publish workflow? Also, I only see Circle tests here and no GitHub actions? |
Good point, I will make a branch on the upstream |
Closing in favor of #549 |
This replaces #544 and addresses #543. This PR adds testing workflows and fixes CMake issues.
The two testing workflows are adapted from Cyclus - one to push new images that have Cycamore and Cyclus installed, and one that pulls in the latest Cyclus image and builds/tests Cycamore against it.
The CMake fixes that I initially didn't think were necessary (since Cycamore doesn't use LibXML or SQLite) actually did need to be in place, since Cycamore is linked against
libcyclus.so
(build would fail on linking if I removed the Libxml and sqlite requirements)