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

Outputs revamp #921

Merged
merged 73 commits into from
Aug 22, 2018
Merged

Outputs revamp #921

merged 73 commits into from
Aug 22, 2018

Conversation

hdoupe
Copy link
Collaborator

@hdoupe hdoupe commented Jul 18, 2018

Implements the prototype in #916. A Tax-Calculator PR will be opened and worked on in tandem with this PR in order to prepare Tax-Calculator for the new outputs standards.

Goals:

  • create simple HTML page to render the tables created by Tax-Calculator
  • simplify compute.py logic corresponding to @lucassz's work in Initial commit pb_deploy#1 *
  • move new outputs logic and worker-node facing code to a new app core **
  • implement simpler database schema
  • keep backwards compatibility (can still view old results)

* Once this is merged, there will only be one AWS endpoint per worker node cluster to query instead of one for each worker node. Thus, there is no need to implement our own round robin logic--this is done by celery. When a job is posted, it is placed on celery's task queue (which is run via redis) and celery pulls those jobs in a LIFO manner. I think we should implement this simplifying compute.py logic and the ability to display the new tables simultaneously because these sections of the code base are tightly coupled. This means that changing one requires significant changes to the other. So, changing them at the same time will be a lot of work but not much more work than changing them sequentially. I'm happy to discuss further if others have differing opinions.
** To accomplish the goal of turning PolicyBrain into a platform (#906), more code will need to be shared between apps. Moving code that can be used by multiple apps into this new core app is a step in that direction. Also, see #816.

@hdoupe
Copy link
Collaborator Author

hdoupe commented Jul 18, 2018

@lucassz I left core/models.py for you to fill out. One way to do this is to open a PR on my branch outupts. An alternative is to create an ouptuts-dev branch that we can work off until we are ready to push the new outputs into production. I'm partial to the latter approach as this leaves the master branch in a production ready state.

def normalize(x):
ans = [i.split('#') for i in x]
return ans

Copy link
Collaborator Author

@hdoupe hdoupe Jul 23, 2018

Choose a reason for hiding this comment

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

Do we still need these functions (normalize and denormalize)?

@hdoupe
Copy link
Collaborator Author

hdoupe commented Jul 23, 2018

@lucassz do we still need the taxbrain/compute.py? It looks like Compute is now in the core app. Next, we could move all of the mock compute classes to test_assets or in the taxbrain tests directory.

url = OutputUrl.objects.get(pk=pk)
except OutputUrl.DoesNotExist:
raise Http404

Copy link
Collaborator Author

@hdoupe hdoupe Jul 23, 2018

Choose a reason for hiding this comment

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

@lucassz what do you think about doing the URL routing through the new CoreRun table using the uuid variable?

return render(
request,
'taxbrain/not_avail.html',
not_avail_context)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This logic is aimed at data with a backwards incompatible format. Ideally, we won't need to accommodate backwards incompatible data. I think it's safe to remove it. Hopefully, we won't need to add it back.

error.text = error_contents
error.save()
model.error_text = error
model.save()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@lucassz what do you think about moving the error messages into the CoreRun table?

@lucassz
Copy link
Contributor

lucassz commented Jul 23, 2018

@hdoupe I only temporarily kept normalize and taxbrain/compute.py for the purpose of not breaking things in B-Tax for now. Otherwise, do you know of an easy way to deactivate the B-Tax app, so we can get rid of those things and still have the webserver launch?

I will also remove the not_avail_context part.

I agree with moving the URL routing and error storage into CoreRun, I will do that next (along with the outputs).

@hdoupe
Copy link
Collaborator Author

hdoupe commented Jul 23, 2018

@hdoupe I only temporarily kept normalize and taxbrain/compute.py for the purpose of not breaking things in B-Tax for now. Otherwise, do you know of an easy way to deactivate the B-Tax app, so we can get rid of those things and still have the webserver launch?

Ok, I see that makes sense. Nope, I'm not sure how to do that.

I will also remove the not_avail_context part.

I agree with moving the URL routing and error storage into CoreRun, I will do that next (along with the outputs).

Great, thanks.

@hdoupe
Copy link
Collaborator Author

hdoupe commented Aug 22, 2018

All tests are passing in PR #921. @lucassz thank you for your hard work in developing and implementing the new approach to displaying outputs. @martinholmer thank you for your feedback as this feature was developed. The changes made in this PR open up many possibilities for how outputs can be rendered in the future. Further, many patterns developed here such as using a core app and Class-Based-Views will be very beneficial for development in the future.

All of the TaxBrain apps (static, behavioral, and macro-elasticity) are now running with the new outputs implementation. Once it has been polished up, there should not be any noticeable difference in how the outputs look. This fall, I plan to transition the Cost-of-Capital calculator to this new approach.

A lot of work is still required to clean up after this major change. There needs to be some of editorial work to indicate that the behavioral app parameters are now on the main TaxBrain page and that the macro elasticity app is available if no behavioral parameters are specified on the main taxbrain page. A comprehensive solution will be applied to this problem once the outputs work started in this PR is complete. I don't anticipate finishing up this work taking more than a week or two.

@hdoupe hdoupe merged commit a09a2b3 into ospc-org:master Aug 22, 2018
@hdoupe hdoupe changed the title Outputs Outputs revamp Aug 23, 2018
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

3 participants