Define a minimal Python version of the package containing `vars_rename` #31

jeancochrane · 2024-11-20T22:39:22Z

This PR defines a minimal Python ccao package in this repo with one function ported over from R, vars_rename. The package lives in the python/ subdir and uses the following tools:

uv for packaging
ruff for linting/formatting
pytest for unit tests
sphinx for docs

We add a new pytest-coverage GitHub workflow to ensure that tests run on PRs and commits to the main branch. We also rename the pkgdown workflow to docs and update it so that it deploys Python docs alongside R docs in the relative path /ccao/python/.

Closes #29.

⚠️ Note that for some reason, CI thinks that the build-pkgdown-site check needs to complete before merging, so it's not reporting the CI checks as being successful. I don't understand why this is happening, given that the build-pkgdown-site step no longer exists. Maybe there's a repo-level setting that is requiring this stage? If not, I figure we can just force merge this when it's ready and hope it's a caching bug that resolves itself.

… out

… test it out" This reverts commit 9cc256b.

codecov · 2024-11-21T23:18:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (969dae7) to head (bbbaf68).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master       #31   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines          382       382           
=========================================
  Hits           382       382

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

.github/workflows/R-CMD-check.yaml

.github/workflows/docs.yaml

jeancochrane · 2024-11-21T23:05:15Z

.github/workflows/docs.yaml

        run: pkgdown::build_site_github_pages(new_process = FALSE, install = FALSE)
        shell: Rscript {0}

+      - name: Build Python docs
+        run: sphinx-build python/docs/source docs/python


Building these docs into the docs/python subdirectory makes them show up under the relative path /python in the deployed docs site.

suggestion (non-blocking): IMO it would be nice to have the R docs at /r for consistency, with a redirect/301 at the root to maintain old links.

I don't actually think redirects are possible on GitHub Pages, at least if this feature request is still up to date. We could implement redirects if we switched to a different static hosting provider like Netlify, but that's a bigger discussion. Let me know if you can think of any other way to accomplish something similar and I'm happy to take a crack at it.

You might be able to do client-side redirects with http-equiv.

.github/workflows/pytest-coverage.yaml

python/.gitignore

python/docs/source/conf.py

python/docs/source/index.rst

python/pyproject.toml

jeancochrane · 2024-11-21T23:26:18Z

python/tests/fixtures/chars_sample_athena.csv

@@ -0,0 +1,71 @@
+"pin","year","class","char_yrblt","char_bldg_sf","char_land_sf","char_beds","char_rooms","char_fbath","char_hbath","char_frpl","char_type_resd","char_cnst_qlty","char_apts","char_tp_dsgn","char_attic_fnsh","char_gar1_att","char_gar1_area","char_gar1_size","char_gar1_cnst","char_attic_type","char_bsmt","char_ext_wall","char_heat","char_repair_cnd","char_bsmt_fin","char_roof_cnst","char_use","char_age","char_site","char_ncu","char_renovation","char_porch","char_air","char_tp_plan"


I wish we could just share these fixtures with the R package, but the R package needs the fixtures to be saved as RData objects, and I feel like it would be kind of weird to bundle that in the Python package. Down to give it a try if you have a strong preference, though.

suggestion: IMO it's worth it to create a shared directory for these fixtures that the R scripts use to instantiate the .rda files and the Python package uses for fixtures. The dir would contain this data, saved as CSV and created via SQL (from the existing R data-raw scripts).

Nice idea! Done in e66eff3. Until we refactor our data management in #30, it seemed like the simplest path forward was just to keep data-raw/ as the shared source of CSV data for now, but let me know if you have another preference.

python/tests/test_vars_funs.py

dfsnow

@jeancochrane This is looking great! Nice first pass, no major blockers. Looking forward to the remaining package.

dfsnow · 2024-11-23T18:14:02Z

python/README.md

+You can install the released version of `ccao` directly from GitHub:
+
+```bash
+pip install "git+https://github.com/ccao-data/ccao.git#egg=ccao&subdirectory=python"


There doesn't seem to be a ccao package on PyPI. Why not push it there?

dfsnow · 2024-11-23T18:15:40Z

python/pyproject.toml

+    {name = "Dan Snow", email="[email protected]"},
+    {name = "Jean Cochrane", email="[email protected]"},


suggestion (non-blocking): I didn't make any of this package! At least put yourself first.

Kind of you! Done in cd5bc3e.

dfsnow · 2024-11-23T18:16:27Z

python/pyproject.toml

+]
+
+[project.optional-dependencies]
+test = [


suggestion (non-blocking): I've taken to naming this dev instead of test since ruff and mypy aren't specific to testing. I don't feel super strongly about it though.

Makes sense, done in aaebf7c!

dfsnow · 2024-11-23T18:50:01Z

python/.python-version

question (non-blocking): Why have a .python-version file if it's not used in the GH workflows? Just to set the default version for uv?

Good point, removed in 1b6bbef.

dfsnow · 2024-11-23T18:51:52Z

.github/workflows/docs.yaml

        run: pkgdown::build_site_github_pages(new_process = FALSE, install = FALSE)
        shell: Rscript {0}

+      - name: Build Python docs
+        run: sphinx-build python/docs/source docs/python


suggestion (non-blocking): IMO it would be nice to have the R docs at /r for consistency, with a redirect/301 at the root to maintain old links.

dfsnow · 2024-11-23T18:55:21Z

python/ccao/vars_funs.py

+vars_dict = pd.read_csv(str(_data_path / "vars_dict.csv"))
+
+# Prefix we use to identify variable name columns in the variable dictionary
+var_name_prefix = "var_name"


nitpick: Shouldn't this be an all caps constant?

True, done in c8f4e49.

dfsnow · 2024-11-23T18:58:41Z

python/ccao/vars_funs.py

+    data: list[str] | pd.DataFrame,
+    names_from: str,
+    names_to: str,
+    output_type: OutputType | str = OutputType.INPLACE,


question: I'm kind of a dumdum when it comes to this kind of stuff, but what's the advantage of enumerating the possible type values like this? Why not just check them in the function body?

My thinking was that a DRYer approach would reduce maintenance burden in case we have to change the options someday or define logic that might best be owned by the param abstraction. But I think it's a bit overengineered for the current use-case so I stripped it out in 81d8c20 in favor of strings that we check in the body of the function.

dfsnow · 2024-11-23T19:00:38Z

python/ccao/vars_funs.py

+    # Validate names arguments
+    if not isinstance(names_from, str) or not isinstance(names_to, str):
+        raise ValueError("names_from and names_to must be strings")


question: (non-blocking): Since the function is typed, isn't this check redundant? Same goes for the other isinstance checks.

I was thinking that it might be helpful to double-check types in case users don't have type-checking enabled in their project, but since this package is pretty specific to us and we always enable type checking, I doubt it's worth the extra code. I removed the duplicative type checking in b837865.

dfsnow · 2024-11-23T19:10:41Z

python/tests/fixtures/chars_sample_athena.csv

@@ -0,0 +1,71 @@
+"pin","year","class","char_yrblt","char_bldg_sf","char_land_sf","char_beds","char_rooms","char_fbath","char_hbath","char_frpl","char_type_resd","char_cnst_qlty","char_apts","char_tp_dsgn","char_attic_fnsh","char_gar1_att","char_gar1_area","char_gar1_size","char_gar1_cnst","char_attic_type","char_bsmt","char_ext_wall","char_heat","char_repair_cnd","char_bsmt_fin","char_roof_cnst","char_use","char_age","char_site","char_ncu","char_renovation","char_porch","char_air","char_tp_plan"


suggestion: IMO it's worth it to create a shared directory for these fixtures that the R scripts use to instantiate the .rda files and the Python package uses for fixtures. The dir would contain this data, saved as CSV and created via SQL (from the existing R data-raw scripts).

dfsnow · 2024-11-23T19:16:13Z

python/uv.lock

question (non-blocking): Why include this lock file if the the dependencies are already captured in pyproject.toml?

Good point, removed in 5960235.

jeancochrane · 2024-11-25T20:05:12Z

I made some major changes based on your comments @dfsnow, so it would be good to get a second look before I merge!

dfsnow

This looks great @jeancochrane. Nice fixups!

dfsnow · 2024-11-25T21:12:44Z

python/tests/test_vars_funs.py

-    if output_type == "inplace":
-        assert list(result.columns) == expected
-    else:
+class TestVarsRename:


🤦 How did I not notice this wasn't in a test class. Nice catch.

jeancochrane added 5 commits November 20, 2024 13:39

Add basic Python project with just vars_rename

0231af4

Add unit tests for vars_rename

a738b8b

Add pytest-coverage workflow

adb87a6

Add Development docs to python/README.md

c0739e8

Clean up docs in vars_funs.py

6d63b51

jeancochrane linked an issue Nov 20, 2024 that may be closed by this pull request

Create and deploy a minimal Python version of this package #29

Closed

5 tasks

jeancochrane added 15 commits November 20, 2024 16:43

Fix typo in pytest-coverage workflow

f3f38e1

Accept any python >=3.9 in python package

abdeca0

Use optional-dependencies for dev deps in pyproject.toml

ec0a63e

Fix vars_rename docstring in Python package

08e418a

Update typing in vars_funs.py to be compatible with Python 3.9

001b079

Add Sphinx docs for Python package

2aae031

Update actions/checkout versions across workflows

4513bcb

Add Python docs generation to docs workflow

44fd662

Fix ruff linter errors

4070f6f

Install both test and docs requirements when running pytest

854aefc

Fix paths in pytest-coverage workflow

8afc1d4

Better path management in docs conf.py

2549daf

Rename build jobs in docs workflow

723834e

Include csv files in package data when building Python package

c323370

Temporarily disable branch restriction for docs deployment to test it…

9cc256b

… out

jeancochrane had a problem deploying to github-pages November 21, 2024 23:00 — with GitHub Actions Failure

jeancochrane had a problem deploying to github-pages November 21, 2024 23:04 — with GitHub Actions Failure

jeancochrane had a problem deploying to github-pages November 21, 2024 23:07 — with GitHub Actions Failure

Update deploy-pages version

eb9a619

jeancochrane temporarily deployed to github-pages November 21, 2024 23:15 — with GitHub Actions Inactive

Revert "Temporarily disable branch restriction for docs deployment to…

bbbaf68

… test it out" This reverts commit 9cc256b.

jeancochrane added 2 commits November 21, 2024 17:19

Fix broken link in Python docs

b0055b4

Switch to new style python type hints since we don't support 3.9 anyway

be12f3e

Remove unnecessary templates_path config from pyproject.toml

bd6835d

jeancochrane commented Nov 21, 2024

View reviewed changes

Empty commit to try to bust build-pkgdown-site actions cache

5eb1e23

jeancochrane marked this pull request as ready for review November 21, 2024 23:33

jeancochrane requested a review from a team as a code owner November 21, 2024 23:33

jeancochrane requested a review from dfsnow November 21, 2024 23:41

dfsnow approved these changes Nov 23, 2024

View reviewed changes

jeancochrane added 8 commits November 25, 2024 12:10

Remove unnecessary .python-version file

1b6bbef

Add pip install directions to README and index.rst for Python package

bca6864

Remove unnecessary uv.lock file

5960235

Rename 'test' -> 'dev' in pyproject optional-dependencies

aaebf7c

Switch order of authors in pyproject.toml

cd5bc3e

Capitalize VAR_NAME_PREFIX constant in vars_funs.py

c8f4e49

Remove unnecessary OutputType enum from vars_funs.py

81d8c20

Remove duplicative type checking in vars_funs.py

b837865

jeancochrane force-pushed the jeancochrane/29-create-and-deploy-a-minimal-python-version-of-this-package branch from c1c40d9 to b837865 Compare November 25, 2024 19:37

jeancochrane and others added 2 commits November 25, 2024 13:41

Wrap Python tests in classes for clearer organization

848db78

Change chars_sample fixtures to symlinks to R data in Python package

e66eff3

jeancochrane requested a review from dfsnow November 25, 2024 20:04

dfsnow approved these changes Nov 25, 2024

View reviewed changes

jeancochrane merged commit 5be78a6 into master Nov 26, 2024
14 checks passed

jeancochrane deleted the jeancochrane/29-create-and-deploy-a-minimal-python-version-of-this-package branch November 26, 2024 17:16

This was referenced Nov 26, 2024

Finish migration of vars_funs module for Python package #32

Merged

Make the the Python package compatible with Athena PySpark #33

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define a minimal Python version of the package containing `vars_rename` #31

Define a minimal Python version of the package containing `vars_rename` #31

jeancochrane commented Nov 20, 2024 •

edited

Loading

codecov bot commented Nov 21, 2024

jeancochrane Nov 21, 2024

dfsnow Nov 23, 2024

jeancochrane Nov 25, 2024

dfsnow Nov 25, 2024

jeancochrane Nov 21, 2024

dfsnow Nov 23, 2024

jeancochrane Nov 25, 2024

dfsnow left a comment

dfsnow Nov 23, 2024

dfsnow Nov 23, 2024

jeancochrane Nov 25, 2024

dfsnow Nov 23, 2024

jeancochrane Nov 25, 2024

dfsnow Nov 23, 2024

jeancochrane Nov 25, 2024

dfsnow Nov 23, 2024

dfsnow Nov 23, 2024

jeancochrane Nov 25, 2024

dfsnow Nov 23, 2024

jeancochrane Nov 25, 2024

dfsnow Nov 23, 2024

jeancochrane Nov 25, 2024

dfsnow Nov 23, 2024

dfsnow Nov 23, 2024

jeancochrane Nov 25, 2024

jeancochrane commented Nov 25, 2024

dfsnow left a comment

dfsnow Nov 25, 2024

		@@ -0,0 +1,71 @@
		"pin","year","class","char_yrblt","char_bldg_sf","char_land_sf","char_beds","char_rooms","char_fbath","char_hbath","char_frpl","char_type_resd","char_cnst_qlty","char_apts","char_tp_dsgn","char_attic_fnsh","char_gar1_att","char_gar1_area","char_gar1_size","char_gar1_cnst","char_attic_type","char_bsmt","char_ext_wall","char_heat","char_repair_cnd","char_bsmt_fin","char_roof_cnst","char_use","char_age","char_site","char_ncu","char_renovation","char_porch","char_air","char_tp_plan"

		{name = "Dan Snow", email="[email protected]"},
		{name = "Jean Cochrane", email="[email protected]"},

Define a minimal Python version of the package containing vars_rename #31

Define a minimal Python version of the package containing vars_rename #31

Conversation

jeancochrane commented Nov 20, 2024 • edited Loading

codecov bot commented Nov 21, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfsnow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeancochrane commented Nov 25, 2024

dfsnow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Define a minimal Python version of the package containing `vars_rename` #31

Define a minimal Python version of the package containing `vars_rename` #31

jeancochrane commented Nov 20, 2024 •

edited

Loading