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

Remove mkl dependency from environment.yml and setup.py #840

Merged
merged 3 commits into from
Jan 5, 2023

Conversation

rickecon
Copy link
Member

@rickecon rickecon commented Jan 4, 2023

@jdebacker. This PR removes the mkl Intel processor numerical computation accelerator package dependency from environment.yml and setup.py. @SeaCelo noted in Issue #821 that the mkl dependency is incompatible with Apple M1 processor machines that use the M1 optimized Python distribution from Anaconda. I ran some speed tests (reported here) on Linux and the two relevant specifications of Mac OSx to see if removing the mkl package caused any big slowdowns in computation time, which it does not.

If all our CI tests pass, and the full set of tests pass on my machine, then I propose we merge this update. This will make loading the OG-Core environment and any of the country-specific calibrations compatible with all current Windows, Linux, and Mac operating system and anaconda configurations.

@rickecon rickecon linked an issue Jan 4, 2023 that may be closed by this pull request
@rickecon rickecon added the bug label Jan 4, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 4, 2023

Codecov Report

Merging #840 (b95261f) into master (86b8a3c) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #840   +/-   ##
=======================================
  Coverage   82.24%   82.24%           
=======================================
  Files          18       18           
  Lines        3981     3981           
=======================================
  Hits         3274     3274           
  Misses        707      707           
Flag Coverage Δ
unittests 82.24% <100.00%> (ø)

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

Impacted Files Coverage Δ
ogcore/__init__.py 100.00% <100.00%> (ø)

@rickecon
Copy link
Member Author

rickecon commented Jan 4, 2023

@jdebacker. I removed the conda.recipe/ folder from the repo because I don't think we use it since we committed to PyPI.org.

@rickecon
Copy link
Member Author

rickecon commented Jan 5, 2023

@jdebacker. This passed all tests locally on my machine.

============================= test session starts ==============================
platform darwin -- Python 3.10.8, pytest-7.2.0, pluggy-1.0.0
rootdir: /Users/richardevans/Docs/Economics/OSE/OG-Core, configfile: pytest.ini, testpaths: ./tests
plugins: xdist-3.1.0
collected 489 items                                                            

tests/test_SS.py .............................                           [  5%]
tests/test_TPI.py ...........................                            [ 11%]
tests/test_aggregates.py .....................................           [ 19%]
tests/test_basic.py ....                                                 [ 19%]
tests/test_elliptical_u_est.py .......                                   [ 21%]
tests/test_execute.py .                                                  [ 21%]
tests/test_firm.py ..................................................... [ 32%]
.............                                                            [ 34%]
tests/test_fiscal.py ..................                                  [ 38%]
tests/test_household.py ..............................................   [ 48%]
tests/test_output_plots.py ............................................. [ 57%]
                                                                         [ 57%]
tests/test_output_tables.py .............                                [ 59%]
tests/test_parameter_plots.py ...................................        [ 67%]
tests/test_parameter_tables.py .......                                   [ 68%]
tests/test_parameters.py .............                                   [ 71%]
tests/test_run_example.py ..                                             [ 71%]
tests/test_run_ogcore.py .                                               [ 71%]
tests/test_tax.py ......................................                 [ 79%]
tests/test_txfunc.py ..........................                          [ 84%]
tests/test_user_inputs.py .........                                      [ 86%]
tests/test_utils.py .................................................... [ 97%]
.............                                                            [100%]
=========== 489 passed, 77 warnings in 13863.08s (3:51:03) ===========

@jdebacker
Copy link
Member

LGTM and local tests passing on my machine as well. Thanks @rickecon.

@SeaCelo
Copy link

SeaCelo commented Jan 6, 2023

@rickecon, @jdebacker: FYI I cannot run the model if I use this version of the environment.yml to build the conda environment. The model complains about missing pip, pandas-datareader, ogcore, statsmodels. Adding these lines back solves the problem.

I tested by using this version of environment.yml to install the OG-ZAF model:

  1. conda env remove --name ogzaf-dev
  2. conda env create -f environment.yml
  3. conda activate ogzaf-dev
  4. pip install -e .
  5. python run_og_zaf.py

@rickecon rickecon deleted the mkl branch January 8, 2023 08:34
@rickecon
Copy link
Member Author

rickecon commented Jan 9, 2023

@SeaCelo. I submitted a PR to OG-ZAF that updates that repository to use this updated version of OG-Core with no mkl dependency. Everything works well on my machine, which is an Apple M1 chip, and all tests are passing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Intel mkl dependency may break the installation for ARM Macs
4 participants