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

Where did OG-USA related metadata from taxcalc.dropq go? #1368

Closed
PeterDSteinberg opened this issue May 16, 2017 · 17 comments
Closed

Where did OG-USA related metadata from taxcalc.dropq go? #1368

PeterDSteinberg opened this issue May 16, 2017 · 17 comments

Comments

@PeterDSteinberg
Copy link
Contributor

@martinholmer I found this diff in the github blame report for taxcalc.dropq:

fc06a73#diff-0fbb9d8bd5e87c63c64db8be5816d69bL38

It shows that OGUSA_ROW_NAMES has been deleted, among other OG-USA variables.

When we make changes to Tax-Calculator, we need to test locally with all the tentative release candidates of:

  • webapp-public - running locally
  • deploy - (will be moved to webapp-public soon)
  • B-Tax
  • Tax-Calculator
  • OG-USA

We have had problems recently due to one of those repos being changed without being tested against the versions of the other repos that will be released at the same time.

@MattHJensen
Copy link
Contributor

@martinholmer warned @PeterDSteinberg, @rickecon, and @jdebacker of this issue 15 days ago in this comment #1314 (comment) before the PR was merged.

@MattHJensen
Copy link
Contributor

MattHJensen commented May 16, 2017

@PeterDSteinberg said:

When we make changes to Tax-Calculator, we need to test locally with all the tentative release candidates of:

webapp-public - running locally
deploy - (will be moved to webapp-public soon)
B-Tax
Tax-Calculator
OG-USA

Webapp-public, deploy, b-tax, and og-usa developers should not depend on taxcalc devs to check compatibility before releases just like taxcalc devs don't expect Pandas devs to test new pandas versions against taxcalc.

We have had problems recently due to one of those repos being changed without being tested against the versions of the other repos that will be released at the same time.

[emphasis added]

This seems like the problem. Why do we need to release versions of all repos at the same time?

Why isn't it like this:

  1. Taxcalc releases 0.8.4
  2. b-tax, og-usa, and webapp-public test against taxcalc 0.8.4
  3. b-tax, og-usa, and webapp-public fix any incompatibilities
  4. b-tax, og-usa, and webapp-public adopt 0.8.4 when they are ready

@PeterDSteinberg
Copy link
Contributor Author

Your sequenced steps above (1 to 4) look good @MattHJensen but running webapp-public locally with all the repos on the right branches finds the bugs with 1/4 of that personnel hours investment. I'm sorry I missed @martinholmer 's important comment, but I don't know if it makes sense to merge a Tax-Calculator PR to master when the cooperating repos have not started work on corresponding changes to be compatible. I'll make the changes @martinholmer suggested after my meeting now.

@MattHJensen
Copy link
Contributor

but running webapp-public locally with all the repos on the right branches finds the bugs with 1/4 of that personnel hours investment

@PeterDSteinberg, what about setting pins in og-usa/b-tax/webapp-public to allow this workflow:

  1. Taxcalc releases 0.8.5
  2. Run webapp-public with taxcalc 0.8.5 and pre-existing versions of b-tax, og-usa, webapp-public.
  3. If there is a problem with b-tax, og-usa, or webapp-public:
    • Update that project
    • change the taxcalc pin for that project
    • release a new version of that project
  4. Try webapp-public again with taxcalc 0.8.5 and the updated version of the project that was having problems from step 3.

@MattHJensen
Copy link
Contributor

I think a fundamental problem here is thinking about taxcalc, webapp-public, b-tax, and og-usa as part of the same project with the same development team. In fact, they are separate projects with separate development teams.

@MattHJensen
Copy link
Contributor

Something else that will make this easier for everyone is if taxcalc declares a public API and adopts semantic versioning to identify which releases make incompatible API changes, which releases add backwards-compatible functionality, and which releases make backwards-compatible bug fixes. More on that later.

@PeterDSteinberg
Copy link
Contributor Author

I agree with your numbered steps above, @MattHJensen , regarding tentative release of taxcalc, then running webapp-public to see if other repos need changes, adjusting version pinning as necessary, and re-testing with webapp-public.

@MattHJensen
Copy link
Contributor

@PeterDSteinberg, one nitpick: it would be a "real" release of taxcalc not a "tentative" release, right? Because taxcalc users (other than webapp-public/b-tax/og-usa users), could always upgrade to it.

@PeterDSteinberg
Copy link
Contributor Author

It is possible to do it that way @MattHJensen (making taxcalc releases be easily installed from main label), however we essentially did that over the last week or so with the latest release and pandas version update. The result then was that conda install -c ospc taxcalc btax ogusa did not work as expected due inconsistency in pandas pinning among the 3 repos. It should be fine to release to the main channel, but still probably advisable to build taxcalc in the dev label and try it out informally first before switching to the main channel (it could just be an hour of informally testing on the dev label before the switch).

@brittainhard
Copy link
Contributor

@PeterDSteinberg @MattHJensen , a while ago I made a script that can check between versions and look for API changes. We can probably make use of it here. Here's the change from 0.8.3 to 0.8.4

DELETED: taxcalc.dropq.dropq_utils.create_dropq_distribution_table
DELETED: taxcalc.dropq.dropq_utils.create_json_blob
DELETED: taxcalc.dropq.dropq_utils.create_dropq_difference_table
DELETED: taxcalc.dropq.dropq_utils.format_print
DELETED: taxcalc.dropq.dropq_utils.create_json_table
DELETED: taxcalc.dropq.dropq.run_model
DELETED: taxcalc.dropq.dropq.random_seed
DELETED: taxcalc.dropq.dropq.groupby_means_and_comparisons
DELETED: taxcalc.dropq.dropq.run_nth_year_model
DELETED: taxcalc.dropq.dropq.format_macro_results
DELETED: taxcalc.dropq.dropq.drop_records
DELETED: taxcalc.dropq.dropq.results
DELETED: taxcalc.dropq.dropq.random_seed_from_subdict
DELETED: taxcalc.dropq.dropq.run_gdp_elast_model
DELETED: taxcalc.dropq.dropq.check_user_mods
DELETED: taxcalc.dropq.dropq.calculate_baseline_and_reform
DELETED: taxcalc.dropq.dropq.chooser
ADDED: taxcalc.dropq.dropq_utils.random_seed
ADDED: taxcalc.dropq.dropq_utils.dropq_calculate
ADDED: taxcalc.dropq.dropq_utils.dropq_summary
ADDED: taxcalc.dropq.dropq_utils.check_user_mods
ADDED: taxcalc.dropq.dropq_utils.drop_records
ADDED: taxcalc.dropq.dropq_utils.results
ADDED: taxcalc.dropq.dropq_utils.random_seed_from_subdict
ADDED: taxcalc.dropq.dropq_utils.dropq_dist_table
ADDED: taxcalc.dropq.dropq_utils.dropq_diff_table
ADDED: taxcalc.dropq.dropq_utils.chooser
ADDED: taxcalc.dropq.dropq.run_nth_year_tax_calc_model
ADDED: taxcalc.dropq.dropq.create_json_table

@brittainhard
Copy link
Contributor

@PeterDSteinberg i put a commit in for webapp that just adds the content of that list into webapp itself. I'm gonna push it up if that's okay with you.

@PeterDSteinberg
Copy link
Contributor Author

Sounds good, @brittainhard . Is this a local commit for you on webapp and push to heroku? Is it pushed elsewhere yet? I was also just running webapp locally and looking for changes needed on imports. I'll add anything I find to your PR as needed.

@brittainhard
Copy link
Contributor

@PeterDSteinberg this is being added to the form_additions PR. That's effectively the master branch right now.

@MattHJensen
Copy link
Contributor

MattHJensen commented May 16, 2017

I said:

what about setting pins in og-usa/b-tax/webapp-public to allow this workflow:

  1. Taxcalc releases 0.8.5
  2. Run webapp-public with taxcalc 0.8.5 and pre-existing versions of b-tax, og-usa, webapp-public.
  3. If there is a problem with b-tax, og-usa, or webapp-public:
    • Update that project
    • change the taxcalc pin for that project
    • release a new version of that project
  4. Try webapp-public again with taxcalc 0.8.5 and the updated version of the project that was having problems from step 3.

and then @PeterDSteinberg said:

It is possible to do it that way @MattHJensen (making taxcalc releases be easily installed from main label), however we essentially did that over the last week or so with the latest release and pandas version update. The result then was that conda install -c ospc taxcalc btax ogusa did not work as expected due inconsistency in pandas pinning among the 3 repos. It should be fine to release to the main channel, but still probably advisable to build taxcalc in the dev label and try it out informally first before switching to the main channel (it could just be an hour of informally testing on the dev label before the switch).

@PeterDSteinberg, if we are pinning webapp-public to specific versions of taxcalc, btax, and ogusa, and then following the above workflow with new taxcalc releases, then do we really need to be able to safely run conda install -c ospc taxcalc btax ogusa every time a new taxcalc is released? I am thinking that we can pin things in such a way that it relaxes the requirement that the latest versions of taxcalc, btax, and ogusa always need to work in the same environment, and then the separate development teams can bring all of the packages forward one by one rather than needing to do them in lockstep.

I certainly agree that taxcalc devs should build taxcalc in the dev channel and test it by itself before moving to main, but I really don't think it makes sense for taxcalc devs to test it against downstream repositories. I hope we get to the point where there are many more downstream repositories than just btax and ogusa, so we need to adopt a workflow that is scalable and will accomodate that. Similarly, Pandas does not test it's new packages against taxcalc before releasing them.

@PeterDSteinberg
Copy link
Contributor Author

The only advantage of conda install -c ospc taxcalc btax ogusa is its simplicity for new users approaching the project. As of about a month ago, the deploy repo was set up to deploy any branch of git or any conda version of each repo. The README for getting started with the project is oriented around that simple command, but it can be changed. I'll make an issue to update the docs with a variety of install options and mention how packages are built and pinned there.

I see your point regarding testing downstream repos and the difficulty of tying taxcalc devs to removing bugs related to interactions with an expanding number of add-on models. Hopefully the changes in webapp PR 553 will make it easier to run webapp-public locally and/or get test feedback on the API-related problems and I think we'll find these problems more quickly.

@jdebacker
Copy link
Member

From my perspective as someone working on models that depend on Tax-Calc, I agree that having Tax-Calc developers test compatibility with downstream models is too onerous. And I don't think there is much value in having a single conda install for all OSPC packages since, as noted, the development teams/users of the models often different. Rather we can have each package (OG-USA, B-Tax) that depends on Tax-Calc pinned to the latest TC version it's compatible with.

But I do think the deploy repo/webapp-public should call only versions that are compatible with one another given their interactions. Thus there should be some testing for compatibility before the version of the models those repos are pinned to are updated.

@martinholmer
Copy link
Collaborator

Tax-Calculator issue #1368 has been open for almost a month even though the question posed by @PeterDSteinberg was answered immediately by @MattHJensen. Under the assumption that the OG-USA developers know what code needs to be added to OG-USA (see pull request #1314 for what was removed from Tax-Calculator and needs to be added to OG-USA), issue #1368 is being closed.

The subsequent discussion in issue #1368 about how best to coordinate development activity across the several OSPC-sponsored projects has culminated in webapp-public issue #560. Any further discussion of those coordination issues should take place in that issue.

@MattHJensen @PeterDSteinberg @rickecon @jdebacker @kerkphil

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

No branches or pull requests

5 participants