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

Updated calitp-data-analysis #2944

Merged
merged 15 commits into from Sep 25, 2023
Merged

Updated calitp-data-analysis #2944

merged 15 commits into from Sep 25, 2023

Conversation

amandaha8
Copy link
Contributor

@amandaha8 amandaha8 commented Sep 18, 2023

Description

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

  • We are moving some of the shared_utils scripts to data-infra so we can use them across different repos.

Resolves #870

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.

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)
  • Open a PR and make sure the test publication works as expected, with the new version number displayed
  • Merge the PR and make sure the production publication works as expected, with the new version number displayed
  • Open and merge a second PR that references the new version number here, here, and anywhere else you find a reference to the old version of the package in the data-infra repository (you'll also want to do the same for any other Cal-ITP repositories that reference the calitp-data-analysis package).

@tiffanychu90
Copy link
Member

Notes from today's session:
poetry manages dependencies, maintains pyproject.toml and poetry.lock files.
They are like requirements.txt where they record the versions of the packages that this project depends on.

within data-infra/packages/calitp_data_analysis.
In terminal: poetry add dask poetry add altair
git status will show changes in pyproject.toml and poetry.lock.
commit and push. if you do it to all the packages in the error msgs, the GH action will succeed because it can find the packages.

anyone who syncs repo after, if they run poetry update, they will get the changes.

pyproject are things that you say you need.
even if geopandas refactored and took away shapely, we would still add shapely because we wanted to use it. for now, yes, geopandas does install shapely as dependency.

poetry run mypy
poetry is fancy version of virtualenv
poetry install to make sure you have stuff installed, otherwise poetry run mypy --install-types.
It does an audit to check your types to make sure your type hints are working as intended.

We use juptyer image, so try those versions for packages, because we know those are the versions that actually build successfully.

use poetry add most of the time.

update the version date in data-infra/packages, and when you change it in the jupyter image, it's telling jupyter to get that updated version. until you actually push the changes, the date doesn't matter. it will get mad if the version you're pointing at doesn't exist yet. do 1 PR where you add all the packages and update the version, then a follow-up PR and point at the new updated version. jupyter is pulling from pypi when it builds our image.

Copy link
Contributor

@lauriemerrell lauriemerrell left a comment

Choose a reason for hiding this comment

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

I just am confirming that the updates look reasonable from a structural perspective (dependency specification looks ok, and I see the test publish succeeded). Defer to @tiffanychu90 for substantive review on the actual shared_utils content; would not want to merge without her review.

@amandaha8 amandaha8 merged commit aa77bc0 into main Sep 25, 2023
2 checks passed
@amandaha8 amandaha8 deleted the add_shared_utils branch September 25, 2023 15:07
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.

Feature Request: move portions of shared_utils into data-infra repo
3 participants