Class-based EntityReader lookups + app-runner FindMixin migration#563
Draft
leoschwarz wants to merge 4 commits into
Draft
Class-based EntityReader lookups + app-runner FindMixin migration#563leoschwarz wants to merge 4 commits into
leoschwarz wants to merge 4 commits into
Conversation
Replace all 16 deprecated Entity.find/find_all/find_by (FindMixin) read-path
call sites in bfabric_app_runner with the modern client.reader API
(read_id/read_ids/query_one). Behavior-preserving:
- find(id) -> reader.read_id(endpoint, id, expected_type=...)
- find_by(obj) -> reader.query_one(endpoint, obj, expected_type=...)
(callers only used the first/truthiness of the result)
- find_all(ids) -> reader.read_ids(endpoint, ids, expected_type=...)
read_ids is URI-keyed and includes None for misses, whereas find_all was
int-keyed and dropped misses. So:
* .values() consumers now filter out None
* id-indexed consumers re-key uri.components.entity_id -> entity and drop
None, preserving the prior "missing id -> KeyError" behavior
The four possibly-None accesses that were previously grandfathered in the
basedpyright baseline (execution.dataset arg, dataset.to_polars(),
registration.workunit_id/storage_id) are now handled with targeted
# pyright: ignore comments; the baseline shrinks accordingly (no new
suppressions).
Tests updated to mock the client.reader path (real EntityUri keys for
read_ids); added focused coverage for the re-key/None-drop paths in the
dataset, resource, and resource-flow resolvers.
…pers Deduplicate the 're-key EntityReader results by id / drop not-found' transform that FindMixin.find_all/find_by performed inline into two reusable free functions in bfabric.entities.core.reader_utils. FindMixin now delegates to entities_by_id, and the app-runner FindMixin-migration sites use the helpers instead of inline dict-comprehensions, collapsing the four bulk read_ids sites to one line each. No behavior change. The basedpyright baseline shrinks by one entry (find_by's grandfathered reportReturnType is now fixed via a localized cast).
read_id/read_ids/query/query_one now accept an entity class in place of the endpoint string (e.g. client.reader.read_id(Resource, id)), inferring both the endpoint and the result type via import_entity.entity_type_of (the class->string inverse of import_entity, keyed on the lowercase-class-name convention). The string form (with optional expected_type) still works. Adopt the class form across bfabric_app_runner + a few core sites, dropping the redundant endpoint string; narrow the read-only ResolveBfabricDatasetSpecs and ResolveBfabricResourceDatasetSpecs to take an EntityReader instead of the full Bfabric client.
leoschwarz
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FindMixin(Entity.find/find_all/find_by) is deprecated and emitsDeprecationWarningon every call;bfabric_app_runnerwas its heaviest consumer (16 sites). This migrates them toclient.readerand extracts the id-keying / not-found filtering those methods did inline into two reusable core helpers, so the migration deduplicates rather than scatters that logic.Key changes
bfabric— newbfabric.entities.core.reader_utilswithentities_by_id(re-key a reader result by integer id, drop not-found) andpresent_entities(drop not-found).FindMixin.find_all/find_bynow delegate toentities_by_id.bfabric_app_runner— all 16FindMixincall sites moved toclient.reader(read_id/read_ids/query_one); the four bulkread_idssites use the new helpers.bfabric(6) andbfabric_scripts(12)FindMixinsites are separate follow-ups.Notes for review
read_idsis URI-keyed and includesNonefor not-found (vsfind_all's int-keyed, misses-dropped). The id-indexed sites re-key and dropNone(so a missing id still raisesKeyErrordownstream, as before); the value-consuming sites dropNone.find_alllogged an "Only found N out of M" warning on partial misses. That warning is no longer emitted at the two value-consuming sites (dispatch_resource_flow, resource-dataset resolve), which now drop silently; the id-keyed sites still fail loudly viaKeyError.dispatch_individual_resources(keepsNonefor a friendly per-idValueError) and a pre-existing latentNonebug in_resolve_bfabric_annotation_specs.py(a separate follow-up, not yet filed).Testing
pytest tests/bfabric(763) andpytest tests/bfabric_app_runner(325),basedpyright(both packages, 0/0/0), andruff— all green. New unit tests cover the helpers and the re-key / None-drop paths.🤖 Prepared with assistance from Claude Opus 4.8 via Claude Code.