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

Bring in calitp-data-analysis and remove calitp-data #2838

Merged
merged 12 commits into from
Jul 31, 2023

Conversation

atvaccaro
Copy link
Contributor

@atvaccaro atvaccaro commented Jul 24, 2023

Description

Describe your changes and why you're making them. Please include the context, motivation, and relevant dependencies.

Should merge #2837 first and rebase this one.

Brings calitp-data-analysis here and completely removes dependencies on calitp-data. Migrates any references/updates all images.

Every time I do this, it makes me think we should combine some images. :)

TODO: could be a follow-up PR, but still need to set up publishing to pypi for the 3 packages (map utils, data infra, data analysis)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

How has this been tested?

Include commands/logs/screenshots as relevant.

CI is running mypy and tests. I've testing the archiver, Airflow, and pod operators.

TODO: test jupyter

Post-merge follow-ups

Document any actions that must be taken post-merge to deploy or otherwise implement the changes in this PR (for example, running a full refresh of some incremental model in dbt). If these actions will take more than a few hours after the merge or if they will be completed by someone other than the PR author, please create a dedicated follow-up issue and link it here to track resolution.

  • No action required
  • Actions required (specified below)
    Deploy new Jupyter pipeline images. Run stuff in production. Mark calitp-py repo as deprecated.

@atvaccaro atvaccaro self-assigned this Jul 24, 2023
@atvaccaro atvaccaro changed the title Bring in calitp data analysis Bring in calitp-data-analysis and remove calitp-data Jul 25, 2023
@github-actions
Copy link

@atvaccaro atvaccaro marked this pull request as ready for review July 25, 2023 23:49
@atvaccaro atvaccaro force-pushed the bring-in-calitp-data-analysis branch from 0d4caa2 to 629c445 Compare July 26, 2023 15:02
@atvaccaro atvaccaro force-pushed the delete-legacy-payments-code branch from 6195c8d to 65d492c Compare July 26, 2023 15:35
@atvaccaro atvaccaro force-pushed the bring-in-calitp-data-analysis branch from 629c445 to 23648fb Compare July 26, 2023 15:35
@atvaccaro atvaccaro added the do-not-merge Do not merge, even if approved label Jul 26, 2023
@atvaccaro atvaccaro force-pushed the delete-legacy-payments-code branch from 05efcc3 to ab21330 Compare July 27, 2023 20:16
Base automatically changed from delete-legacy-payments-code to main July 27, 2023 20:19
@atvaccaro atvaccaro force-pushed the bring-in-calitp-data-analysis branch from 23648fb to c8d2126 Compare July 28, 2023 02:09
@atvaccaro atvaccaro removed the do-not-merge Do not merge, even if approved label Jul 28, 2023
Copy link
Contributor

@SorenSpicknall SorenSpicknall left a comment

Choose a reason for hiding this comment

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

Approving, with one question: what's the separation rationale behind removing the get_fs call from __init__.py in calitp_map_utils? Just trying to reduce dependencies that stretch across different parts of the ecosystem?

@atvaccaro
Copy link
Contributor Author

atvaccaro commented Jul 28, 2023

Approving, with one question: what's the separation rationale behind removing the get_fs call from __init__.py in calitp_map_utils? Just trying to reduce dependencies that stretch across different parts of the ecosystem?

Right exactly, the original calitp-py became a dumping ground for shared code as I split up/created jobs. Now I'm trying to rectify that by limiting the chain of dependencies, even if that means copy-pasting (relatively small) pieces of code that don't really need to be a packaged function anyways.

@atvaccaro atvaccaro force-pushed the bring-in-calitp-data-analysis branch from 4654972 to cc28c80 Compare July 31, 2023 15:19
@atvaccaro atvaccaro merged commit 6e2e8a8 into main Jul 31, 2023
16 checks passed
@atvaccaro atvaccaro deleted the bring-in-calitp-data-analysis branch July 31, 2023 15:29
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.

2 participants