search

PSLmodels / Tax-Calculator

USA Federal Individual Income and Payroll Tax Microsimulation Model
https://taxcalc.pslmodels.org
Other
255 stars 154 forks source link

AppVeyor testing on Windows has stopped working #2204

Closed martinholmer closed 5 years ago

martinholmer commented 5 years ago

About 25 months ago, @talumbau introduced in PR #1111 the execution of pytest on Windows for each GitHub pull request using the AppVeyor service. The testing has worked ever since. It is far slower than the Travis-CI execution of pytest on Linux; so much so that sometimes the development process is slowed down waiting for AppVeyor to run the tests. But it has worked.

A few days ago, it stopped working. The Travis-CI tests under both Python 3.6 and 3.7 continue to work fine.

After spending some time trying to diagnose and fix this problem, I've concluded that I don't know nearly enough about how AppVeyor works to fix this problem. For example, I can't even figure out how to make it use Python 3.6 to run the tests. It seems to want to use the latest Python 3.7 version. Look at the AppVeyor test failures for Tax-Calculator pull request #2203 for an example of the (cryptic) messages being generated over the past few days since this problem appeared.

If this problem can't be fixed soon, I vote to drop the whole AppVeyor service and rely on the Travis-CI service.

@MattHJensen @hdoupe

hdoupe commented 5 years ago

@martinholmer This seems like another fun build error. It looks like the issue comes up when bokeh imports PIL: 

taxcalc\parameters.py:11: in <module>
    from taxcalc.utils import read_egg_json, json_to_dict
taxcalc\utils.py:17: in <module>
    import bokeh.io as bio
C:\Miniconda36-x64\envs\taxcalc-dev\lib\site-packages\bokeh\io\__init__.py:51: in <module>
    from .export import export_png
C:\Miniconda36-x64\envs\taxcalc-dev\lib\site-packages\bokeh\io\export.py:34: in <module>
    from ..embed import file_html
C:\Miniconda36-x64\envs\taxcalc-dev\lib\site-packages\bokeh\embed\__init__.py:54: in <module>
    from .server import server_document
C:\Miniconda36-x64\envs\taxcalc-dev\lib\site-packages\bokeh\embed\server.py:30: in <module>
    from ..resources import DEFAULT_SERVER_HTTP_URL
C:\Miniconda36-x64\envs\taxcalc-dev\lib\site-packages\bokeh\resources.py:42: in <module>
    from .model import Model
C:\Miniconda36-x64\envs\taxcalc-dev\lib\site-packages\bokeh\model.py:33: in <module>
    from .core.properties import Any, Dict, Instance, List, String
C:\Miniconda36-x64\envs\taxcalc-dev\lib\site-packages\bokeh\core\properties.py:255: in <module>
    from .property.dataspec import AngleSpec; AngleSpec
C:\Miniconda36-x64\envs\taxcalc-dev\lib\site-packages\bokeh\core\property\dataspec.py:40: in <module>
    from .visual import FontSize, MarkerType
C:\Miniconda36-x64\envs\taxcalc-dev\lib\site-packages\bokeh\core\property\visual.py:29: in <module>
    import PIL.Image
C:\Miniconda36-x64\envs\taxcalc-dev\lib\site-packages\PIL\Image.py:94: in <module>
    from . import _imaging as core
E   ImportError: DLL load failed: The specified module could not be found.

I found this issue in the PIL repo: https://github.com/python-pillow/Pillow/issues/2945

It seems like people found multiple solutions to the problem:

Fixing these build errors seems like more art than science most of the time. I hope one of these commands resolves the issue. If they aren't successful, we could remove the windows build while we are still digging into the problem. We have a good number of users at AEI who are using windows. So, it's nice to know that Tax-Calculator is passing the tests on a windows machine.

martinholmer commented 5 years ago

@hdoupe said:

It looks like the issue comes up when bokeh imports PIL: ... I found this issue in the PIL repo: python-pillow/Pillow#2945

@hdoupe, thanks for the diagnosis. It seem like the Anaconda people haven't been able to resolve this problem in over a year!

martinholmer commented 5 years ago

@hdoupe said in issue #2204:

If [these suggested fixes] aren't successful, we could remove the windows build while we are still digging into the problem. ... So, it's nice to know that Tax-Calculator is passing the tests on a windows machine.

I don't understand this line of thinking. If the tests run smoothly on a Mac under Python 3.6 and run smoothly on Linux user both Python 3.6 and Python 3.7, why would there be a problem with Tax-Calculator code running under Windows?

@hdoupe also said:

We have a good number of users at AEI who are using windows.

I'm surprised to hear this. There has not been even one download of Windows taxcalc packages for either version 0.23.3 or 0.24.0 (the two most recent versions). If you're correct, then nobody has told them they need to keep current. The documentation makes that clear. So if you are correct, the "good number of" AEI users either don't read the documentation or don't follow the procedures it recommends.

martinholmer commented 5 years ago

@hdoupe said in Tax-Calculator issue #2204:

it's nice to know that Tax-Calculator is passing the tests on a windows machine.

Yes, it would be "nice to know" is the costs of running the Windows tests were less than the benefits of knowing for certain.

But running Windows tests does not seem to be a PSL requirement. Besides Tax-Calculator and Behavioral-Responses, there are now two other "PSL cataloged" models: OG-USA and B-Tax, neither of which run Windows tests under AppVeyor.

Because the costs have recently risen substantially and the benefits are marginal and I don't have the knowledge or time to fix this problem, I think I'll just remove the AppVeyor service from the Tax-Calculator and Behavioral-Responses repositories.

@MattHJensen, are you OK with this plan?

hdoupe commented 5 years ago

@martinholmer Dropping the windows appveyor build makes since to me given these issues. I think the most common problem for writing python code that is compatible with windows, mac, and linux is keeping the different file path separators straight. Tax-Calculator uses the os.path module for building up the paths. So, I don't see any problems there--it's just something to keep in mind.

MattHJensen commented 5 years ago

Removing AppVeyor makes sense to me.

martinholmer commented 5 years ago

@hdoupe said in Tax-Calculator issue #2204:

Dropping the windows appveyor [testing] makes sense to me given these issues. I think the most common problem for writing python code that is compatible with windows, mac, and linux is keeping the different file path separators straight. Tax-Calculator uses the os.path module for building up the paths. So, I don't see any problems there--it's just something to keep in mind.

@hdoupe, thanks for you thoughts on this.

martinholmer commented 5 years ago

A week after opening this issue AppVeyor began working again (on 2019-01-29) without any changes in the the code in this repository. This suggests what many have suspected: that AppVeyor is not only very slow but a bit flaky.

@MattHJensen @hdoupe