-
Notifications
You must be signed in to change notification settings - Fork 32
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
Reinstate ogusa #710
base: master
Are you sure you want to change the base?
Reinstate ogusa #710
Conversation
Below is the traceback for the error that I am getting now. The problem is that Tax-Calculator is looking for a @jdebacker @rickecon @PeterDSteinberg
|
This looks good from my end. @hdoupe @jdebacker @MattHJensen |
@brittainhard Most of the work here is done. One problem is that Tax-Calculator is looking for the puf file at
I'm not sure if this is a numpy version problem or something else. |
@hdoupe I know in the case of CCC it wants the extracted puf.csv. This could also be the case with OGUSA |
@brittainhard this is the case for OG-USA, too. Where does B-Tax get the extracted puf.csv file? |
@@ -179,14 +179,20 @@ def example(): | |||
|
|||
@app.route('/ogusa_start_job', methods=['POST']) | |||
def ogusa_start_job(): | |||
start_year int(request.form["start_year"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got a syntax error here @hdoupe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @brittainhard. I'll have a fix up shortly.
@brittainhard there are a couple other issues. Hang on |
Should be good to go now. |
@hdoupe i'm seeing the error you posted earlier, |
@brittainhard No, I did not. The up-to-date (including OG-USA PRs merged after 0.5.9) OG-USA version I worked with in PSLmodels/OG-Core#316 worked fine. I need to dig in deeper to see why I was able to run the regression reforms, but now it doesn't work. My hunches are:
|
In the regression tests, I ran the reforms with This could be causing a problem. @jdebacker what do you think? I'm also going to try to run the |
@hdoupe I do not this that is the issue. My guess is the error is a result of some issue with a version of the packages. OG-USA needs the environment set correctly to run without error. Is the environment set exactly as in the OG-USA |
Ok, no I think it is running in the
and got:
You can see that pandas is |
@hdoupe I'm not 100% sure, but I think the int/float issue is due to the difference in versions from NumPy. If you can pin |
I'm running the same
If this runs OK, then we will know it's the environment. [UPDATE] The model appears to be running OK. Here's the result of the conda export for
|
@jdebacker I'm not sure if we want to go back to numpy 0.11.2. @martinholmer how would moving back to numpy 0.11.1 affect Tax-Calculator? |
@jdebacker said:
Yes, yet another example of the evils of exact pinning (see this discussion for a general discussion of why exact pinning should be avoided). @hdoupe asked:
Tax-Calculator uses at-least pinning for numpy and pandas:
And the current version of those two packages are: The solution, as @jdebacker realizes, is to fix the code in OG-USA so that it runs using modern versions of numpy and pandas. Tax-Calculator developers have had to do this in the past as described in item 2 in this discussion. Doing this made the code less buggy and Tax-Calculator ran much faster because the the efficiency improvements in the newer versions of numpy and pandas. (Remember exact pinning to an old version of numpy will logically imply the use of an older version of pandas, which is likely to be much slower than modern versions.) I suppose you could run OG-USA on its own AWS instance, which would allow it to have a different environment than used by Tax-Calculator, but that approach is likely to take more time than simply fixing the OG-USA bugs that arise when using modern versions of numpy and pandas. |
@hdoupe You probably don't. I'm awaiting my thrashing from @martinholmer. I might be able to try to figure out some packaging stuff in the next 24 hours. Can you identify other conflicts between |
@martinholmer Thanks for your input and I agree. @jdebacker after comparing the
I'll add some tolerance checking on the regression testing PR (PSLmodels/OG-Core#316) that I opened up ages ago. That way once we fix the version problems we can make sure that we didn't break anything in the process. |
@hdoupe said:
|
@martinholmer We could stick with >=0.18.1 version but the 1.0.0 version was released two days ago. I'm not sure why it's not available on conda yet. Maybe we should just stick with >=0.18.1. @jdebacker I don't think Tax-Calculator or B-Tax uses scipy. So whichever version you can get to work with the proper versions of Numpy will work. |
Ok, thanks for the input @hdoupe and @martinholmer. Other than the integer/float issue, I think some changes to the SciPy's optimization routines are relevant to the model and so it's good to know what the other PolicyBrain dependencies are that and related packages. |
With the merge of PSLmodels/OG-Core#322, the version issue should be resolved. I'm still not sure how to handle the issue where OG-USA expects there to be a |
Either have both puff.csv and puf.csv.gz available or bring OG-USA into line with taxcalc.
…Sent from my iPhone
On Oct 27, 2017, at 11:46 AM, Henry Doupe ***@***.***> wrote:
With the merge of PSLmodels/OG-Core#322, the version issue should be resolved.
I'm still not sure how to handle the issue where OG-USA expects there to be a puf.csv file but we instead have a puf.csv.gz file. How does CCC handle this so that it does not throw an error?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
I just checked and the worker nodes have both The next version of OG-USA is pinned to |
This PR reinstates OG-USA on TaxBrain. In doing so, the input processing logic for the
dynamic/views.py
functiondyanamic_input
was changed so that it used the input processing logic introduced in PR #641. Further, the input processing that was done inDynamicCompute.submit_ogusa_calculation
is now done indynamic_input
.