search

ELVIS-Project / vis-framework

Thoroughly modern symbolic musical data analysis suite.
http://elvisproject.ca/
31 stars 6 forks source link

Travis-CI builds are failing on master branch #354

Closed crantila closed 9 years ago

crantila commented 9 years ago

And they have been for months. And on other branches. There are two types of failures: one is an error in the Python 3.3 build caused by a bug in the latest "stable" music21 release. At this point, we might as well simply disable the Python 3.3 build. Aside from that, all the builds are failing for some reason.

VIS used to always pass its tests, but this is... wait for it... a "travisty." :dancer:

mrbannon commented 9 years ago

I'm working on this now, actually.

mrbannon commented 9 years ago

@crantila 3.4 seems to be failing, too. Looking at the travis log it doesn't seem to like virtualenv...? (Note: I've had to add a virtualenv setting for travis so I can install lapack and gfortran. scipy depends on this stuff, and Alex depends on scipy.)

As for why it wasn't working earlier, it was because mock wasn't being install in the requirements. I've made a 'travis_requirements.txt' especially for travis (duh). Plus, I have to

crantila commented 9 years ago

Yeah, I noticed you started doing stuff right after I posted this... awkward coincidence. I'll take a look at your questions.

On this topic though, do you have the keys to the relevant Travis-CI and Coveralls.io accounts? I don't remember if their permissions automatically reflect what's on GitHub.

mrbannon commented 9 years ago

Coveralls.io is a third-party app for GitHub. No problem there. As for Travis, it's a webhook. So, I don't think any keys are required.

On Thu, Aug 13, 2015 at 5:22 PM, Christopher Antila < notifications@github.com> wrote:

Yeah, I noticed you started doing stuff right after I posted this... awkward coincidence. I'll take a look at your questions.

On this topic though, do you have the keys to the relevant Travis-CI and Coveralls.io accounts? I don't remember if their permissions automatically reflect what's on GitHub.

— Reply to this email directly or view it on GitHub https://github.com/ELVIS-Project/vis-framework/issues/354#issuecomment-130848388 .

mrbannon commented 9 years ago

Here's the deal:

So, builds are gonna be a failin' until I can sort out the travis timeout issue. Working on it now

On Thu, Aug 13, 2015 at 5:24 PM, Michael Ryan Bannon ryan.bannon@gmail.com wrote:

Coveralls.io is a third-party app for GitHub. No problem there. As for Travis, it's a webhook. So, I don't think any keys are required.

On Thu, Aug 13, 2015 at 5:22 PM, Christopher Antila < notifications@github.com> wrote:

Yeah, I noticed you started doing stuff right after I posted this... awkward coincidence. I'll take a look at your questions.

On this topic though, do you have the keys to the relevant Travis-CI and Coveralls.io accounts? I don't remember if their permissions automatically reflect what's on GitHub.

— Reply to this email directly or view it on GitHub https://github.com/ELVIS-Project/vis-framework/issues/354#issuecomment-130848388 .

crantila commented 9 years ago

I'm a little confused. Is there a reason you've changed the VERSION_HISTORY so that some of the existing changes, like Python 3 support, are supposedly slated for version 2.3 instead of 2.2?

Regarding "order of things," my feeling is that if your commit makes a test fail, you are responsible for resolving the situation before merging the commit to master. Commenting a test so you can merge to master is like taking out a seatbelt because it's sort of annoying to wear. The reason to have staging branches at all is to fix issues like this before they end up on the master branch, which can be released at any time.

I accept that I'm no longer participating in this project, and you may not want to use your branches like this, or be bothered by failing Travis builds. It just seems a lot more complicated to sort out now that there are many troublesome issues (the commented tests, dendrogram dependencies, and something about multiprocessing) all messing around with each other.

mrbannon commented 9 years ago

see below

On Thu, Aug 13, 2015 at 5:44 PM, Christopher Antila < notifications@github.com> wrote:

I'm a little confused. Is there a reason you've changed the VERSION_HISTORY so that some of the existing changes, like Python 3 support, are supposedly slated for version 2.3 instead of 2.2?

When the Python version 3 support was implemented the version history was not updated. That's all.

Regarding "order of things," my feeling is that if your commit makes a test fail, you are responsible for resolving the situation before merging the commit to master. Commenting a test so you can merge to master is like taking out a seatbelt because it's sort of annoying to wear. The reason to have staging branches at all is to fix issues like this before they end up on the master branch, which can be released at any time.

Yes, I know. I screwed up and I'm trying to resolve it.

I accept that I'm no longer participating in this project, and you may not want to use your branches like this, or be bothered by failing Travis builds. It just seems a lot more complicated to sort out now that there are many troublesome issues (the commented tests, dendrogram dependencies, and something about multiprocessing) all messing around with each other.

See above.

— Reply to this email directly or view it on GitHub https://github.com/ELVIS-Project/vis-framework/issues/354#issuecomment-130855021 .

crantila commented 9 years ago

Okay, I'm sorry for getting frustrated, it's not really going to help anyone. There are a number of us who could have done/said something sooner, and nobody did, and as far as I'm aware nothing bad even happened yet, so we'll just sort it out. I'm looking into the Python 3.4 virtualenv thing.

mrbannon commented 9 years ago

Don't worry about that. I know what the issue is. Right now I'm just going to rollback master.

On Thu, Aug 13, 2015 at 5:51 PM, Christopher Antila < notifications@github.com> wrote:

Okay, I'm sorry for getting frustrated, it's not really going to help anyone. There are a number of us who could have done/said something sooner, and nobody did, and as far as I'm aware nothing bad even happened yet, so we'll just sort it out. I'm looking into the Python 3.4 virtualenv thing.

— Reply to this email directly or view it on GitHub https://github.com/ELVIS-Project/vis-framework/issues/354#issuecomment-130859269 .

mrbannon commented 9 years ago

I've reverted back to 2.1.3 on master. Travis is working on 2.7, but not 3.3 and 3.4. I will address these issues on Monday.

On Thu, Aug 13, 2015 at 5:52 PM, Michael Ryan Bannon ryan.bannon@gmail.com wrote:

Don't worry about that. I know what the issue is. Right now I'm just going to rollback master.

On Thu, Aug 13, 2015 at 5:51 PM, Christopher Antila < notifications@github.com> wrote:

Okay, I'm sorry for getting frustrated, it's not really going to help anyone. There are a number of us who could have done/said something sooner, and nobody did, and as far as I'm aware nothing bad even happened yet, so we'll just sort it out. I'm looking into the Python 3.4 virtualenv thing.

— Reply to this email directly or view it on GitHub https://github.com/ELVIS-Project/vis-framework/issues/354#issuecomment-130859269 .

mrbannon commented 9 years ago

@alexandermorgan Please take a look at https://travis-ci.org/ELVIS-Project/vis-framework/builds/75535475

If you look at the end of each log (for both Python 2.7 and 3.4) you'll see the errors that are currently causing Travis to fail.

For the Python 3.4 issue, I imagine this is a syntax issue? For the Python 2.7 issue, I'm not really sure. Could you take a guess? (I'll keep investigating.)

mrbannon commented 9 years ago

@alexandermorgan

Regarding 2.7, I see the issue: I think matplot is expecting $DISPLAY to be set. What should this env. variable be set to? Or is there another way to do the test without "displaying" anything? Rather, is it pyplot?

ahankinson commented 9 years ago

Since this is a library, there should be nothing in the core that calls out to a windowing toolkit. Perhaps this would be greatly simplified if the vis part simply produced the data and then an auxiliary script (not part of vis) did the drawing.

If we start calling out to desktop dependant code it will make it more difficult to use this in a server environment. This is what we're seeing now with the Travis build.

If the rendering is moved from the core, will this also remove the dependancy on scipy?

mrbannon commented 9 years ago

Excellent point.

I'm not sure that this will remove the script dep, although I can't answer that for certain. That's Alex's call. On Aug 14, 2015 09:59, "Andrew Hankinson" notifications@github.com wrote:

Since this is a library, there should be nothing in the core that calls out to a windowing toolkit. Perhaps this would be greatly simplified if the vis part simply produced the data and then an auxiliary script (not part of vis) did the drawing.

If we start calling out to desktop dependant code it will make it more difficult to use this in a server environment. This is what we're seeing now with the Travis build.

If the rendering is moved from the core, will this also remove the dependancy on scipy?

— Reply to this email directly or view it on GitHub https://github.com/ELVIS-Project/vis-framework/issues/354#issuecomment-131113620 .

alexandermorgan commented 9 years ago

Even if we don't do any dendrogram rendering in VIS, we would still need a scipy function to create the data, though I guess we could potentially not have the matplotlib dependency with this work around. Also, we could go one step further and just send the arguments that the required scipy function (linkage_matrix()) needs and then I believe we wouldn't need scipy directly in VIS anymore. Personally I don't like the idea of removing scipy just to pass the Travis tests.

ahankinson commented 9 years ago

Looking at the code, I don't think scipy should be removed, but matplotlib should definitely be factored out. We would want this to be usable by any plotting library, both on the desktop and on the server and there is (right now) a hard dependency on matplotlib for this experimenter, which forces a hard dependency on there being a display available (where it's looking for the $DISPLAY variable).

alexandermorgan commented 9 years ago

@mrbannon , ok I checked out the DISPLAY issue and it looks like it should be set to 'Agg'. See this stack overflow question, especially Chris Q.'s answer as well as the approved answer: http://stackoverflow.com/questions/2801882/generating-a-png-with-matplotlib-when-display-is-undefined

mrbannon commented 9 years ago

K, thanks. Still, could you address Andrew's question? Is matplotlib necessary? On Aug 14, 2015 11:14 AM, "Alexander Morgan" notifications@github.com wrote:

@mrbannon https://github.com/mrbannon , ok I checked out the DISPLAY issue and it looks like it should be set to 'Agg'. See this stack overflow question, especially Chris Q.'s answer as well as the approved answer: http://stackoverflow.com/questions/2801882/generating-a-png-with-matplotlib-when-display-is-undefined

— Reply to this email directly or view it on GitHub https://github.com/ELVIS-Project/vis-framework/issues/354#issuecomment-131143392 .

ahankinson commented 9 years ago

I think that's still not quite addressing the underlying architectural design change. I propose that vis be treated solely as a data generator, and leave display of the data to another component. If that means keeping scipy around I'm OK with that, but I definitely think matplotlib should be moved out of vis core since it's only there for display, not data generation.

mrbannon commented 9 years ago

Agreed On Aug 14, 2015 11:18 AM, "Andrew Hankinson" notifications@github.com wrote:

I think that's still not quite addressing the underlying architectural design change. I propose that vis be treated solely as a data generator, and leave display of the data to another component. If that means keeping scipy around I'm OK with that, but I definitely think matplotlib should be moved out of vis core since it's only there for display, not data generation.

— Reply to this email directly or view it on GitHub https://github.com/ELVIS-Project/vis-framework/issues/354#issuecomment-131144912 .

ahankinson commented 9 years ago

@alexandermorgan I can also do a code review on your changes if you like.

mrbannon commented 9 years ago

Alex, I see your point, though. You want vis to be a one stop shop for your analysis needs. But I'm leaning towards Andrew's view.

Having said that, in rodan we can create graph rendering jobs that take the scipy output. On Aug 14, 2015 11:19 AM, "Michael Ryan Bannon" ryan.bannon@gmail.com wrote:

Agreed On Aug 14, 2015 11:18 AM, "Andrew Hankinson" notifications@github.com wrote:

I think that's still not quite addressing the underlying architectural design change. I propose that vis be treated solely as a data generator, and leave display of the data to another component. If that means keeping scipy around I'm OK with that, but I definitely think matplotlib should be moved out of vis core since it's only there for display, not data generation.

— Reply to this email directly or view it on GitHub https://github.com/ELVIS-Project/vis-framework/issues/354#issuecomment-131144912 .

alexandermorgan commented 9 years ago

I guess it would be possible not to have matplotlib if I cut out some code, but we would loose several of the graphing features, like the possibility to annotate the dendrogram connections with their specific dissimilarity readings. I don't think it's possible to offer highly customizable visual output without deciding which rendering library we will use so my vote is to keep matplotlib.

ahankinson commented 9 years ago

As a nice middle ground, we could ship vis with an experiments folder, where we put auxilliary scripts. Then you can put any code that runs on vis there, while still not making a hard dependency on rendering.

alexandermorgan commented 9 years ago

To put it another way, lines 294-313 of the dendrogram script wouldn't work without matplotlib. It's debatable whether or not these lines go beyond just visual rendering: https://github.com/ELVIS-Project/vis-framework/blob/develop/vis/analyzers/experimenters/dendrogram.py

alexandermorgan commented 9 years ago

That sounds like a good solution to me Andrew. Note that there is already a folder for auxilliary scripts. It's "vis-framework/vis/scripts".

mrbannon commented 9 years ago

There is a folder called elements currently. I'm not sure at the moment I'd it's tested out not. (I think it is.) It holds different types of pre-baked analysis scripts.

Alex, are those useful?. I question whether they should be part of the framework proper. On Aug 14, 2015 11:25 AM, "Andrew Hankinson" notifications@github.com wrote:

As a nice middle ground, we could ship vis with an experiments folder, where we put auxilliary scripts. Then you can put any code that runs on vis there, while still not making a hard dependency on rendering.

— Reply to this email directly or view it on GitHub https://github.com/ELVIS-Project/vis-framework/issues/354#issuecomment-131148412 .

crantila commented 9 years ago

First, from the sounds of it, your code there is mixing steps that ought to be separated into independent Experimenters. Second, I suggest using the "scripts" directory, as Alex literally said just now, because that's how the bar charts are made.

It might be a bit of a challenge to make sure you can export everything you need from the Experimenter, Alex, but it's probably worth it.

mrbannon commented 9 years ago

Experiments, not elements. Sorry. (Typing in a car) On Aug 14, 2015 11:32 AM, "Michael Ryan Bannon" ryan.bannon@gmail.com wrote:

There is a folder called elements currently. I'm not sure at the moment I'd it's tested out not. (I think it is.) It holds different types of pre-baked analysis scripts.

Alex, are those useful?. I question whether they should be part of the framework proper. On Aug 14, 2015 11:25 AM, "Andrew Hankinson" notifications@github.com wrote:

As a nice middle ground, we could ship vis with an experiments folder, where we put auxilliary scripts. Then you can put any code that runs on vis there, while still not making a hard dependency on rendering.

— Reply to this email directly or view it on GitHub https://github.com/ELVIS-Project/vis-framework/issues/354#issuecomment-131148412 .

crantila commented 9 years ago

Is this in VIS, Ryan? Experimenters are a different type of Indexer.

alexandermorgan commented 9 years ago

Also, the experimenters are tested just about as heavily as the indexers.

mrbannon commented 9 years ago

K. My bad On Aug 14, 2015 11:34 AM, "Christopher Antila" notifications@github.com wrote:

Is this in VIS, Ryan? Experimenters are a different type of Indexer.

— Reply to this email directly or view it on GitHub https://github.com/ELVIS-Project/vis-framework/issues/354#issuecomment-131151740 .

ahankinson commented 9 years ago

Yeah, it looks like 294-313 can be mostly factored out to another script. Users will (likely) want to be in control of labelling, and they probably don't want to dig around in the core of vis to customize it.

It seems the only bit that's actually dependant on the d_data is the zip function call, so it would be pretty easy to just move that out of the indexer wholescale.

A review of the settings would also be in order. There are few options that seem (to me) to be somewhat redundant or confusing; like if return_data is false, and no_plot is true, the function won't produce anything!

alexandermorgan commented 9 years ago

That's not actually true. If you read the documentation on the dendrogram() function here: http://docs.scipy.org/doc/scipy/reference/generated/scipy.cluster.hierarchy.dendrogram.html#scipy.cluster.hierarchy.dendrogram you'll see that if no_plot is set to True, it just won't produce any visuals. This is precisely for circumstances where you just want the data and don't want to render anything. In addition to this, I override the user's setting for no_plot to always make sense with the type of output requested.

mrbannon commented 9 years ago

BTW, if you guys do decide to lose matplotlib (in the sense that is not necessary for tests), please update the travis requirements. Txt file accordingly. Tx On Aug 14, 2015 11:42 AM, "Alexander Morgan" notifications@github.com wrote:

That's not actually true. If you read the documentation on the dendrogram() function here:

http://docs.scipy.org/doc/scipy/reference/generated/scipy.cluster.hierarchy.dendrogram.html#scipy.cluster.hierarchy.dendrogram you'll see that if no_plot is set to True, it just won't produce any visual data. This is precisely for circumstances where you just want the data and don't want to render anything. In addition to this, I override the user's setting for no_plot to always make sense with the type of output requested.

— Reply to this email directly or view it on GitHub https://github.com/ELVIS-Project/vis-framework/issues/354#issuecomment-131154719 .

alexandermorgan commented 9 years ago

Also, users don't have to dig around the core of vis to control the labelling of dendrograms. They're completely controlled by the user-provided graph_settings argument.

ahankinson commented 9 years ago

OK, well, it looks like dendogram() returns all the data you need for plotting anyway, right? Can you simply pass d_data to plt.plot and get the same results.

alexandermorgan commented 9 years ago

Not if you want axis labels, connection annotations, etc.

alexandermorgan commented 9 years ago

Perhaps the three of us could just talk about this in person before the meeting this afternoon.

ahankinson commented 9 years ago

Sorry, but I don't see that -- it looks like you're doing all the labelling outside of the dendogram call anyway -- Lines 305, 306, 307 for example.

mrbannon commented 9 years ago

I'm on my way to Toronto, so I can't meet. Whatever you guys decide I'm sure will be fine.

Note t that the travis yaml file is in a good state, so no need to mess with that. On Aug 14, 2015 11:51 AM, "Alexander Morgan" notifications@github.com wrote:

Perhaps the three of us could just talk about this in person before the meeting this afternoon.

— Reply to this email directly or view it on GitHub https://github.com/ELVIS-Project/vis-framework/issues/354#issuecomment-131157835 .

alexandermorgan commented 9 years ago

@ahankinson and @collectivewisdom, before I close this issue (because the Travis builds are passing again), I want to see what people think of an alternative solution to putting a separate file in the scripts folder. If we just import matplotlib.pyplot only in the event that the user requests a visual output then we're equally not dependant on matplotlib but our code is much simpler and in my opinion easier to understand. This alternative has the advantage of avoiding the complication of saving data to a file only to immediately open it and manipulate it with another python script. I've made this change to the alex_devel branch so you can look at it. It's this commit: https://github.com/ELVIS-Project/vis-framework/commit/97a3b741e06fec854e647d4baf64c2b43338f0f6 Having an import statement inside of a function instead of at the top of the script is useful for just this sort of dependency issue, as noted in the approved answer of this stack overflow question: http://stackoverflow.com/questions/128478/should-python-import-statements-always-be-at-the-top-of-a-module

ahankinson commented 9 years ago

If you want to do that, fine. I was just trying to help. Personally, I think you're making a big mistake by mixing it, but I'm not going to put any more energy into this issue.

I'll leave it up to @mrbannon and the others who are working on this to accept it or not.

crantila commented 9 years ago

I'm not sure I see all the issues Andrew has in mind, but I'm also not sure this "import in a function" is a good idea.

The PEP 8 is pretty clear about where to put import statements, and since neither solution is terrific, it seems better to follow the established convention. Plus, having an entire "if" suite that will never be executed with the automated test suite really feels like an invitation for trouble.