Test updates: stabler debup hash and cli integration coverage#462
Conversation
| test_file = tmp_path / "test_file.txt" | ||
| test_file.write_text("test file") | ||
| aws_handler.upload_file(str(test_file)) |
There was a problem hiding this comment.
a bit out of scope but a simple change, switched tests to write via tmp_path so they leave no trace like test_file.txt in the repo root
| pack(recipe=recipe_data, config_path=str(config_path)) | ||
|
|
||
|
|
||
| def test_pack_with_default_config(tmp_path, monkeypatch): |
There was a problem hiding this comment.
similar to tmp_path in aws tests, we don't want test files written inside the repo, so we change directory to tmp_path with monkeypatch.chdir()
There was a problem hiding this comment.
Pull request overview
This PR strengthens recipe deduplication by normalizing recipe JSON prior to hashing (so semantically equivalent recipes hash the same), and adds/updates tests to cover both the hashing behavior and the CLI workflow.
Changes:
- Add
DataDoc._normalize_for_hashing()and updategenerate_hash()to canonicalize order-insensitive lists while preserving positional numeric lists. - Add integration tests for
cellpack/bin/pack.pycovering recipe path input, recipe dict input, and default config behavior. - Refactor AWS handler tests to write temporary files under
tmp_pathinstead of the working directory.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
cellpack/autopack/DBRecipeHandler.py |
Implements normalization logic to produce stable dedup hashes across semantically equivalent JSON. |
cellpack/tests/test_data_doc.py |
Adds test cases that assert hash stability across dict key order and order-insensitive list permutations. |
cellpack/tests/test_pack_cli.py |
Introduces CLI integration tests covering recipe-as-path, recipe-as-dict, and default config flows. |
cellpack/tests/test_aws_handler.py |
Updates tests to use tmp_path for safer cross-platform temp file handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_pack_with_recipe_path(tmp_path): | ||
| recipe_path = tmp_path / "recipe.json" | ||
| recipe_path.write_text(json.dumps(recipe_data)) | ||
| config_path = _write_config(tmp_path) | ||
| pack(recipe=str(recipe_path), config_path=str(config_path)) |
There was a problem hiding this comment.
recipe_data is a module-level dict that gets passed into pack() (directly or via json.dumps). RecipeLoader mutates the input dict when recipe is a dict (it reuses nested references and later replaces objects[*]["representations"]/partners with class instances), so test order can become flaky (e.g., json.dumps(recipe_data) can fail after another test runs). Use a per-test fresh recipe dict (e.g., a pytest fixture returning a new dict or copy.deepcopy(recipe_data) at call sites) before writing/packing.
meganrm
left a comment
There was a problem hiding this comment.
looks good, thanks for all the tests
mogres
left a comment
There was a problem hiding this comment.
Very clever solution using ._is_positional!! Looks good to me and thanks for adding tests (and fixing the test file issue)!
* user workflow improvements and config updates (#411) * default upload_results to false * update config file options * update schema docs * propose to remove config files from the packing commands but add a section to explain how configs work * set open_results_in_browser to false since it's dependent on upload_results * improve loggings * add a config file for upload results * correct option default and log level * update schema * add aws db to path * add a layer to detect non-interactive envs for db selection * fix readme * add cellpack website url * remove unnecessary restriction * update readme * add screenshots to readme (#413) * add screenshots to readme * Quick fix: update bbox value in recipes (#420) * add randomness_seed to upload and download (#416) * update bbox value in recipes * add config option for plot display * replace result screenshots and update readme * update docs/index to display images correctly * Add description field to recipe schema and backend utilities (#430) * add `description` and `randomness_seed` to top level recipe settings * add description in example recipes * add description to download and upload script * add description field to unit tests * pr commenting: use native github CLI (#432) * test github cli * remove test branch * Minimal fix: set camera position (#431) * set camera * use max dimension * add checks for pull requests (#434) * Feature/client upload script (#417) * add upload script * add example data and more documentation * point to correct collection * return firebase IDs from upload_recipe adn upload_config * add documentation * add installation step * lint fixes * lint fix * fix typos * keep all upload functionality in upload.py, and rename client to studio * remove accidential line * update example editable fields to match updates we've done * add output param * lint fixes * small wording fixes * make config_path optional * ci(dependabot): bump astral-sh/setup-uv from 4 to 7 (#436) Bumps [astral-sh/setup-uv](https://github.com/astral-sh/setup-uv) from 4 to 7. - [Release notes](https://github.com/astral-sh/setup-uv/releases) - [Commits](astral-sh/setup-uv@v4...v7) --- updated-dependencies: - dependency-name: astral-sh/setup-uv dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Fix Comment Update job: use gh api instead of gh pr review (#438) * use gh api instead of gh pr review * remove backslashes * trigger workflow to test comment update * use patch instead of delete * take2-trigger workflow to test comment update * use an actual new line to avoid \n being treated as literal chars * Studio: add recipe and config json to outputs for downloading (#435) * add recipe and config to output * refactor * path guard * Potential fix for code scanning alert no. 21: Uncontrolled data used in path expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> * format * fix 404 comment * handle firebase recipe, serialize it before passing to s3 * save recipe data directly to outputs * remove unused import * one more unused import --------- Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> * ci(dependabot): bump actions/upload-artifact from 4 to 6 (#439) Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 4 to 6. - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](actions/upload-artifact@v4...v6) --- updated-dependencies: - dependency-name: actions/upload-artifact dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Miscellaneous gradient updates (#428) * Add gradient weights to Environment.py and Gradient.py * Remove unused import * Refactor Environment.py and utils.py This commit refactors the `Environment.py` file by separating the loading of grid, compartment grids, and mesh store into separate loops. It also clears the triangles_tree cache and resets the grid. Additionally, the `load_object_from_pickle` function has been removed from `utils.py` as it is no longer used. * Ignore venv folders * Update logging level * Update `aicsimageio` to `bioio` - Also update the logging of recipe validation to `debug` * Suppress logging outputs for lower level functions * Update logs in `ImageWriter.py` * Update path preference loading to catch empty json * Update logging for recipe validation * sort imports * Update uv lock * Allow gradient weights to be specified as a dict while combining * Add uniform weight map option * Fix gradient weight scaling for equal weights * Update scaling to just divide by max value * Add recipe to test gradients * Rename scaling method to normalize_by_max_value and update references * Use uniform weight if `decay_length` is 0 * Linting updates * Update validation for decay length * Update validation for multiple gradients * Add zero decay gradient in test recipe * ci(dependabot): bump aws-actions/configure-aws-credentials from 4 to 5 (#447) * ci(dependabot): bump aws-actions/configure-aws-credentials from 4 to 5 Bumps [aws-actions/configure-aws-credentials](https://github.com/aws-actions/configure-aws-credentials) from 4 to 5. - [Release notes](https://github.com/aws-actions/configure-aws-credentials/releases) - [Changelog](https://github.com/aws-actions/configure-aws-credentials/blob/main/CHANGELOG.md) - [Commits](aws-actions/configure-aws-credentials@v4...v5) --- updated-dependencies: - dependency-name: aws-actions/configure-aws-credentials dependency-version: '5' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> * matching the regular pr format * trigger codeQL --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ruge Li <rugeli0605@gmail.com> * ci(dependabot): bump peter-evans/create-pull-request from 7 to 8 (#450) Bumps [peter-evans/create-pull-request](https://github.com/peter-evans/create-pull-request) from 7 to 8. - [Release notes](https://github.com/peter-evans/create-pull-request/releases) - [Commits](peter-evans/create-pull-request@v7...v8) --- updated-dependencies: - dependency-name: peter-evans/create-pull-request dependency-version: '8' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Use native gh CLI for requirements actions (#453) * update UV setup to version 7 * update Dependabot configuration * remove unnecessary permissions for open-PR job in make-requirements.yml * Make timestamp unique using the run ID * Avoid expanding the WORKFLOW_PAT * ci(dependabot): bump the actions group with 4 updates (#456) Bumps the actions group with 4 updates: [actions/checkout](https://github.com/actions/checkout), [actions/upload-artifact](https://github.com/actions/upload-artifact), [aws-actions/configure-aws-credentials](https://github.com/aws-actions/configure-aws-credentials) and [actions/download-artifact](https://github.com/actions/download-artifact). Updates `actions/checkout` from 4 to 6 - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v4...v6) Updates `actions/upload-artifact` from 6 to 7 - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](actions/upload-artifact@v6...v7) Updates `aws-actions/configure-aws-credentials` from 5 to 6 - [Release notes](https://github.com/aws-actions/configure-aws-credentials/releases) - [Changelog](https://github.com/aws-actions/configure-aws-credentials/blob/main/CHANGELOG.md) - [Commits](aws-actions/configure-aws-credentials@v5...v6) Updates `actions/download-artifact` from 6 to 8 - [Release notes](https://github.com/actions/download-artifact/releases) - [Commits](actions/download-artifact@v6...v8) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions - dependency-name: actions/upload-artifact dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions - dependency-name: aws-actions/configure-aws-credentials dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions - dependency-name: actions/download-artifact dependency-version: '8' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Upgrading dependencies (#457) * Upgrading dependencies * Update black target version * Formatting via `uv run black cellpack` * Updating requirements.txt after change to `uv.lock` was pushed to `main` (#458) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * Server should Accept Recipe JSON (#419) * add upload script * add example data and more documentation * point to correct collection * have server accept recipe as json object in body of request * update documentation * remove accidential dockerfile changes * rename param json_recipe * remove file that shouldn't be in this PR * remove accidential file * lint fixes * refactor to try to improve clarity of json recipe vs file path * lint fixes * lint fix * minimize changeset * minimize changeset * simplify changeset * code cleanup * minimize changeset * remove trailing comma * Feature/firebase lookup (#445) * remove os fetch for job_id * use dedup_hash instead of job id * proposal: get hash from recipe loader * renaming and add TODOs * format * rename param to hash * remove unused validate param and doc strings in pack * simplify get_ dedup_hash * refactor job_status update * cleanup * fix upload_job_status to handle awshandler * pass dedup_pash to env for fetching across files * add tests * format1 * format test * Only upload simularium file once (#446) * proposal: get hash from recipe loader * simplify get_ dedup_hash * only post simularium results file once for server job runs * update code for rebase * code cleanup --------- Co-authored-by: Ruge Li <rugeli0605@gmail.com> * Maint/firebase collection cleanup (#448) * remove local metadata writes for auto-pop feature * remove cleanup firebase workflow * remove cleanup firebase code * 1. make doc url a constant 2.remove unused param * handle both recipe_path and json body requests (#449) * change error message body * lint fixes * add more checks when attempting to read json body * add upload script * add example data and more documentation * point to correct collection * have server accept recipe as json object in body of request * update documentation * remove accidential dockerfile changes * rename param json_recipe * remove file that shouldn't be in this PR * remove accidential file * lint fixes * refactor to try to improve clarity of json recipe vs file path * lint fixes * lint fix * minimize changeset * minimize changeset * simplify changeset * code cleanup * minimize changeset * remove trailing comma * Feature/firebase lookup (#445) * remove os fetch for job_id * use dedup_hash instead of job id * proposal: get hash from recipe loader * renaming and add TODOs * format * rename param to hash * remove unused validate param and doc strings in pack * simplify get_ dedup_hash * refactor job_status update * cleanup * fix upload_job_status to handle awshandler * pass dedup_pash to env for fetching across files * add tests * format1 * format test * Only upload simularium file once (#446) * proposal: get hash from recipe loader * simplify get_ dedup_hash * only post simularium results file once for server job runs * update code for rebase * code cleanup --------- Co-authored-by: Ruge Li <rugeli0605@gmail.com> * Maint/firebase collection cleanup (#448) * remove local metadata writes for auto-pop feature * remove cleanup firebase workflow * remove cleanup firebase code * 1. make doc url a constant 2.remove unused param * handle both recipe_path and json body requests (#449) * change error message body * lint fixes * add more checks when attempting to read json body * let recipe loader check the input and key stripping * Update cellpack/autopack/writers/__init__.py Co-authored-by: Saurabh Mogre <saurabh.mogre@alleninstitute.org> * use `isinstance` for AWSHandler, and misc * update aws tests * initialize dedup_hash * add in-line comment * temp solution: use requirement.txt --------- Co-authored-by: Ruge Li <91452427+rugeli@users.noreply.github.com> Co-authored-by: Ruge Li <rugeli0605@gmail.com> Co-authored-by: Saurabh Mogre <saurabh.mogre@alleninstitute.org> * ci(dependabot): bump the actions group across 1 directory with 4 updates (#466) Bumps the actions group with 4 updates in the / directory: [actions/configure-pages](https://github.com/actions/configure-pages), [actions/upload-pages-artifact](https://github.com/actions/upload-pages-artifact), [actions/deploy-pages](https://github.com/actions/deploy-pages) and [codecov/codecov-action](https://github.com/codecov/codecov-action). Updates `actions/configure-pages` from 5 to 6 - [Release notes](https://github.com/actions/configure-pages/releases) - [Commits](actions/configure-pages@v5...v6) Updates `actions/upload-pages-artifact` from 4 to 5 - [Release notes](https://github.com/actions/upload-pages-artifact/releases) - [Commits](actions/upload-pages-artifact@v4...v5) Updates `actions/deploy-pages` from 4 to 5 - [Release notes](https://github.com/actions/deploy-pages/releases) - [Commits](actions/deploy-pages@v4...v5) Updates `codecov/codecov-action` from 5 to 6 - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](codecov/codecov-action@v5...v6) --- updated-dependencies: - dependency-name: actions/configure-pages dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions - dependency-name: actions/upload-pages-artifact dependency-version: '5' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions - dependency-name: actions/deploy-pages dependency-version: '5' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions - dependency-name: codecov/codecov-action dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Test updates: stabler debup hash and cli integration coverage (#462) * normalize data order before hashing and tests * test pack cli * refactor test file: use tmp_path instead of writing to a working dir * cleanup * formatting * add recipe fixture * Remove Support for Recipe URL Param (#464) * we only support recipe as JSON body now, remove support for reading recipe file path from URL * update documentation to use new communication protocol and strip out references to AWS batch * remove unused import * Maint/deprecate requirements txt (#467) * remove requirements.txt related code * update pyproject.toml * generate new uv file * update pip install for dockerfile * use uv sync * move server dep list under dependency-groups * update docker * fix path and use uv sync * add pydantic to pyproject toml * update lock file * Feature/v2 bloodplasma (#471) * update migration script and validation * add v2 blood plasma recipe * ci(dependabot): bump actions/add-to-project in the actions group (#469) Bumps the actions group with 1 update: [actions/add-to-project](https://github.com/actions/add-to-project). Updates `actions/add-to-project` from 1.0.2 to 2.0.0 - [Release notes](https://github.com/actions/add-to-project/releases) - [Commits](actions/add-to-project@v1.0.2...v2.0.0) --- updated-dependencies: - dependency-name: actions/add-to-project dependency-version: 2.0.0 dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * fix KeyError: 'weight_mode' when packing v2.1 recipes with under-specified gradients (#470) * Add recipe and config to test gradient mixing in peroxisome data * normalize grad in early process * add tests --------- Co-authored-by: mogres <saurabh.mogre@alleninstitute.org> * pack: log resolved cellpack package path at DEBUG level Adds a DEBUG-level log of which cellpack package pack.py imported (autopack path), so it is easy to confirm whether a run used an installed/pinned cellpack vs a checked-out branch when debugging. Kept at DEBUG so normal packing output is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Ruge Li <91452427+rugeli@users.noreply.github.com> Co-authored-by: Alli <111383930+ascibisz@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Co-authored-by: Ruge Li <rugeli0605@gmail.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>


Problem
follow-up to #419, closes #460
Solution
_normalize_for_hashingto sort lists whose element order is not meaningful (e.g."interior": ["A", "B", "C"],"interior": [ {"object": "green_sphere", "count": 5}, "outer_sphere"]), while leaving order-sensitive lists(vectors, colors, etc.) unchanged.test_pack_cli.pytmp_pathinstead of the working dirOne note: this change can alter hash values for recipes that have lists, so re-run the same recipe after this merge may hash differently. Shouldn't cause issues, but worth noting so we can keep an eye on database tidiness.
Type of change