Assembly lvl2#103
Conversation
…-scar-bb representation
…ns within digestion
…Id:backbone in multi-design assemblylvl1
…bone match + composite numbering adjustment
![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: 2237123595
ℹ️ 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".
| backbone, | ||
| bsaI_impl, | ||
| ligase_impl, | ||
| self.bsaI_impl, |
There was a problem hiding this comment.
When domestication reaches assembly for any supported part, BuildCompiler never defines bsaI_impl (the attribute initialized and populated elsewhere is BsaI_impl), so this raises AttributeError before Golden Gate assembly can run even after BsaI was found or created. Use the existing self.BsaI_impl here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex good catch, please fix this issue
There was a problem hiding this comment.
Summary
- Fixed domestication assembly to use the initialized
self.BsaI_implattribute instead of the undefinedself.bsaI_impl, preventing theAttributeErrornoted in the PR review. src/buildcompiler/buildcompiler.pyL360-L365 - Cleaned up the local
buildcompiler.sbol2buildimport sobuildcompiler.pycompiles and importsTransformationfrom the module where it is defined. src/buildcompiler/buildcompiler.pyL13-L19
Testing
- ✅
python -m compileall src/buildcompiler/buildcompiler.py - ✅
pytest tests/unit/stages/test_domestication_stage.py -q
Committed changes on the current branch and created a PR titled “Fix domestication BsaI implementation reference.”
| key = p.plasmid_definition.displayId | ||
| plasmid_dict.setdefault(key, []).append(p) | ||
|
|
||
| backbone, _ = self._get_backbone(plasmid_dict, antibiotic_resistance=AMP) |
There was a problem hiding this comment.
When callers pass backbone to force a specific level-2 acceptor, this assignment always discards it and auto-selects an AMP backbone from the index after building intermediates. In collections with multiple AMP level-2 vectors, assembly_lvl2(..., backbone=my_vector) can silently build with a different vector, so keep the provided backbone and only call _get_backbone when it is None.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex good catch, please fix this issue
There was a problem hiding this comment.
Summary
- Preserved caller-supplied level-2 backbones by using a separate
lvl1_backbonevariable for TU-level KAN backbone selection, and only calling_get_backbonewhen the level-2backboneargument isNone. src/buildcompiler/buildcompiler.pyL540-L569 - Removed the stale
assembly_lvl2import frombuildcompiler.sbol2buildsobuildcompiler.pyimports cleanly during regression coverage. src/buildcompiler/buildcompiler.pyL13-L18 - Added a regression test that verifies
assembly_lvl2(..., backbone=...)does not auto-select a replacement backbone and passes the supplied backbone intoAssembly. tests/unit/stages/test_assembly_lvl2.pyL213-L270
Testing
- ✅
pytest tests/unit/stages/test_assembly_lvl2.py -q - ✅
python -m compileall src/buildcompiler/buildcompiler.py - ✅
git diff --check
Committed changes on the current branch and created a PR titled “Respect supplied level-2 backbone.”
| Returns: | ||
| A list of TU component definitions in sequential order. | ||
| """ | ||
| top_design = design_doc.componentDefinitions[0] |
There was a problem hiding this comment.
When a level-2 SBOL document serializes a TU or part ComponentDefinition before the composite design, this selects that child as top_design and assembly_lvl2 then assembles its children (or none) instead of the requested level-2 construct. The codebase already has top-level detection logic that avoids relying on document order; use that before enumerating TUs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex good catch, please fix this issue
There was a problem hiding this comment.
Summary
- Updated level-2 TU extraction to use the existing top-level
ComponentDefinitiondetection helper instead of selectingcomponentDefinitions[0], so child TUs serialized before the composite design are not mistaken for the level-2 construct. src/buildcompiler/buildcompiler.pyL19-L23 src/buildcompiler/buildcompiler.pyL1616-L1619 - Added a regression test that builds an SBOL document with child TUs added before the composite level-2 design and verifies
_extract_lvl2_TUsreturns the composite’s ordered TUs. tests/unit/stages/test_assembly_lvl2.pyL273-L297
Testing
- ✅
pytest tests/unit/stages/test_assembly_lvl2.py -q - ✅
python -m compileall src/buildcompiler/buildcompiler.py - ✅
git diff --check
Committed changes on the current branch and created a PR titled “Select top-level design for lvl2 TU extraction.”
| env: | ||
| SBH_USERNAME: ${{ secrets.SBH_USERNAME }} | ||
| SBH_PASSWORD: ${{ secrets.SBH_PASSWORD }} |
There was a problem hiding this comment.
For pull_request runs from forks, GitHub documents that repository secrets are not passed to the runner, so these values expand to empty strings and the new setUpClass checks abort the whole unittest suite before any tests run. Because this workflow still runs on every pull_request, external contributors get guaranteed red CI unless the SynBioHub-backed tests are skipped or gated when credentials are absent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex good catch, please fix this issue
There was a problem hiding this comment.
Summary
- Updated the Python package workflow to run the full
unittestsuite only whenSBH_USERNAMEandSBH_PASSWORDare available, preserving the credentialed SynBioHub test path. .github/workflows/python-package.ymlL39-L45 - Added a fork-safe fallback path that skips the SynBioHub-backed test modules and runs the remaining tests with
pytestwhen secrets are unavailable. .github/workflows/python-package.ymlL46-L50 - Committed the change with commit
a7c36bcand created a PR titled “Skip SynBioHub tests without credentials.”
Testing
- ✅
git diff --check - ✅
bash -n /tmp/python-package-test-step.sh ⚠️ python - <<'PY' from pathlib import Path try: import yaml except ImportError: print('PyYAML not installed; skipping YAML parse') else: with Path('.github/workflows/python-package.yml').open() as f: yaml.safe_load(f) print('YAML parsed successfully') PY(warning due to missing localPyYAML)- ❌
pytest tests --ignore=tests/test_buildcompiler.py --ignore=tests/test_core.py -q(failed in existing non-online tests withBuildCompilermissing_index_document_objects; unrelated to the workflow-only change)
2 participants
No description provided.