Skip to content
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 mock as a requirement of Tax-Calc conda recipe and environment.yml #1180

Merged
merged 2 commits into from
Mar 1, 2017

Conversation

PeterDSteinberg
Copy link
Contributor

Fixes #1173 (ImportError related to mock on Windows). I have encountered that mock import error on other projects. I'm not sure why Windows differs from other OS's in terms of mock being there or not, but this PR will ensure it is always installed when doing conda install -c ospc taxcalc or conda env create from the source directory.

In the meantime (before the next Tax-Calculator conda packages are built) if you encounter an ImportError regarding mock, then do the following to install mock:

conda install mock

@codecov-io
Copy link

codecov-io commented Feb 6, 2017

Codecov Report

Merging #1180 into master will not impact coverage.

@@           Coverage Diff           @@
##           master    #1180   +/-   ##
=======================================
  Coverage   98.88%   98.88%           
=======================================
  Files          38       38           
  Lines        3063     3063           
=======================================
  Hits         3029     3029           
  Misses         34       34

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f021633...47f88a5. Read the comment docs.

@martinholmer
Copy link
Collaborator

@PeterDSteinberg, Why are you adding mock to environment.yml in #1180 when it is already there?

@PeterDSteinberg
Copy link
Contributor Author

@martinholmer Good point ! Oversight fixed in latest commit.

@MattHJensen
Copy link
Contributor

@PeterDSteinberg, given that mock was already in environment.yml, do you still think this solves the particular problem described in #1173 / #1172 ?

@PeterDSteinberg
Copy link
Contributor Author

@MattHJensen - I am assuming that the users experiencing the bug in #1172 / #1173 are users who have installed from the conda package of taxcalc. The conda package does not use the environment.yml, but rather the conda.recipe/meta.yaml that was missing mock.

@zrisher
Copy link
Contributor

zrisher commented Feb 7, 2017

There's an issue for that, fyi: #1118. Not sure if it's an easy fix or not.

@MattHJensen
Copy link
Contributor

I am assuming that the users experiencing the bug in #1172 / #1173 are users who have installed from the conda package of taxcalc.

@PeterDSteinberg, the user was actually installing from source. An interesting observation is that nothing happened when he ran activate taxcalc-dev

@martinholmer
Copy link
Collaborator

@PeterDSteinberg, What is the difference between build: requirements and run: requirements in the conda.recipe/meta.yaml file?

@martinholmer
Copy link
Collaborator

@PeterDSteinberg said:

I am assuming that the users experiencing the bug in #1172 / #1173 are users who have installed from the conda package of taxcalc. The conda package does not use the environment.yml, but rather the conda.recipe/meta.yaml that was missing mock.

OK, but you haven't explained what the environment.yml file is used for. What is its role in all this conda environment magic?

@MattHJensen @feenberg @zrisher

@martinholmer
Copy link
Collaborator

@PeterDSteinberg, While we are waiting for your answers to several questions posed in #1180, I'd like to add a few more questions about the contents of the taxcalc/environment.yml and taxcalc/conda.recipe/meta.yaml files. In the first of those two files, it says this:

IN environment.yml:
   - numpy =1.11.2
   - pandas =0.18.0

while in the second of those two files it says this:

IN meta.yaml:
   - numpy ==1.10.4
   - pandas <=0.18.0

Why are the numpy versions different?
Why can't we be using the current numpy version 1.12.0?
Why cant't we be using the current pandas version 0.19.2?

@MattHJensen @zrisher

@PeterDSteinberg
Copy link
Contributor Author

@martinholmer Thank you for pointing out the inconsistency in pandas and numpy versions. To my knowledge there is no reason why the the conda package conda.recipe/meta.yaml should specify different versions than environment.yml. Travis CI is using the environment.yml's version specs successfully, so my guess is that it is okay to make both the conda package and environment.yml specific the newer numpy and pandas versions. I'll make that change.

@PeterDSteinberg
Copy link
Contributor Author

PeterDSteinberg commented Feb 9, 2017

@martinholmer

OK, but you haven't explained what the environment.yml file is used for. What is its role in all this conda environment magic?

There are two separate pathways in which one may use Tax-Calculator's code:

  • From source: Clone the repo, then do cd Tax-Calculator && conda env create && source activate taxcalc-dev && python setup.py install
  • From conda package: conda install -c ospc taxcalc

In the case of From conda package, then environment.yml has no effect at all - not used in any way. In the case of From source, then the contents of conda.recipe folder have no effect at all - not used in any way.

@martinholmer
Copy link
Collaborator

@PeterDSteinberg said:

There are two separate pathways in which one may use Tax-Calculator's code:

From source: Clone the repo, then do cd Tax-Calculator && conda env create && python setup.py install

From conda package: conda install -c ospc taxcalc

In the case of From conda package, environment.yml has no effect at all - not used in any way.
In the case of From source, the contents of conda.recipe folder have no effect at all - not used in any way.

Thanks for the clear explanation.

Why can't conda use just one file (in both cases) to specify the environment?
I don't understand why source-code vs package would make a difference with what Python environment is required.

@MattHJensen @zrisher

@PeterDSteinberg
Copy link
Contributor Author

@martinholmer This situation happens in many packages I have worked on ("this situation" being having an environment.yml file and a conda.recipe package spec folder), and we have gotten your criticism before about its unclarity. When I'm developing on Tax-Calculator, I find both modes of installation helpful for different reasons:

  • From source: As a Tax-Calculator developer, I prefer to use the From source method in local iterative development. Though not mentioned above, you have the choice of doing either

conda env create && source activate taxcalc-dev && python setup.py install
or
conda env create && source activate taxcalc-dev && python setup.py develop

Both of those commands install the package specs of environment.yml into a named environment. The difference is that setup.py install will copy the source files to the conda environment's site-packages dir (within the Python installation), while setup.py develop effectively allows you to import and use taxcalc from wherever you have it cloned. With setup.py develop, when I modify the source code, the changes are automatically reflected when I restart a Python session (I don't have to setup.py develop again to see my edits work). In contrast, setup.py install means that each time the code is edited, setup.py install has to be run again in order for the changes to be reflected. I find python setup.py develop useful as a developer because it allows me to forget about installation stuff while I am making fast edits.

  • From conda package: I like to have a conda package for Tax-Calculator when deploying in a production setting because taxcalc can then be listed as a dependency in the environment.yml file of some other package. If one wanted to build the conda package locally and install it from the local built packages cache, one could do:

conda build conda.recipe && conda install --use-local taxcalc

I do that command periodically to confirm we haven't broken the conda recipe, however I don't find it as helpful for iterative development locally because the command has to be re-run each time I change any part of the code.

In summary, From Source is faster for iterative development. From conda package gives us more options in production.

Note: I just edited my comment above this one because I forgot to include the source activate taxcalc-dev part after conda env create. The source activate taxcalc-dev part in between conda env create and python setup.py develop (or setup.py install) is critical, otherwise taxcalc will be installed into the root environment if you didn't already have taxcalc-dev activated.

@MattHJensen
Copy link
Contributor

MattHJensen commented Feb 10, 2017

@PeterDSteinberg, do you know why it is necessary to have both a meta.yml and an environment.yml? Why don't both the conda recipe and the developer environment rely on just one .yml file to specify dependencies instead of two separate ones (meta.yml and environment.yml)? This is related to #1118.

@PeterDSteinberg
Copy link
Contributor Author

@MattHJensen In addressing #1158 (streamlining build process), I could change the build process to define meta.yaml requirements by doing yaml.load on the environment.yml and inserting those dependencies in the meta.yaml. That would address the underlying problem in #1118 (that version specs in several places need to be updated simulataneously - an error prone process currently). In making those changes the meta.yaml in the conda.recipe I'd put a comment in meta.yaml that it is just a template to be updated programmatically by the environment.yml.

@martinholmer
Copy link
Collaborator

@PeterDSteinberg said:

In addressing #1158 (streamlining build process), I could change the build process to define meta.yaml requirements by doing yaml.load on the environment.yml and inserting those dependencies in the meta.yaml. That would address the underlying problem in #1118 (that version specs in several places need to be updated simultaneously -- an error prone process currently). In making those changes the meta.yaml I'd put a comment in conda.recipe/meta.yaml that it is just a template to be updated programmatically by the environment.yml.

This sounds like an improvement to me. If others agree, why don't you go ahead and revise #1180 to be like this.

@MattHJensen @zrisher

@martinholmer
Copy link
Collaborator

@PeterDSteinberg, Where do you stand on completing the work you suggested in #1180?

@PeterDSteinberg
Copy link
Contributor Author

In #1158 we discussed deferring the work until after a milestone. This already held me up today some (inefficiency building packages). I'll fix this PR and #1158 tomorrow / Friday. I can come up with a non-invasive way of making build systems for all the packages that will not slow down development towards milestones.

@martinholmer
Copy link
Collaborator

Merging #1180 which is a long-overdo (23 days of inaction) fix of bug reported in #1173.

@martinholmer martinholmer merged commit fce60dd into PSLmodels:master Mar 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants