[ISSUE-05] Implement SBOL resolver and inventory foundation#72
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c672344bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def add_generated_product(self, product: IndexedPlasmid) -> None: | ||
| self.generated_products_by_identity[product.identity] = product | ||
| self._add_plasmid(product) |
There was a problem hiding this comment.
add_generated_product updates identity maps but then blindly calls _add_plasmid, which appends to all secondary lists without clearing any existing entry for the same product.identity. If the same product is indexed again (e.g., retries or state/metadata updates), lookups like find_single_part_plasmids and find_lvl1_region_plasmids can return duplicate or stale plasmid records, which can cascade into duplicate planning/execution actions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Fixed stale/duplicate plasmid secondary-index behavior by adding
_remove_plasmid_from_secondary_indexes, which removes old insert/fusion-site/antibiotic mappings for an existing plasmid identity before reindexing. This ensures deterministic replacement instead of append-only duplication when the same generated product is added again. src/buildcompiler/inventory/inventory.pyL69-L120 - Updated
_add_plasmidto detect existing identity entries and clear their old secondary-index references before inserting updated data, while keeping identity map behavior unchanged. src/buildcompiler/inventory/inventory.pyL96-L120 - Added a regression test that re-adds the same generated product identity with changed metadata and validates old index keys are cleared and new keys contain only the updated record. tests/unit/inventory/test_inventory_indexes.pyL73-L100
Testing
- ✅
pytest tests/unit/inventory/test_inventory_indexes.py
| self.maybe_pull(identity) | ||
| obj = self.document.find(identity) |
There was a problem hiding this comment.
After maybe_pull(identity) in MISSING_ONLY mode, the resolver only re-checks document.find(...) and ignores the pull return value, so resolution works only if pull_client mutates document as an implicit side effect. A valid pull client that returns the fetched SBOL object directly will still trigger LookupError, even though retrieval succeeded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Fixed
SbolResolver._getso that in bothALWAYS_REFRESHandMISSING_ONLYmodes, ifpull_clientreturns an object of the expected SBOL type, that object is returned immediately (instead of requiringdocument.find(...)to succeed via side effect). This resolves the review feedback atsrc/buildcompiler/sbol/resolver.pyline 50. src/buildcompiler/sbol/resolver.pyL40-L56 - Added a regression helper pull client (
ReturnOnlyPullClient) and a targeted test that verifiesMISSING_ONLYresolution succeeds even when the pull client does not mutate the local document. tests/unit/sbol/test_resolver.pyL17-L25 tests/unit/sbol/test_resolver.pyL96-L109 - Committed changes on the current branch with commit message:
Fix resolver to accept pulled SBOL objects directly. - Created a follow-up PR entry via
make_prwith title:[ISSUE-05 Follow-up] Fix resolver behavior for non-mutating pull clients.
Testing
- ✅
pytest tests/unit/sbol/test_resolver.py
Gonza10V
deleted the
codex/implement-sbol-resolver-and-inventory-foundation-i2plk2
branch
1 participant
Motivation
Documentlookups.Inventoryfacade that indexes plasmids, backbones, and reagents for fast deterministic queries needed by the full-build pipeline.Description
PullPolicyand a smallSbolResolverthat resolves SBOL objects from a localsbol2.Documentand optionally calls an injectedpull_client; exported asbuildcompiler.sbol.PullPolicyandbuildcompiler.sbol.SbolResolverinsrc/buildcompiler/sbol/.Inventory(eager deterministic indexes and incremental generated-product indexing) with the required query helpers and exports insrc/buildcompiler/inventory/.src/buildcompiler/domain(reusedIndexedPlasmid,IndexedBackbone,IndexedReagent,MaterialState, andBuildStage) and relied on a minimalmetadataconvention for insert identities, fusion sites, antibiotics, and optional backbonestagevalues.src/buildcompiler/sbol/resolver.py,src/buildcompiler/sbol/__init__.py,src/buildcompiler/inventory/inventory.py,src/buildcompiler/inventory/__init__.py,tests/unit/sbol/test_resolver.py, andtests/unit/inventory/test_inventory_indexes.py.Testing
python -c "from buildcompiler.sbol import SbolResolver, PullPolicy",python -c "from buildcompiler.inventory import Inventory", andpython -c "from buildcompiler.api.options import BuildOptions; BuildOptions()"all succeeded.pytest tests/unit/sbol tests/unit/inventoryandpytest tests/unit/domain tests/unit/api tests/unit/sbol tests/unit/inventoryran successfully (all tests passed locally).uv run pytest ...failed due to the environment trying to fetch an optional git dependency (SBOLInventory) and being blocked by network/git access (HTTP 403), souv-based virtualenv setup was not completed in this environment.ruff check . && ruff format --check .failed due to pre-existing unrelated lint issues elsewhere in the repository (not introduced by this PR) and is left outside the scope of this change.Codex Task