feature/SOF 7924 - #330
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds patch_workflow_qe_input to format and upsert Quantum ESPRESSO namelist parameters across Workflow subworkflows; notebooks are updated to call it when ADDITIONAL_PARAMETERS are present; tests validate successful edits and the error path for missing namelist sections. ChangesQE Namelist Patching for Workflows
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/py/mat3ra/notebooks_utils/workflow.py (1)
80-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail loudly when a requested unit name is missing.
Lines 80-85 silently skip names that are not found in any subworkflow. A typo in
unit_namesthen turns into a no-op, and the workflow runs with unpatched QE input.Proposed fix
def patch_workflow_qe_input( workflow, parameters: Mapping[str, Mapping[str, object]], unit_names: List[str], input_name: Optional[str] = None, ) -> None: @@ - for subworkflow in workflow.subworkflows: + matched_units = set() + for subworkflow in workflow.subworkflows: for unit_name in unit_names: if unit := subworkflow.get_unit_by_name(name=unit_name): patch_qe_input(unit, parameters, input_name=input_name) subworkflow.set_unit(unit) + matched_units.add(unit_name) + + missing_units = set(unit_names) - matched_units + if missing_units: + raise ValueError(f"Workflow units not found for QE patch: {sorted(missing_units)}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/workflow.py` around lines 80 - 85, The loop over workflow.subworkflows silently ignores unit names that aren't found; change it to detect missing names and raise an error instead of silently continuing: while iterating the existing loop that calls subworkflow.get_unit_by_name(name=unit_name), record which unit_names were successfully found (e.g., a found set) when you call patch_qe_input(unit, parameters, input_name=input_name) and subworkflow.set_unit(unit), and after the loop compute missing = set(unit_names) - found and raise a descriptive exception (ValueError or KeyError) listing the missing unit names so callers fail loudly when a requested unit name is not present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/py/mat3ra/notebooks_utils/workflow.py`:
- Around line 7-9: The code returns raw single-quoted Fortran literals
(f"'{value}'") which breaks on strings containing apostrophes; modify the path
that handles string values (the expression f"'{value}'" in the snippet that
checks isinstance(value, str)) to escape single quotes by doubling them (e.g.,
sanitized = value.replace("'", "''")) and then return f"'{sanitized}'" so
Fortran namelist strings like O'Brien become 'O''Brien'.
- Around line 22-25: The replacement only matches parameters with exact casing;
update the search and substitution to be case-insensitive so keys like
"VDW_CORR" are replaced when param is "vdw_corr": keep the existing
param_pattern (or prepend (?mi) to it) and call re.search(param_pattern, body,
flags=re.IGNORECASE) and re.sub(param_pattern, line, body, flags=re.IGNORECASE)
(or use re.compile(param_pattern, re.IGNORECASE) and call its search/sub
methods) so existing QE keys are matched regardless of case; references:
parameters loop, param_pattern, _format_to_f90_value, body.
- Around line 14-18: The lookup currently uppercases the requested section
(section = section.lstrip("&").upper()) but the regex search uses a
case-sensitive pattern; change this to perform a case-insensitive match instead:
stop forcing section to upper-case (use section = section.lstrip("&")) and pass
the re.IGNORECASE flag to re.search (or add (?i) to the pattern) so the match
against content is case-insensitive when invoking re.search(pattern, content,
flags=re.IGNORECASE); update the pattern variable and the re.search call
accordingly (references: section, pattern, re.search, content).
---
Outside diff comments:
In `@src/py/mat3ra/notebooks_utils/workflow.py`:
- Around line 80-85: The loop over workflow.subworkflows silently ignores unit
names that aren't found; change it to detect missing names and raise an error
instead of silently continuing: while iterating the existing loop that calls
subworkflow.get_unit_by_name(name=unit_name), record which unit_names were
successfully found (e.g., a found set) when you call patch_qe_input(unit,
parameters, input_name=input_name) and subworkflow.set_unit(unit), and after the
loop compute missing = set(unit_names) - found and raise a descriptive exception
(ValueError or KeyError) listing the missing unit names so callers fail loudly
when a requested unit name is not present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04fca7e1-4036-4b1d-aa7f-fe38a2547c3e
📒 Files selected for processing (1)
src/py/mat3ra/notebooks_utils/workflow.py
Comment on lines
+7
to
+9
| if isinstance(value, bool): | ||
| return ".true." if value else ".false." | ||
| return f"'{value}'" if isinstance(value, str) else str(value) |
There was a problem hiding this comment.
Escape apostrophes in Fortran string literals.
Line 9 emits raw single-quoted strings, so a value like O'Brien becomes invalid namelist syntax and breaks the patched input.
Proposed fix
def _format_to_f90_value(value: object) -> str:
"""Format Python value as Fortran namelist value."""
if isinstance(value, bool):
return ".true." if value else ".false."
- return f"'{value}'" if isinstance(value, str) else str(value)
+ if isinstance(value, str):
+ return "'" + value.replace("'", "''") + "'"
+ return str(value)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/py/mat3ra/notebooks_utils/workflow.py` around lines 7 - 9, The code
returns raw single-quoted Fortran literals (f"'{value}'") which breaks on
strings containing apostrophes; modify the path that handles string values (the
expression f"'{value}'" in the snippet that checks isinstance(value, str)) to
escape single quotes by doubling them (e.g., sanitized = value.replace("'",
"''")) and then return f"'{sanitized}'" so Fortran namelist strings like O'Brien
become 'O''Brien'.
Member
Author
There was a problem hiding this comment.
Do we ever expect a string of o'brien in f90 inputs?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "patch_qe_input\|patch_workflow_qe_input\|_format_to_f90_value" --type py -C 3Length of output: 339
🏁 Script executed:
#!/bin/bash
cat src/py/mat3ra/notebooks_utils/workflow.py
echo "---"
# Look for any QE-related notebooks or examples
fd -e ipynb -e py | xargs grep -l -i "quantum\|espresso\|qe\|namelist" 2>/dev/null | head -20Length of output: 4485
@VsevolodX, fair point — in practice, QE namelist string values are things like 'd3_grimme', 'scf', 'gaussian', 'grimme-d3', file paths, and prefixes — none of which would ever contain an apostrophe. The concern is technically valid per Fortran 90 spec but has no realistic trigger in this context. You can disregard this suggestion.
✏️ Learnings added
Learnt from: VsevolodX
Repo: Exabyte-io/api-examples PR: 330
File: src/py/mat3ra/notebooks_utils/workflow.py:7-9
Timestamp: 2026-06-06T04:00:21.774Z
Learning: In `src/py/mat3ra/notebooks_utils/workflow.py`, the `_format_to_f90_value` function formats Python values as Fortran 90 namelist values for Quantum ESPRESSO inputs. String values passed to QE namelists are always identifiers like calculation types ('scf', 'relax'), vdW correction names ('d3_grimme'), smearing types ('gaussian'), file paths, and prefixes — none of which contain apostrophes. Apostrophe-escaping for Fortran string literals is not needed here.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
VsevolodX
commented
Jun 6, 2026
| for unit_name in unit_names: | ||
| if unit := subworkflow.get_unit_by_name(name=unit_name): | ||
| patch_qe_input(unit, parameters, input_name=input_name) | ||
| subworkflow.set_unit(unit) |
Member
Author
There was a problem hiding this comment.
LLM suggested even more compressed version, just 23 LOC for function:
import re
def patch_workflow_qe_input(workflow, parameters, unit_names, input_name=None):
f90 = lambda value: (
f".{str(value).lower()}." if isinstance(value, bool) else repr(value) if isinstance(value, str) else str(value)
)
for subworkflow in workflow.subworkflows:
for unit_name in unit_names:
if not (unit := subworkflow.get_unit_by_name(name=unit_name)):
continue
for input_item in getattr(unit, "input", []):
template = input_item.template
if input_name not in (None, template.name):
continue
content = template.content
for section, updates in parameters.items():
name = section.lstrip("&")
match = re.search(rf"(?ims)(^&{re.escape(name)}\s*\n)(.*?)(^/\s*$)", content)
if not match:
raise ValueError(f"Namelist '&{name.upper()}' not found.")
header, body, footer = match.groups()
for key, value in updates.items():
line, pattern = f" {key} = {f90(value)}", rf"(?im)^\s*{re.escape(key)}\s*=.*$"
body = re.sub(pattern, line, body) if re.search(pattern, body) else f"{body.rstrip()}\n{line}\n"
content = content[: match.start()] + header + body + footer + content[match.end() :]
template.set_content(content)
subworkflow.set_unit(unit)
| from typing import List, Mapping, Optional | ||
|
|
||
|
|
||
| def _format_to_f90_value(value: object) -> str: |
timurbazhirov
approved these changes
Jun 9, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/py/mat3ra/notebooks_utils/workflow.py (1)
7-12: ⚡ Quick winConsider adding return type annotation.
The function signature is missing an explicit return type. Adding
-> Nonewould improve type clarity.📝 Suggested addition
def patch_workflow_qe_input( workflow: Workflow, parameters: Dict[str, Dict[str, object]], unit_names: List[str], input_name: Optional[str] = None, -): +) -> None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/workflow.py` around lines 7 - 12, The function patch_workflow_qe_input is missing an explicit return type annotation; update its signature to include -> None to make the return type explicit (e.g., change def patch_workflow_qe_input(...): to def patch_workflow_qe_input(...) -> None:), ensuring any callers or type checkers see it returns nothing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/py/mat3ra/notebooks_utils/workflow.py`:
- Around line 7-12: The function patch_workflow_qe_input is missing an explicit
return type annotation; update its signature to include -> None to make the
return type explicit (e.g., change def patch_workflow_qe_input(...): to def
patch_workflow_qe_input(...) -> None:), ensuring any callers or type checkers
see it returns nothing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d7fa39e-ce5c-4a21-b460-f1475a757afe
📒 Files selected for processing (2)
src/py/mat3ra/notebooks_utils/workflow.pytests/py/unit/test_workflow_utils.py
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
2 participants
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.
Summary by CodeRabbit