feat: use primer3-py for primer3 instead of the executable #101
Conversation
| @@ -208,7 +208,7 @@ def __str__(self) -> str: | |||
| """ | |||
| # If the bases field is None, replace with MISSING_BASES_STRING | |||
| bases: str = self.bases if self.bases is not None else MISSING_BASES_STRING | |||
| return f"{bases}\t{self.tm}\t{self.penalty}\t{self.span}" | |||
| return f"{bases}\t{self.tm:.2f}\t{self.penalty:.2f}\t{self.span}" | |||
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #101 +/- ##
=======================================
Coverage ? 96.99%
=======================================
Files ? 26
Lines ? 1697
Branches ? 184
=======================================
Hits ? 1646
Misses ? 29
Partials ? 22 ☔ View full report in Codecov by Sentry. |
prymer/primer3/primer3.py
Outdated
| for tag, value in assembled_primer3_tags.items(): | ||
| tag_str = f"{tag}" | ||
| if tag_str.startswith("SEQUENCE"): | ||
| seq_args[tag_str] = value | ||
| else: | ||
| global_args[tag] = value |
There was a problem hiding this comment.
This works ... but I think we could probably also add a is_global_arg() to Primer3InputTag that does this more efficiently (e.g. based on ordinal) or split Primer3InputTag into two classes - one for globals and one for sequence tags?
There was a problem hiding this comment.
Since PrimerInputTag's value is a string, I don't think it's inefficient, so I am going to centralize this as properties in the enum, and get rid of the f-string call (since I don't think that is needed).
prymer/primer3/primer3.py
Outdated
| for key, value in design_primers_retval.items(): | ||
| # Because Primer3 will emit both the input given and the output generated, we | ||
| # discard the input that is echo'ed back by looking for tags (keys) | ||
| # that do not match any Primer3InputTag | ||
| if not any(key == item.value for item in Primer3InputTag): | ||
| primer3_results[key] = value |
There was a problem hiding this comment.
Is this actually needed? I don't think there's any harm in allow the inputs to be present in the dict() used below.
If you feel strongly about this .. perhaps we have a more efficient way than looping through all the values in Primer3InputTag - e.g. create a _ALL_INPUT_TAG_STRINGS set and at the class level and check for membership?
There was a problem hiding this comment.
I'll keep removing them, we can open a PR later if you want to see them. I made it not inefficient via set membership.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
WalkthroughThe pull request introduces several modifications across the Prymer library. In Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
prymer/primer3/primer3.py (2)
248-256: Remove unnecessary call tosuper().__exit__().Since
AbstractContextManagerdoes not have an operational__exit__method, callingsuper().__exit__()is redundant and can be omitted.Apply this diff to remove the call:
def __exit__( self, exc_type: Optional[type[BaseException]], exc_value: Optional[BaseException], traceback: Optional[TracebackType], ) -> None: """Gracefully terminates any running subprocesses.""" - super().__exit__(exc_type, exc_value, traceback) self.close()
383-393: Simplify tag separation with dictionary comprehensions.You can streamline the splitting of
assembled_primer3_tagsintoseq_argsandglobal_argsusing dictionary comprehensions.Apply this refactor:
- seq_args = {} - global_args = {} - for tag, value in assembled_primer3_tags.items(): - if tag.is_sequence_arg: - seq_args[tag] = value - else: - global_args[tag] = value + seq_args = {tag: value for tag, value in assembled_primer3_tags.items() if tag.is_sequence_arg} + global_args = {tag: value for tag, value in assembled_primer3_tags.items() if not tag.is_sequence_arg}prymer/primer3/primer3_input_tag.py (1)
34-43: Enhance docstrings for clarity.Provide more detailed docstrings for
is_sequence_argandis_global_argto explain their roles and usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
prymer/api/oligo.py(1 hunks)prymer/primer3/primer3.py(9 hunks)prymer/primer3/primer3_input_tag.py(1 hunks)pyproject.toml(2 hunks)tests/api/test_oligo.py(9 hunks)tests/primer3/test_primer3.py(2 hunks)tests/primer3/test_primer3_input_tag.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- prymer/api/oligo.py
- tests/api/test_oligo.py
🧰 Additional context used
🪛 GitHub Check: codecov/patch
prymer/primer3/primer3.py
[warning] 406-406: prymer/primer3/primer3.py#L406
Added line #L406 was not covered by tests
[warning] 408-408: prymer/primer3/primer3.py#L408
Added line #L408 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests (3.12)
🔇 Additional comments (6)
prymer/primer3/primer3.py (1)
404-410: 🛠️ Refactor suggestionAdd tests for error handling paths.
Lines 406 and 409 raise exceptions but are not covered by tests. Enhance your test suite to include scenarios that trigger these exceptions.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 406-406: prymer/primer3/primer3.py#L406
Added line #L406 was not covered by tests
[warning] 408-408: prymer/primer3/primer3.py#L408
Added line #L408 was not covered by teststests/primer3/test_primer3_input_tag.py (1)
4-9: Tests effectively validate new properties.The test function accurately verifies the
is_sequence_argandis_global_argproperties for all tags.tests/primer3/test_primer3.py (2)
162-162: LGTM: Error message is more specific.New error message better reflects the actual validation failure.
364-364: LGTM: Error message matches new implementation.Error message correctly reflects file I/O error instead of subprocess termination.
pyproject.toml (2)
43-43: LGTM: Added primer3-py dependency.Version constraint ^2.0.3 is appropriate.
122-122: LGTM: Updated mypy configuration.Added primer3 to ignore_missing_imports list.
|
|
||
| # Check for any errors. Typically, these are in error_lines, but also the results can | ||
| # contain the PRIMER_ERROR key. | ||
| if "PRIMER_ERROR" in primer3_results or len(error_lines) > 0: | ||
| primer3_error("Primer3 failed") | ||
| if "PRIMER_ERROR" in primer3_results: |
There was a problem hiding this comment.
I'm not sure I follow what's happening here. error_lines is no longer present, could be removed from the comment on L402-403. Is the else on L407-408 unreachable?
There was a problem hiding this comment.
Fixed, silly copy and paste left an extra outer if. Good catch!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
prymer/primer3/primer3.py (2)
442-442: Consider using a more specific type thanAny.While
Anyworks, a more specific type likeUnion[str, int, float]would provide better type safety.Also applies to: 547-547, 610-610
404-405: Remove outdated comment about error_lines.Comment references removed error handling logic.
- # Check for any errors. Typically, these are in error_lines, but also the results can - # contain the PRIMER_ERROR key. + # Check for any errors in the results
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
prymer/primer3/primer3.py(9 hunks)
🔇 Additional comments (2)
prymer/primer3/primer3.py (2)
213-213: LGTM: Clean context manager implementation.Proper resource management with context manager protocol.
Also applies to: 245-256, 258-259
384-392: LGTM: Clean refactoring to use primer3 module.Efficient separation of sequence and global arguments.
No description provided.