Closed jdebacker closed 6 months ago
jdebacker
commented
6 months ago Apparently the format of the JSON files from the UN have changed. Using json.loads works better then the requests.response.json at parsing the JSONs, but errors are still occurring after get multiple pages into the results.
cc @rickecon
codecov-commenter
commented
6 months ago Attention: Patch coverage is 66.66667% with 6 lines in your changes are missing coverage. Please review.
Project coverage is 79.84%. Comparing base (
ff2eb25) to head (3d638e8). Report is 2 commits behind head on master.
jdebacker
commented
6 months ago @SeaCelo asked a good question -- why download in JSON format as opposed to CSV?
That solved it -- no more "too many requests" because you get in one page of CSV data what 15+ pages of JSON was giving you.
This PR now makes the switch to retrieve the UN data in CSV format and adds an option to download data from any of the functions which retrieve data from the UN portal. It's easy to use these retrieved data in the get_pop_objs function in a way that results in no pings of the UN portal (see the new test_demographics.py::test_data_download, which provides an example of doing this in the context of the test.
I'd noticed some odd issues in the GH Actions tests (e.g., a failure in TPI.py related to dask distributed processes, which is not affected by this PR), but the changes to the code in demographics.py are ready for review and all of the test_demographics.py test pass. I'll rerun or make necessary adjustments to ensure all tests pass.
jdebacker
commented
6 months ago 3.9 test failure is the UN API throwing an HTTP code 403 -- a denial of access. We will see if this is specific to calls coming from 3.9 (at least one of the 3.10 tests has passed without such an error.
rickecon
commented
6 months ago @jdebacker. I ran all the OG-Core tests locally yesterday and last night (11 hrs, 33 min) and I only got three errors. And the three errors were assert np.allclose() or assert np.all(np.isclose()). I think we should:
demographics.py be local only (@pytest.mark.local) andnp.isclose tolerance parameter in test_txfunc.py so the tests pass on more platforms.
(ogcore-dev) richardevans@Richards-MacBook-Pro-2 OG-Core % pytest
============================= test session starts ==============================
platform darwin -- Python 3.11.8, pytest-8.0.0, pluggy-1.4.0
rootdir: /Users/richardevans/Docs/Economics/OSE/OG-Core
configfile: pytest.ini
testpaths: ./tests
plugins: xdist-3.5.0, anyio-4.2.0
collected 516 items tests/test_SS.py ............................. [ 5%] tests/test_TPI.py ........................... [ 10%] tests/test_aggregates.py ..................................... [ 18%] tests/test_basic.py .... [ 18%] tests/test_demographics.py ................. [ 22%] tests/test_elliptical_u_est.py ....... [ 23%] tests/test_execute.py . [ 23%] tests/test_firm.py ..................................................... [ 33%] ................ [ 37%] tests/test_fiscal.py ................... [ 40%] tests/test_household.py .............................................. [ 49%] tests/test_output_plots.py ............................................. [ 58%] [ 58%] tests/test_output_tables.py ............. [ 60%] tests/test_parameter_plots.py ................................... [ 67%] tests/test_parameter_tables.py ....... [ 68%] tests/test_parameters.py .............. [ 71%] tests/test_run_example.py .. [ 72%] tests/test_run_ogcore.py . [ 72%] tests/test_tax.py ...................................... [ 79%] tests/test_txfunc.py .....F.......F..........F. [ 84%] tests/test_user_inputs.py ......... [ 86%] tests/test_utils.py .................................................... [ 96%] .................. [100%]
=================================== FAILURES =================================== _ test_txfuncest[DEP]
rate_type = 'etr', tax_func_type = 'DEP', numparams = 12 expected_tuple = (array([2.27262567e-23, 6.52115408e-05, 2.58988624e-13, 5.79701075e-09, 3.39354147e-01, 7.60132613e-01, 9.14365...1, 9.02760087e-06, 9.02760087e-06, 3.39345119e-03, 7.60123585e-03, 9.02760087e-06]), 237677.14110076256, 152900) tmpdir = local('/private/var/folders/d4/trj3dssd6s3g8kxvjmczz11w0000gn/T/pytest-of-richardevans/pytest-0/test_txfunc_est_DEP_0')
@pytest.mark.local # only marking as local because platform
# affects results from scipy.opt that is called in this test - so it'll
# pass if run on Mac with MKL, but not necessarily on other platforms
@pytest.mark.parametrize(
"rate_type,tax_func_type,numparams,expected_tuple",
[
("etr", "DEP", 12, expected_tuple_DEP),
("etr", "DEP_totalinc", 6, expected_tuple_DEP_totalinc),
("etr", "GS", 3, expected_tuple_GS),
],
ids=["DEP", "DEP_totalinc", "GS"],
)
def test_txfunc_est(
rate_type, tax_func_type, numparams, expected_tuple, tmpdir
):
"""
Test txfunc.txfunc_est() function. The test is that given
inputs from previous run, the outputs are unchanged.
"""
micro_data = utils.safe_read_pickle(
os.path.join(CUR_PATH, "test_io_data", "micro_data_dict_for_tests.pkl")
)
s = 80
t = 2030
df = txfunc.tax_data_sample(micro_data[str(t)])
output_dir = tmpdir
# Put old df variables into new df var names
df.rename(
columns={
"MTR labor income": "mtr_labinc",
"MTR capital income": "mtr_capinc",
"Total labor income": "total_labinc",
"Total capital income": "total_capinc",
"ETR": "etr",
"expanded_income": "market_income",
"Weights": "weight",
},
inplace=True,
)
test_tuple = txfunc.txfunc_est(
df, s, t, rate_type, tax_func_type, numparams, output_dir, True
)
for i, v in enumerate(expected_tuple):assert np.allclose(test_tuple[i], v)E assert False E + where False = <function allclose at 0x11225f3b0>(array([2.27262567e-23, 6.52120583e-05, 2.58990603e-13, 5.79381098e-09,\n 3.37733247e-01, 8.00000000e-01, 9.14366590e-01, 9.02760087e-06,\n 9.02760087e-06, 3.37724219e-03, 7.99990972e-03, 9.02760087e-06]), array([2.27262567e-23, 6.52115408e-05, 2.58988624e-13, 5.79701075e-09,\n 3.39354147e-01, 7.60132613e-01, 9.14365331e-01, 9.02760087e-06,\n 9.02760087e-06, 3.39345119e-03, 7.60123585e-03, 9.02760087e-06])) E + where <function allclose at 0x11225f3b0> = np.allclose
tests/test_txfunc.py:253: AssertionError ... __ test_tax_func_loop __
@pytest.mark.local
# mark as local run since results work on Mac, but differ on other
# platforms
def test_tax_func_loop():
"""
Test txfunc.tax_func_loop() function. The test is that given inputs from
previous run, the outputs are unchanged.
"""
input_tuple = decompress_pickle(
os.path.join(
CUR_PATH, "test_io_data", "tax_func_loop_inputs_large.pbz2"
)
)
(
t,
micro_data,
beg_yr,
s_min,
s_max,
age_specific,
analytical_mtrs,
desc_data,
graph_data,
graph_est,
output_dir,
numparams,
tpers,
) = input_tuple
tax_func_type = "DEP"
# Rename and create vars to suit new micro_data var names
micro_data["total_labinc"] = (
micro_data["Wage income"] + micro_data["SE income"]
)
micro_data["etr"] = (
micro_data["Total tax liability"] / micro_data["Adjusted total income"]
)
micro_data["total_capinc"] = (
micro_data["Adjusted total income"] - micro_data["total_labinc"]
)
# use weighted avg for MTR labor - abs value because
# SE income may be negative
micro_data["mtr_labinc"] = micro_data["MTR wage income"] * (
micro_data["Wage income"]
/ (micro_data["Wage income"].abs() + micro_data["SE income"].abs())
) + micro_data["MTR SE income"] * (
micro_data["SE income"].abs()
/ (micro_data["Wage income"].abs() + micro_data["SE income"].abs())
)
micro_data.rename(
columns={
"Adjusted total income": "market_income",
"MTR capital income": "mtr_capinc",
"Total tax liability": "total_tax_liab",
"Year": "year",
"Age": "age",
"expanded_income": "market_income",
"Weights": "weight",
},
inplace=True,
)
micro_data["payroll_tax_liab"] = 0
test_tuple = txfunc.tax_func_loop(
t,
micro_data,
beg_yr,
s_min,
s_max,
age_specific,
tax_func_type,
analytical_mtrs,
desc_data,
graph_data,
graph_est,
output_dir,
numparams,
)
expected_tuple = utils.safe_read_pickle(
os.path.join(CUR_PATH, "test_io_data", "tax_func_loop_outputs.pkl")
)
for i, v in enumerate(expected_tuple):
if isinstance(test_tuple[i], list):
test_tuple_obj = np.array(test_tuple[i])
exp_tuple_obj = np.array(expected_tuple[i])
print(
"For element",
i,
", diff =",
np.absolute(test_tuple_obj - exp_tuple_obj).max(),
)
else:
print(
"For element",
i,
", diff =",
np.absolute(test_tuple[i] - v).max(),
)assert np.allclose(test_tuple[i], v, atol=1e-06)E assert False E + where False = <function allclose at 0x11225f3b0>([array([2.55264562e-22, 3.51268116e-05, 3.98196664e-09, 2.22091644e-04,\n 2.94518504e-01, 1.10154861e-03, 1.00000...5296e-01, 1.00000000e+00, 1.01110118e-03,\n 1.01110118e-03, 2.86635079e-03, 1.64414195e-03, 1.01110118e-03]), ...], [array([2.55264562e-22, 3.51268127e-05, 3.88349581e-09, 1.86048713e-04,\n 2.94518501e-01, 1.10154861e-03, 1.00000...0733e-01, 1.00000000e+00, 1.01110118e-03,\n 1.01110118e-03, 2.86635057e-03, 1.67539632e-03, 1.01110118e-03]), ...], atol=1e-06) E + where <function allclose at 0x11225f3b0> = np.allclose
tests/test_txfunc.py:434: AssertionError ... ____ test_tax_func_estimate ____
tmpdir = local('/private/var/folders/d4/trj3dssd6s3g8kxvjmczz11w0000gn/T/pytest-of-richardevans/pytest-0/test_tax_func_estimate0') dask_client = <Client: 'tcp://127.0.0.1:58830' processes=2 threads=4, memory=64.00 GiB>
@pytest.mark.local
def test_tax_func_estimate(tmpdir, dask_client):
"""
Test txfunc.tax_func_loop() function. The test is that given
inputs from previous run, the outputs are unchanged.
"""
input_tuple = utils.safe_read_pickle(
os.path.join(CUR_PATH, "test_io_data", "tax_func_estimate_inputs.pkl")
)
micro_data = utils.safe_read_pickle(
os.path.join(CUR_PATH, "test_io_data", "micro_data_dict_for_tests.pkl")
)
(
BW,
S,
starting_age,
ending_age,
beg_yr,
baseline,
analytical_mtrs,
age_specific,
reform,
data,
client,
num_workers,
) = input_tuple
tax_func_type = "DEP"
age_specific = False
BW = 1
test_path = os.path.join(tmpdir, "test_out.pkl")
test_dict = txfunc.tax_func_estimate(
micro_data,
BW,
S,
starting_age,
ending_age,
start_year=2030,
baseline=baseline,
analytical_mtrs=analytical_mtrs,
tax_func_type=tax_func_type,
age_specific=age_specific,
reform=reform,
data=data,
client=dask_client,
num_workers=NUM_WORKERS,
tax_func_path=test_path,
)
expected_dict = utils.safe_read_pickle(
os.path.join(CUR_PATH, "test_io_data", "tax_func_estimate_outputs.pkl")
)
del expected_dict["tfunc_time"]
for k, v in expected_dict.items():
if isinstance(v, str): # for testing tax_func_type object
assert test_dict[k] == v
elif isinstance(expected_dict[k], list):
test_dict_obj = np.array(test_dict[k])
exp_dict_obj = np.array(expected_dict[k])
print(
"For element",
k,
", diff =",
np.absolute(test_dict_obj - exp_dict_obj).max(),
)
else: # for testing all other objects
print(
"Max diff for ", k, " = ", np.absolute(test_dict[k] - v).max()
)assert np.all(np.isclose(test_dict[k], v))E assert False E + where False = <function all at 0x112257370>(array([[ True, True, True, True, True, True, True, True, True,\n True, True, True, True, True, Tru...rue, True, True, True, True, True, True, True,\n True, True, True, True, True, True, True, True]])) E + where <function all at 0x112257370> = np.all E + and array([[ True, True, True, True, True, True, True, True, True,\n True, True, True, True, True, Tru...rue, True, True, True, True, True, True, True,\n True, True, True, True, True, True, True, True]]) = <function isclose at 0x11225f4f0>(array([[ 0. , 0. , 0. ,\n 0. , 0. , 0. ,\n... 0. ,\n 0. , 0. , 0. ,\n 0. , 0. ]]), array([[ 0. , 0. , 0. ,\n 0. , 0. , 0. ,\n... 0. ,\n 0. , 0. , 0. ,\n 0. , 0. ]])) E + where <function isclose at 0x11225f4f0> = np.isclose
tests/test_txfunc.py:722: AssertionError ... =========================== short test summary info ============================ FAILED tests/test_txfunc.py::test_txfunc_est[DEP] - assert False FAILED tests/test_txfunc.py::test_tax_func_loop - assert False FAILED tests/test_txfunc.py::test_tax_func_estimate - assert False ========= 3 failed, 513 passed, 16816 warnings in 41615.52s (11:33:35) =========
@jdebacker. The three test_txfunc.py tests that are failing on my full pytest run on my local machine are all marked to only run on local tests. So we don't need to worry about those issues for this PR. However, I think we do need to get rid of the data download in demographics.py in the CI tests that run on GitHub. I think those http errors are what are blowing up the dask workers. We should use downloaded data for the tests that run on GitHub. So we should not have any GitHub CI tests that call the get_un_data() function in demographics.py.
rickecon
commented
6 months ago @jdebacker. I think we should merge this PR with the errors. Then create an issue documenting the errors. Then I would propose opening a new PR that makes all tests that access the get_un_data() function run only locally and makes all the tests that run on the GH CI use saved UN population data.
jdebacker
commented
6 months ago @rickecon Thanks for your review!
Latest commits make your proposed changes (I think) -- I marked all tests that I think talk to the data portal as "local" and use cached data for that other tests.
I hope to have all CI tests passing before merging this PR.
jdebacker
commented
6 months ago Tests of demographics.py ok now. But still getting weird failures in TPI:
FAILED tests/test_TPI.py::test_run_TPI[Baseline] - concurrent.futures._base.CancelledError: root-3e362076-3158-4fe9-a38a-2cbd498c2091
Any ideas @rickecon ?
rickecon
commented
6 months ago @jdebacker. The error in all three cases is distributed.worker - ERROR - Could not find data: ['Specifications-5eb283f0acecd3e9c42a9de8dd1bb382'] where the long hash number at the end varies across tests. This suggests to me that the demographic data might not be loaded or located properly for the test.
rickecon
commented
6 months ago @jdebacker. The two TPI tests that are failing, I think are failing because they require the new demographic data files to already be in the repository master file folder. With this hypothesis, I am going to open and merge PR #907 that puts these files in the /tests/test_io_data/ directory. Then all the tests should pass.
rickecon
commented
6 months ago @jdebacker. By adding the demographics data files to the ./tests/test_io_data/ folder in PR #907, I was able to get the Linux Python 3.10 tests to pass. However, the Linux Python 3.11 tests had one error in test_TPI.py and one error in test_demographics.py. I currently have the Mac Python 3.10 tests running. We'll see how those go.
In the meantime, you should update the version in setup.py and ./ogcore/__init__.py to 0.11.3. And also make an update to CHANGELOG.md both at the top of the file as well as the markdown links at the bottom of the file.
jdebacker
commented
6 months ago That's all odd - given I had added those data in this PR, I think the tests should be pulling the cached demographic data from this branch just fine for the test. I did see a failure with an HTTP 429 error as I forgot to mark a test that does ping the UN. Just fixed that.
The error in test_TPI.py seem like there might be something technical with how the local cluster is working for some of the GH Action instances:
async def _gather(self, futures, errors="raise", direct=None, local_worker=None):
unpacked, future_set = unpack_remotedata(futures, byte_keys=True)
mismatched_futures = [f for f in future_set if f.client is not self]
if mismatched_futures:
raise ValueError(
"Cannot gather Futures created by another client. "
f"These are the {len(mismatched_futures)} (out of {len(futures)}) "
f"mismatched Futures and their client IDs (this client is {self.id}): "
f"{ {f: f.client.id for f in mismatched_futures} }" # noqa: E201, E202
)
keys = [future.key for future in future_set]
bad_data = dict()
data = {}@jdebacker. I am still re-running tests one-by-one, although both Mac OS Python 3.10 and Python 3.11 have now failed. I don't know what the issue is, and I can't find any related GitHub issues, Dask issues, or StackOverflow threads that are related to this (e.g., "GitHub Action Dask concurrent.futures._base.CancelledError").
This problem seems to have arisen with this most recent pull request. And I don't think it is related to the content of this PR, although iI guess it could still be some rogue calls to the UN API. I recommend we try the following changes.
.py files. For example, change line 16 in test_SS.py to be the following. And make that change in all the .py files in the ./tests/ directory
NUM_WORKERS = min(multiprocessing.cpu_count(), 4)
SeaCelo
commented
6 months ago @rickecon @jdebacker We are in touch with the API developers to see what is causing the errors. They are asking for some code that replicates it. Can you share something that they can run in python to test?
cc: @sezdi0810
rickecon
commented
6 months ago @jdebacker. We know this passes tests locally. We can make an issue about these tests not passing. Merging.
This PR adds sleep commands to avoid HTTP 429 error from accessing the UN data portal for demographics data.