Skip to content

fix: make TaskInstance and Realization hashable#26

Merged
jeqcho merged 3 commits into
robocurve:mainfrom
Adityakk9031:#4
Jul 21, 2026
Merged

fix: make TaskInstance and Realization hashable#26
jeqcho merged 3 commits into
robocurve:mainfrom
Adityakk9031:#4

Conversation

@Adityakk9031

@Adityakk9031 Adityakk9031 commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

closes:#4

Summary:

TaskInstance and Realization are decorated as frozen dataclasses, but they contain dictionary fields. By default, Python's generated hash method attempts to hash these mutable dictionaries, raising a TypeError. This PR implements custom hash methods for both classes to convert their dictionary fields into sorted tuples of items, resolving the unhashable type error and allowing them to be used in sets or as dictionary keys.

Changes:

  • src/kitchenbench/instances.py: Added custom hash implementations to Realization and TaskInstance.
  • tests/test_instances.py: Added test_instances_and_realizations_are_hashable to assert that TaskInstance, Realization, and TaskSpec instances are hashable.

Verification:

  • Ran all tests via pytest, maintaining 100% code coverage.
  • Verified that ruff checks and mypy type checks pass.

@Adityakk9031

Copy link
Copy Markdown
Contributor Author

@jeqcho have a look

@jeqcho

jeqcho commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

@Adityakk9031 Hi, happy to review again if the failing tests are fixed.

@jeqcho

jeqcho commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Thanks @Adityakk9031!

On the red CI: the macOS py3.12 failure was our infrastructure, not you — setup-uv got rate-limited before anything of yours ran, and I've re-run that job. The lint failure is real but trivial: ruff format --check wants to reformat src/kitchenbench/instances.py and tests/test_instances.py. Run uv run ruff format ., commit, push, and it should go green. I'll review properly once it is.

@Adityakk9031

Copy link
Copy Markdown
Contributor Author

@jeqcho have a look

@Adityakk9031

Copy link
Copy Markdown
Contributor Author

@jeqcho check latest commit

@jeqcho
jeqcho merged commit d66cc2a into robocurve:main Jul 21, 2026
9 checks passed
@jeqcho

jeqcho commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Merged, thanks @Adityakk9031. The part I want to call out: relying on dataclass(frozen=True) keeping an explicitly defined __hash__ rather than fighting it with unsafe_hash or eq=False was the clean way through, and covering TaskSpec in the test even though the PR never touches specs.py shows you traced the transitive fix (it embeds tuple[TaskInstance, ...]). Nothing in either hash is dropped relative to __eq__, which is the invariant that matters.

Review surfaced two small follow-ups, written up as #30: an equal-objects-equal-hash test (the current test passes even under identity hashing), and the static: dict[str, Any] annotation that could let a future spec silently reintroduce the bug this PR fixed. First refusal is yours if you want them. Thanks again for sticking through the format rounds.

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