theochem / cgrid

C++ version of horton (2.x) grid functionality
GNU General Public License v3.0
4 stars 1 forks source link

Do not test everything in every travis run, to speed things up #7

Closed tovrstra closed 7 years ago

tovrstra commented 7 years ago

Tagged commits:

Other commits and PRs:

matt-chan commented 7 years ago

For the OSes, there's a significant snag. The main reason a build takes so long is because travis is always short of OSX VMs. Even though a build takes 5min, it will often wait for 1 hour in the queue to get a worker. There's no way to have code run before the worker is requested.

The best solution I can think of for now is to mark the OSX builds as allowed to fail, and then to allow fast-finishing. This means we'll get a checkmark on github as soon as the linux jobs are done. However, we would still need to check the travis page to see when OSX finishes.

The other downside to this approach is that we fill up our job queue. We can't start CI on another commit in the same repo until the previous commits are done (including OSX). We can get around it slightly by telling travis to cancel previous builds if a new commit is made.

tovrstra commented 7 years ago

I've also noticed that they are short on OSX vms. We can do a few things about it:

matt-chan commented 7 years ago

We can use the $TRAVIS_TAG variable, but I can't figure out how to exclude a build based on an empty variable...

Also is it possible to exclude a build based on envs that haven't been defined in the matrix? I've only ever seen examples of exclusion which reference envs in the build matrix. The travis documentation is more demonstrative than definitive though...

On Fri, 1 Sep 2017 at 09:14 Toon Verstraelen notifications@github.com wrote:

I've also noticed that they are short on OSX vms. We can do a few things about it:

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/theochem/qcgrids/issues/7#issuecomment-326508125, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_-NT3GzCex05bWa8iB_3eODXUCsacGks5sd67AgaJpZM4PI17r .

-- Matt

Sent from my phone

tovrstra commented 7 years ago

This will eventually become useful for speedups https://docs.travis-ci.com/user/build-stages/

matt-chan commented 7 years ago

Yes, I was looking at that as well last night. It's still a bit too unpolished for my liking though. Also it's a bit too complicated. It lets us use build matrices within a stage though, so I think we'll be able to have finer grain control over whether we provision an osx worker or not.

On Fri, 1 Sep 2017 at 13:11 Toon Verstraelen notifications@github.com wrote:

This will eventually become useful for speedups https://docs.travis-ci.com/user/build-stages/

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/theochem/qcgrids/issues/7#issuecomment-326554676, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_-NdsyYCb6Fh8UMV3-LiPY1S0M7Freks5sd-Z2gaJpZM4PI17r .

-- Matt

Sent from my phone

tovrstra commented 7 years ago

Someone once asked the question on build matrix and TRAVIS_TAG here: https://stackoverflow.com/questions/31867394/excluding-jobs-in-travis-ci-build-matrix-for-non-tag-builds It does not seem to work but we should try to be sure.

matt-chan commented 7 years ago

Yes, I saw that post. I'm going to give it a shot anyways with the qcgrids repo in a bit. We'll see what happens.

On Fri, 1 Sep 2017 at 13:21 Toon Verstraelen notifications@github.com wrote:

Someone once asked the question on build matrix and TRAVIS_TAG here: https://stackoverflow.com/questions/31867394/excluding-jobs-in-travis-ci-build-matrix-for-non-tag-builds It does not seem to work but we should try to be sure.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/theochem/qcgrids/issues/7#issuecomment-326556198, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_-NXoAC33vSRA4m5ymwq1XhdBWF56hks5sd-iXgaJpZM4PI17r .

-- Matt

Sent from my phone

matt-chan commented 7 years ago

@tovrstra So do we still do the dynamic lint tests for tagged releases? Currently the code only does cardboard for the inplace builds. I can copy it over for the dynamic ones, but I think the only real addition would be pylint, since there won't be coverage for the others.

tovrstra commented 7 years ago

I guess pylint is not so essential when building packges. I think it is OK to drop the dynamic ones when building a package for release.

matt-chan commented 7 years ago

Sounds good.

@tovrstra is there a reason that for the inplace build, we do the Py project first (depending on conda's C++ install) and then the C++ after? With the split builds, I have to build the C++ first and then the Py after.

tovrstra commented 7 years ago

The in-place build was just the easiest way to compile and test with debugging options enabled. I also guessed that coverage data would be easier to process because all files involved are relative to the source tree. (The latter argument may not matter much. I did not test.)

matt-chan commented 7 years ago

Hmm. I think I'll have to add a library search path in the setup.py... sigh. I thought we were free of those. Oh well.

On Fri, 1 Sep 2017 at 19:23 Toon Verstraelen notifications@github.com wrote:

The in-place build was just the easiest way to compile and test with debugging options enabled. I also guessed that coverage data would be easier to process because all files involved are relative to the source tree. (The latter argument may not matter much. I did not test.)

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/theochem/qcgrids/issues/7#issuecomment-326637746, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_-NX31azuBrEOEj7DFun-auZQhhL8Eks5seD2IgaJpZM4PI17r .

-- Matt

Sent from my phone

tovrstra commented 7 years ago

Hmm. Test should not be necessary. Are you building with conda gcc etc?

On Sep 1, 2017 7:47 PM, "Matthew Chan" notifications@github.com wrote:

Hmm. I think I'll have to add a library search path in the setup.py... sigh. I thought we were free of those. Oh well.

On Fri, 1 Sep 2017 at 19:23 Toon Verstraelen notifications@github.com wrote:

The in-place build was just the easiest way to compile and test with debugging options enabled. I also guessed that coverage data would be easier to process because all files involved are relative to the source tree. (The latter argument may not matter much. I did not test.)

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/theochem/qcgrids/issues/7#issuecomment-326637746, or mute the thread https://github.com/notifications/unsubscribe- auth/AA_-NX31azuBrEOEj7DFun-auZQhhL8Eks5seD2IgaJpZM4PI17r .

-- Matt

Sent from my phone

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/theochem/qcgrids/issues/7#issuecomment-326643204, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGEZ4gPhX58cQD2etVZgICwWEG_tkdzks5seENAgaJpZM4PI17r .

matt-chan commented 7 years ago

Yes, but since the C++ library is being built in-place (and not with conda), then the library link path/include path are wrong. Also it's not on the LD search path, so that needs to be updated too. The only thing the conda gcc does for us is give cellcutoff for free. The C++ qcgrids project doesn't get it that benefit =/.

I think we could do a C++ install alternatively... It shouldn't really matter if we munge the system directories. It'll be destroyed later anyways...

tovrstra commented 7 years ago

We should meet at some point. Things are getting to difficult to discuss over GitHub.

On Sep 1, 2017 9:10 PM, "Matthew Chan" notifications@github.com wrote:

Yes, but since the C++ library is being built in-place (and not with conda), then the library link path/include path are wrong. Also it's not on the LD search path, so that needs to be updated too. The only thing the conda gcc does for us is give cellcutoff for free. The C++ qcgrids project doesn't get it that benefit =/.

I think we could do a C++ install alternatively... It shouldn't really matter if we munge the system directories. It'll be destroyed later anyways...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/theochem/qcgrids/issues/7#issuecomment-326661690, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGEZ2KNigr0eTRwSa5SWX5lDGqhwB1Xks5seFa4gaJpZM4PI17r .

matt-chan commented 7 years ago

Okay, I'll be there Monday. I think I can get this stuff + the caching running on all the projects by tomorrow night.

On Fri, 1 Sep 2017 at 21:35 Toon Verstraelen notifications@github.com wrote:

We should meet at some point. Things are getting to difficult to discuss over GitHub.

On Sep 1, 2017 9:10 PM, "Matthew Chan" notifications@github.com wrote:

Yes, but since the C++ library is being built in-place (and not with conda), then the library link path/include path are wrong. Also it's not on the LD search path, so that needs to be updated too. The only thing the conda gcc does for us is give cellcutoff for free. The C++ qcgrids project doesn't get it that benefit =/.

I think we could do a C++ install alternatively... It shouldn't really matter if we munge the system directories. It'll be destroyed later anyways...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/theochem/qcgrids/issues/7#issuecomment-326661690, or mute the thread < https://github.com/notifications/unsubscribe-auth/AAGEZ2KNigr0eTRwSa5SWX5lDGqhwB1Xks5seFa4gaJpZM4PI17r

.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/theochem/qcgrids/issues/7#issuecomment-326666675, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_-NfYMnpcNlwLJAMC52sKcMVKnVQGNks5seFyBgaJpZM4PI17r .

-- Matt

Sent from my phone

tovrstra commented 7 years ago

I'm going to take a quick look at the PR, see if I can already help out. Just a sec.

tovrstra commented 7 years ago

This is fixed by #10 and #11