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

TST: Add an appveyor starter file. #787

Merged
merged 31 commits into from Sep 18, 2018

Conversation

Projects
None yet
6 participants
@arokem
Copy link
Member

arokem commented Nov 25, 2015

So we can test on Windows.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Nov 25, 2015

I'm going to work out all the kinks over here: arokem#23, before we flip the switch to turn on appveyor here as well.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 7, 2015

Hey @omarocegueda - the appveyor machine runs out of memory on test_affreg_all_transforms: https://ci.appveyor.com/project/arokem/dipy/build/1.0.496/job/0jrao9di53eu26yn. Any thoughts on how to reduce memory requirements of that test?

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 7, 2015

In good news, it runs cleanly and tests pass on Python 2.7!

https://ci.appveyor.com/project/arokem/dipy/build/1.0.496/job/occt9m4c1wua7vh3

@omarocegueda

This comment has been minimized.

Copy link
Contributor

omarocegueda commented Dec 7, 2015

Thanks Ariel!,
yes, I have seen this before but I couldn't repro even by running the tests
on the actual buildbot that reported the failure
#694

I don't know how to proceed here, any ideas?

On Mon, Dec 7, 2015 at 12:08 PM, Ariel Rokem notifications@github.com
wrote:

In good news, it runs cleanly and tests pass on Python 2.7!

https://ci.appveyor.com/project/arokem/dipy/build/1.0.496/job/occt9m4c1wua7vh3


Reply to this email directly or view it on GitHub
#787 (comment).

"Cada quien es dueño de lo que calla y esclavo de lo que dice"
-Proverbio chino.
"We all are owners of what we keep silent and slaves of what we say"
-Chinese proverb.

http://www.cimat.mx/~omar

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 7, 2015

OK - I remember this now. Strangely, it seems to happen consistently on
Python 3.3, but not on Python 2.7, or Python 3.4...

How about reducing the number of slices used in this test from 45 to 20
(see recent commit). Does that pose a problem in any other sense?

On Mon, Dec 7, 2015 at 10:40 AM, Omar Ocegueda notifications@github.com
wrote:

Thanks Ariel!,
yes, I have seen this before but I couldn't repro even by running the tests
on the actual buildbot that reported the failure
#694

I don't know how to proceed here, any ideas?

On Mon, Dec 7, 2015 at 12:08 PM, Ariel Rokem notifications@github.com
wrote:

In good news, it runs cleanly and tests pass on Python 2.7!

https://ci.appveyor.com/project/arokem/dipy/build/1.0.496/job/occt9m4c1wua7vh3


Reply to this email directly or view it on GitHub
#787 (comment).

"Cada quien es dueño de lo que calla y esclavo de lo que dice"
-Proverbio chino.
"We all are owners of what we keep silent and slaves of what we say"
-Chinese proverb.

http://www.cimat.mx/~omar


Reply to this email directly or view it on GitHub
#787 (comment).

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 8, 2015

@omarocegueda

This comment has been minimized.

Copy link
Contributor

omarocegueda commented Dec 8, 2015

Thanks Ariel, I think it's ok for now because it appears to happen under
very specific conditions. I will try to reproduce the failure again anyway:
affine registration does not consume a lot of memory, it should be fine to
use even more slices.

On Mon, Dec 7, 2015 at 8:36 PM, Ariel Rokem notifications@github.com
wrote:

That seemed to do the trick:
https://ci.appveyor.com/project/arokem/dipy/build/1.0.503/job/dybb7p58om4vo1ke


Reply to this email directly or view it on GitHub
#787 (comment).

"Cada quien es dueño de lo que calla y esclavo de lo que dice"
-Proverbio chino.
"We all are owners of what we keep silent and slaves of what we say"
-Chinese proverb.

http://www.cimat.mx/~omar

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 8, 2015

Hi Omar - it seems that it doesn't happen anymore when I set the number of
slices in the test down to 20 (here:
arokem@0b134f3
).

Any reason not to do that?

On Mon, Dec 7, 2015 at 7:33 PM, Omar Ocegueda notifications@github.com
wrote:

Thanks Ariel, I think it's ok for now because it appears to happen under
very specific conditions. I will try to reproduce the failure again anyway:
affine registration does not consume a lot of memory, it should be fine to
use even more slices.

On Mon, Dec 7, 2015 at 8:36 PM, Ariel Rokem notifications@github.com
wrote:

That seemed to do the trick:

https://ci.appveyor.com/project/arokem/dipy/build/1.0.503/job/dybb7p58om4vo1ke


Reply to this email directly or view it on GitHub
#787 (comment).

"Cada quien es dueño de lo que calla y esclavo de lo que dice"
-Proverbio chino.
"We all are owners of what we keep silent and slaves of what we say"
-Chinese proverb.

http://www.cimat.mx/~omar


Reply to this email directly or view it on GitHub
#787 (comment).

@omarocegueda

This comment has been minimized.

Copy link
Contributor

omarocegueda commented Dec 8, 2015

I think it is ok to run the tests with 20 slices so we can move on for now (the failure happens under very specific circumstances, so it is unlikely that the users will have problems with it), but I still have to figure out what went wrong to make sure it's not a bug in the registration module.

@arokem arokem force-pushed the arokem:appveyor branch 2 times, most recently from 7b9aaa0 to ae9a4cb Dec 20, 2015

@arokem arokem force-pushed the arokem:appveyor branch from ae9a4cb to d91ec58 Dec 28, 2015

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Dec 30, 2015

@arokem is this PR ready for merging? Do you think that it will be good to introduce the win bot?

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 30, 2015

Almost. The builds on this branch are all-but-one coming back green:

https://ci.appveyor.com/project/arokem/dipy/build/1.0.547

There's still this issue to sort out:
https://ci.appveyor.com/project/arokem/dipy/build/1.0.547/job/2cq3qxukbomlk2as

I think that @omarocegueda is on it. That was the memory leak discussion on gitter.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 30, 2015

Once we get this all green on my fork, we can merge this, and then I will turn on appveyor for the main fork, so every PR here gets tested there as well. Whether this is a good thing or not is a good question... It takes a while to run, that's for sure (about 2 hours per build). On the other hand, more testing on Windows will probably help us identify issues quickly, before things hit the build-bots.

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Dec 30, 2015

Sure let's give it a try. Some questions. Is this a registration related issue failing? Are these bots 64bit?

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 30, 2015

Yes - I believe this is a registration issue. Happens only on Python 3.3 -- presumably because of the memory leak that @omarocegueda and I were discussing on gitter a few days ago.

And yes: I think this is 64 bit. There's a print out of the system info at the top, and it says: "System type: x64-based PC".

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Dec 30, 2015

Cool. :)

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Dec 30, 2015

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 30, 2015

Oh - good question. Let me try changing that.

On Wed, Dec 30, 2015 at 12:01 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

What is PYTHON_ARCH=32 at the top of
https://ci.appveyor.com/project/arokem/dipy/build/1.0.547/job/pmkj41cykylbek18


Reply to this email directly or view it on GitHub
#787 (comment).

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 30, 2015

That seems to have done the trick! All the Appveyor runs come back green now (but you see how long that took...): https://ci.appveyor.com/project/arokem/dipy/build/1.0.567

So - it seems that you can merge this in now, and once you do, I can flip the switch and everyone will need to rebase all their PRs...

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 31, 2015

Hmm. Maybe need to proceed with caution. The same machine failed on a subsequent run: https://ci.appveyor.com/project/arokem/dipy/build/1.0.568/job/h9ipqqf5o1wb5i0y

@arokem arokem force-pushed the arokem:appveyor branch from 68b3dfb to b1f2b70 Jan 11, 2016

@arokem arokem force-pushed the arokem:appveyor branch 3 times, most recently from bc0c7e1 to 9dcace7 Jan 19, 2016

@arokem arokem referenced this pull request Feb 3, 2016

Closed

Use appveyor for Windows CI? #643

@arokem arokem force-pushed the arokem:appveyor branch from 9dcace7 to 20fdb5d Feb 14, 2016

@arokem arokem force-pushed the arokem:appveyor branch from 20fdb5d to 7d6e38c Apr 15, 2016

@arokem arokem force-pushed the arokem:appveyor branch from 7d6e38c to be604ae Jun 9, 2016

@arokem arokem force-pushed the arokem:appveyor branch from eecc70b to 23f4294 Aug 9, 2018

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Aug 10, 2018

OK - looks like Travis is happy now. Reductions in coverage are because of the added LOC in the tools folder and appveyor file, I believe. I think that we are now ready to merge this and flip the switch on for Appveyor to monitor the repo.

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Aug 10, 2018

Good for me too! @Garyfallidis, can you have a look?

I will merge this PR tomorrow if there is any other comment

@arokem arokem changed the title WIP: TST: Add an appveyor starter file. TST: Add an appveyor starter file. Aug 14, 2018

for ttype in sorted(factors):
dim = ttype[1]
if dim == 2:
nslices = 1
else:
nslices = 45
nslices = 20

This comment has been minimized.

@Garyfallidis

Garyfallidis Aug 15, 2018

Member

Hello @skoudoro, I see you have reduced the number of slices. I suppose this is for speeding up the tests? Maybe this will create problems in the future. Let's keep an eye for this.

This comment has been minimized.

@skoudoro

skoudoro Aug 15, 2018

Member

I think we can put it back as it was. This was not the main reason of the infinite build. It should ok now. Can you do that @arokem please? Thank you

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Aug 15, 2018

@arokem

This comment has been minimized.

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Sep 15, 2018

This error is related to #1266. This test compares the computation time of 2 algorithms but it doesn't seem to be reliable on windows because it failed 7/10 times since we merge it.

I think it would be better if we comment this assert and create an issue to understand it later.

What do you think @arokem?

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Sep 16, 2018

Let me try that.

TST: Test this only on non-windows systems.
See: #787 (comment)

Also: reorder imports to follow conventions.
@arokem

This comment has been minimized.

Copy link
Member

arokem commented Sep 16, 2018

OK. Seems to work. If no one else minds, I suggest that we merge this now, flip the Appveyor switch on for our repo, and keep an issue open referring to the changes in ec68068

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Sep 16, 2018

+1, I can merge it

@skoudoro skoudoro merged commit 0a144bd into nipy:master Sep 18, 2018

2 of 3 checks passed

codecov/patch 83.33% of diff hit (target 87.34%)
Details
codecov/project 87.34% (+<.01%) compared to 5a6aa5a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Sep 18, 2018

Thanks @arokem! 🎉

it is time to activate appveyor!! 😄

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Sep 18, 2018

Boom!

Are you going to flip on the Appveyor switch, or do you want me to do that?

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Sep 18, 2018

I let you this pleasure 😄

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Sep 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment