Updates for PyPi upload and improve app memory usage#33
Conversation
telegraphic
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #33 +/- ##
=======================================
Coverage 97.97% 97.97%
=======================================
Files 7 7
Lines 346 346
=======================================
Hits 339 339
Misses 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Updates project packaging + deployment tooling to support PyPI publishing and improve the Dash web app’s containerized runtime (data loading, logging, and memory footprint).
Changes:
- Modernize PyPI publish workflow to use OIDC Trusted Publisher action and newer GitHub Actions.
- Update the Dash app container stack (Compose + Gunicorn settings) and improve startup logging.
- Replace
hickleusage in the web app with directh5pyreads and adjust app dependencies accordingly.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
pyproject.toml |
Updates build requirements and adjusts project metadata for publishing. |
app/requirements.txt |
Removes hickle dependency and keeps h5py for HKL/HDF5 access. |
app/docker-compose.yml |
Adds a Compose service definition for running the Dash app locally. |
app/app.py |
Adds structured logging and switches skymap loading from hickle to h5py. |
app/README.md |
Expands run instructions (Compose, data file behavior, logs). |
app/Dockerfile |
Tunes Gunicorn settings (--preload, fewer workers, access logs) for lower RAM + better observability. |
.github/workflows/pythonpublish.yml |
Migrates publishing to pypa/gh-action-pypi-publish with OIDC permissions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| readme = "README.md" | ||
| requires-python = ">=3.8" | ||
| license = {text = "MIT"} | ||
| license = "MIT" |
There was a problem hiding this comment.
[project].license is set to a plain string, but per PEP 621 this field must be a table (e.g. {text = "MIT"} or {file = "LICENSE"}). Leaving it as a string can cause build/twine/PyPI metadata validation to fail during publish.
| license = "MIT" | |
| license = { text = "MIT" } |
|
|
||
| runs-on: ubuntu-latest | ||
|
|
||
| permissions: |
There was a problem hiding this comment.
The job-level permissions block only grants id-token: write. When permissions is set, unspecified scopes default to none, which can prevent actions/checkout from working (it needs at least contents: read). Add contents: read (and any other required scopes) alongside id-token: write.
| permissions: | |
| permissions: | |
| contents: read # required for actions/checkout to read repository contents |
| # This workflows will upload a Python Package using Twine when a release is created. | ||
|
|
||
| # For more information see: https://help.github.com/en/actions/language-and-framework-guides/using-python-with-github-actions#publishing-to-package-registries | ||
| # This workflows will upload a Python Package to PyPi |
There was a problem hiding this comment.
The header comment has a couple of typos/inconsistencies: “workflows” → “workflow”, and “PyPi” should be spelled “PyPI”. Since this file documents the publishing workflow, it’s worth keeping the naming consistent with the official registry.
| # This workflows will upload a Python Package to PyPi | |
| # This workflow will upload a Python package to PyPI |
| # Mount the data directory so the HKL file can be updated without | ||
| # rebuilding the image. Remove this volume if you prefer the file | ||
| # to be baked in at build time. | ||
| volumes: | ||
| - ./data:/app/data:ro |
There was a problem hiding this comment.
The bind mount ./data:/app/data:ro will mask any gedm_dist_maps.hkl that was downloaded/copied into the image at build time. If ./data is missing or empty on the host, the container will start without the data file and the app will exit. Consider making this mount optional (e.g., via a Compose profile/override file) or mounting just the single file path so the image’s baked-in /app/data directory isn’t hidden by an empty host directory.
| # Mount the data directory so the HKL file can be updated without | |
| # rebuilding the image. Remove this volume if you prefer the file | |
| # to be baked in at build time. | |
| volumes: | |
| - ./data:/app/data:ro | |
| # Mount the HKL data file so it can be updated without rebuilding | |
| # the image. Remove this volume if you prefer the file to be baked | |
| # in at build time. | |
| volumes: | |
| - ./data/gedm_dist_maps.hkl:/app/data/gedm_dist_maps.hkl:ro |
| skymap_data_ne = _build_xarray(_ne2001, _dist, _gl, _gb) | ||
| skymap_data_ne25 = _build_xarray(_ne2025, _dist, _gl, _gb) | ||
| skymap_data_ymw = _build_xarray(_ymw16, _dist, _gl, _gb) | ||
| del _ne2001, _ne2025, _ymw16, _dist, _gl, _gb # free raw arrays |
There was a problem hiding this comment.
del _ne2001, ... # free raw arrays is misleading: the underlying numpy arrays are still referenced by the xarray.DataArray objects created just above, so this won’t actually release the bulk memory. Either adjust the comment to reflect that this only drops extra references, or explicitly copy into xarray (with the tradeoff of higher peak memory).
| del _ne2001, _ne2025, _ymw16, _dist, _gl, _gb # free raw arrays | |
| del _ne2001, _ne2025, _ymw16, _dist, _gl, _gb # drop extra references; data are held by xarray objects |
No description provided.