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

OpenFisca-UK integration #2

Merged
merged 21 commits into from
Apr 21, 2021
Merged

Conversation

jdebacker
Copy link
Member

This PR integrates OpenFisca-UK into oguk_calibration. This microsimulation model will provide the underlying data on marginal and effective tax rates that will go into the UK calibration of the OG model.

@jdebacker
Copy link
Member Author

@nikhilwoodruff Can you look at the testing failures and see if you can identify why the conflict in setting up the environment arises? The issue is described here:

  The conflict is caused by:
1093
      The user requested openfisca-core 35.0.0 (from git+https://github.com/nikhilwoodruff/openfisca-core.git)
1094
      openfisca-uk 0.2.2 depends on openfisca-core 35.0.0 (from git+https://github.com/nikhilwoodruff/openfisca-core)
  ERROR: Cannot install -r /home/runner/work/OG-UK-Calibration/OG-UK-Calibration/condaenv.1lzvviqe.requirements.txt (line 5) and openfisca-core 35.0.0 (from git+https://github.com/nikhilwoodruff/openfisca-core.git) because these package versions have conflicting dependencies.
1128
  ERROR: ResolutionImpossible: for help visit https://pip.pypa.io/en/latest/user_guide/#fixing-conflicting-dependencies

@rickecon
Copy link
Member

rickecon commented Apr 21, 2021

@jdebacker and @nikhilwoodruff. As @jdebacker notes above, the error states:

The conflict is caused by:
      The user requested openfisca-core 35.0.0 (from git+https://github.com/nikhilwoodruff/openfisca-core.git)
      openfisca-uk 0.2.2 depends on openfisca-core 35.0.0 (from git+https://github.com/nikhilwoodruff/openfisca-core)
  
  To fix this you could try to:
  1. loosen the range of package versions you've specified
  2. remove package versions to allow pip attempt to solve the dependency conflict

This is going to take some detective work to figure out what the conflict is. It sounds like it is a conflict between some package in our environment.yml and the openfisca-core 35.0.0.

I tried fixing the Python version in environment.yml to python==3.7.7 and removing the ability to accommodate Python 3.8 and 3.9 in setup.py in a PR that I submitted to @jdebacker's branch. But that gave the same error. That PR is now closed and not merged.

@nikhilwoodruff
Copy link
Collaborator

nikhilwoodruff commented Apr 21, 2021

Thanks @rickecon and @jdebacker it's possible openfisca-uk has some lack of support (purely because of not specifying different package versions) for Python above 3.7 - I'll have a look and see if I can fix it. Not this - see below

Copy link
Collaborator

@nikhilwoodruff nikhilwoodruff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdebacker @rickecon think I found the issue - I've seen it before in other places but only occasionally.

environment.yml Outdated
- git+https://github.com/PSLmodels/OG-USA.git
- git+https://github.com/nikhilwoodruff/openfisca-core.git
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdebacker I think this is causing the issue - the openfisca-uk requirements require:

- git+https://github.com/nikhilwoodruff/openfisca-core

rather than with the .git. It might be easier just to drop this requirement for the sake of clarity and let it be installed as an openfisca-uk dependency?

environment.yml Outdated
- git+https://github.com/PSLmodels/OG-USA.git
- git+https://github.com/nikhilwoodruff/openfisca-core
- git+https://github.com/nikhilwoodruff/frs.git
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- git+https://github.com/nikhilwoodruff/frs.git
- git+https://github.com/nikhilwoodruff/frs

environment.yml Outdated
- git+https://github.com/PSLmodels/OG-USA.git
- git+https://github.com/nikhilwoodruff/openfisca-core
- git+https://github.com/nikhilwoodruff/frs.git
- git+https://github.com/PSLmodels/openfisca-uk.git
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- git+https://github.com/PSLmodels/openfisca-uk.git
- git+https://github.com/PSLmodels/openfisca-uk

environment.yml Outdated
- git+https://github.com/nikhilwoodruff/openfisca-core
- git+https://github.com/nikhilwoodruff/frs.git
- git+https://github.com/PSLmodels/openfisca-uk.git
- git+https://github.com/PSLmodels/microdf.git
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- git+https://github.com/PSLmodels/microdf.git
- git+https://github.com/PSLmodels/microdf

Copy link
Collaborator

@nikhilwoodruff nikhilwoodruff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's the same problem - maybe this just to be on the safe side?

@nikhilwoodruff
Copy link
Collaborator

Just changed the openfisca-uk dependency for microdf (pslmodels/... PSLmodels/...) which should at least fix the current error.

@codecov-commenter
Copy link

codecov-commenter commented Apr 21, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@02cc204). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main       #2   +/-   ##
=======================================
  Coverage        ?   51.76%           
=======================================
  Files           ?        5           
  Lines           ?      736           
  Branches        ?        0           
=======================================
  Hits            ?      381           
  Misses          ?      355           
  Partials        ?        0           
Flag Coverage Δ
unittests 51.76% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 02cc204...8b94be8. Read the comment docs.

@jdebacker jdebacker marked this pull request as ready for review April 21, 2021 15:24
@jdebacker
Copy link
Member Author

Thanks @nikhilwoodruff, tests on 3.7 appear to pass now!

@@ -16,11 +16,12 @@
from dask import delayed, compute
import dask.multiprocessing
import pickle
from ogusa_calibrate import get_micro_data
from og_uk_calibrate import get_micro_data
import ogusa.parameter_plots as pp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdebacker. I make a point in Issue #9 to eventually move all parameter_plots functionality into the OG-UK-Calibration repo as is started in PR #3. But we don't need to do that right now for this PR.

@rickecon
Copy link
Member

I have reviewed all the changes. This looks good to me. @jdebacker are you ready for me to merge this? Thanks, @nikhilwoodruff for the help.

@jdebacker
Copy link
Member Author

@rickecon Yes, this PR is ready to merge. Thanks for reviewing!

@rickecon rickecon merged commit 9b4d7ac into PSLmodels:main Apr 21, 2021
@jdebacker jdebacker mentioned this pull request Apr 21, 2021
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.

None yet

4 participants