feature/SOF-7953 FE NB#343
Open
VsevolodX wants to merge 7 commits into
Open
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
📝 WalkthroughWalkthroughAdds a new ChangesFormation Energy Workflow Notebook
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Notebook
participant APIClient
participant Standata
participant Cluster
Notebook->>APIClient: authenticate via OIDC
Notebook->>Standata: load compound material
Notebook->>Standata: resolve elemental reference materials
Notebook->>APIClient: fetch total_energy properties
Notebook->>Standata: load workflow template
Notebook->>Cluster: list and select compute cluster
Notebook->>APIClient: create and submit job
Notebook->>APIClient: wait for completion
Notebook->>APIClient: retrieve job properties
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Ruff (0.15.20)other/materials_designer/workflows/formation_energy.ipynbUnexpected end of JSON input 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: 5
🤖 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 `@other/materials_designer/workflows/formation_energy.ipynb`:
- Line 81: The notebook’s default input in the Formation Energy workflow is
currently a single-element example, which makes the “Run All” path misleading.
Update the MATERIAL_NAME default in formation_energy.ipynb to a known compound,
and add a guard in the workflow setup that checks the parsed elements list and
fails fast when len(elements) < 2. Use the notebook’s material-loading and
element-parsing logic to place the check near where MATERIAL_NAME is consumed so
the example only runs for valid compounds.
- Around line 326-333: The total-energy validation in the formation_energy
workflow only checks for the presence of a property holder, so holders without a
usable data.value slip through and later indexing fails. Update the holder
validation around the properties.list lookup and the downstream checks in the
reporting paths to require that holder["data"]["value"] exists and is non-empty
before treating it as valid, using the existing holders_by_symbol and holder
access patterns to locate the affected logic.
- Around line 221-230: The fallback in the material-loading block is indexing
material_matches[0] without checking whether the platform query returned
anything, which can crash when no match exists. Update the try/except flow
around Material.create and client.materials.list to verify material_matches is
non-empty before using the first result, and if it is empty, raise or return a
clear “not found” error that preserves the Standata lookup context instead of
failing with an index error.
- Around line 191-193: The project selection in the notebook assumes
`projects[0]` always exists, but `client.projects.list(...)` can return an empty
list for accounts without a default project. Update the selection logic around
the `projects` lookup and `project_id` assignment to check for an empty result
first, and fail early with a clear, actionable message before indexing into the
list.
- Around line 282-286: In the elemental reference lookup that builds
element_materials from client.bank_materials.list(), the current comprehension
silently overwrites duplicate metadata.element entries and makes the chosen
material depend on API order. Update this logic to either raise an error when
more than one material shares the same metadata.element or apply a clearly
deterministic tie-breaker before selecting one, so the reference selection is
unambiguous and stable.
🪄 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: 51f97a21-b578-443b-a95c-e6bf6097538b
📒 Files selected for processing (2)
other/materials_designer/workflows/Introduction.ipynbother/materials_designer/workflows/formation_energy.ipynb
Comment on lines
+191
to
+193
| "projects = client.projects.list({\"isDefault\": True, \"owner._id\": ACCOUNT_ID})\n", | ||
| "project_id = projects[0][\"_id\"]\n", | ||
| "print(f\"✅ Using project: {projects[0]['name']} ({project_id})\")\n" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle accounts without a default project.
projects[0] raises IndexError when the selected account/org has no default project. Fail with an actionable message before indexing.
Proposed fix
projects = client.projects.list({"isDefault": True, "owner._id": ACCOUNT_ID})
+if not projects:
+ raise RuntimeError(
+ f"No default project found for account {selected_account.name} ({ACCOUNT_ID}). "
+ "Create/select a project before running this notebook."
+ )
project_id = projects[0]["_id"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "projects = client.projects.list({\"isDefault\": True, \"owner._id\": ACCOUNT_ID})\n", | |
| "project_id = projects[0][\"_id\"]\n", | |
| "print(f\"✅ Using project: {projects[0]['name']} ({project_id})\")\n" | |
| "projects = client.projects.list({\"isDefault\": True, \"owner._id\": ACCOUNT_ID})\n", | |
| "if not projects:\n", | |
| " raise RuntimeError(\n", | |
| " f\"No default project found for account {selected_account.name} ({ACCOUNT_ID}). \"\n", | |
| " \"Create/select a project before running this notebook.\"\n", | |
| " )\n", | |
| "project_id = projects[0][\"_id\"]\n", | |
| "print(f\"✅ Using project: {projects[0]['name']} ({project_id})\")\n" |
🤖 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 `@other/materials_designer/workflows/formation_energy.ipynb` around lines 191 -
193, The project selection in the notebook assumes `projects[0]` always exists,
but `client.projects.list(...)` can return an empty list for accounts without a
default project. Update the selection logic around the `projects` lookup and
`project_id` assignment to check for an empty result first, and fail early with
a clear, actionable message before indexing into the list.
Comment on lines
+221
to
+230
| " try:\n", | ||
| " material = Material.create(Materials.get_by_name_first_match(MATERIAL_NAME))\n", | ||
| " print(f\"✅ Loaded material from Standata: {material.name}\")\n", | ||
| " except Exception:\n", | ||
| " material_matches = client.materials.list({\n", | ||
| " \"name\": {\"$regex\": re.escape(MATERIAL_NAME), \"$options\": \"i\"},\n", | ||
| " })\n", | ||
| "\n", | ||
| " material = Material.create(material_matches[0])\n", | ||
| " print(f\"♻️ Loaded material from platform: {material_matches[0]['_id']}\")\n", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the platform fallback before indexing the first match.
If MATERIAL_NAME is mistyped or unavailable, material_matches[0] crashes and hides the Standata lookup context. Return a clear “not found” error.
Proposed fix
- except Exception:
+ except Exception as standata_error:
material_matches = client.materials.list({
"name": {"$regex": re.escape(MATERIAL_NAME), "$options": "i"},
})
+ if not material_matches:
+ raise RuntimeError(
+ f"Could not find material '{MATERIAL_NAME}' in uploads, Standata, or platform materials."
+ ) from standata_error
+
material = Material.create(material_matches[0])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| " try:\n", | |
| " material = Material.create(Materials.get_by_name_first_match(MATERIAL_NAME))\n", | |
| " print(f\"✅ Loaded material from Standata: {material.name}\")\n", | |
| " except Exception:\n", | |
| " material_matches = client.materials.list({\n", | |
| " \"name\": {\"$regex\": re.escape(MATERIAL_NAME), \"$options\": \"i\"},\n", | |
| " })\n", | |
| "\n", | |
| " material = Material.create(material_matches[0])\n", | |
| " print(f\"♻️ Loaded material from platform: {material_matches[0]['_id']}\")\n", | |
| " try:\n", | |
| " material = Material.create(Materials.get_by_name_first_match(MATERIAL_NAME))\n", | |
| " print(f\"✅ Loaded material from Standata: {material.name}\")\n", | |
| " except Exception as standata_error:\n", | |
| " material_matches = client.materials.list({\n", | |
| " \"name\": {\"$regex\": re.escape(MATERIAL_NAME), \"$options\": \"i\"},\n", | |
| " })\n", | |
| "\n", | |
| " if not material_matches:\n", | |
| " raise RuntimeError(\n", | |
| " f\"Could not find material '{MATERIAL_NAME}' in uploads, Standata, or platform materials.\"\n", | |
| " ) from standata_error\n", | |
| "\n", | |
| " material = Material.create(material_matches[0])\n", | |
| " print(f\"♻️ Loaded material from platform: {material_matches[0]['_id']}\")\n", |
🤖 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 `@other/materials_designer/workflows/formation_energy.ipynb` around lines 221 -
230, The fallback in the material-loading block is indexing material_matches[0]
without checking whether the platform query returned anything, which can crash
when no match exists. Update the try/except flow around Material.create and
client.materials.list to verify material_matches is non-empty before using the
first result, and if it is empty, raise or return a clear “not found” error that
preserves the Standata lookup context instead of failing with an index error.
Comment on lines
+326
to
+333
| " props = api_client.properties.list(\n", | ||
| " query={\n", | ||
| " \"exabyteId\": exabyte_id,\n", | ||
| " \"slug\": \"total_energy\",\n", | ||
| " },\n", | ||
| " projection={\"sort\": {\"precision.value\": -1}, \"limit\": 1},\n", | ||
| " )\n", | ||
| " holders_by_symbol[symbol] = props[0] if props else None\n", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Validate that the total-energy holder contains a usable value.
The missing-energy check only verifies that a property exists. A holder without data.value passes validation, then later reporting indexes holder['data']['value'] directly.
Proposed fix
- holders_by_symbol[symbol] = props[0] if props else None
+ holder = props[0] if props else None
+ value = (holder or {}).get("data", {}).get("value")
+ holders_by_symbol[symbol] = holder if value is not None else NoneAlso applies to: 353-359, 641-646
🤖 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 `@other/materials_designer/workflows/formation_energy.ipynb` around lines 326 -
333, The total-energy validation in the formation_energy workflow only checks
for the presence of a property holder, so holders without a usable data.value
slip through and later indexing fails. Update the holder validation around the
properties.list lookup and the downstream checks in the reporting paths to
require that holder["data"]["value"] exists and is non-empty before treating it
as valid, using the existing holders_by_symbol and holder access patterns to
locate the affected logic.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
other/materials_designer/workflows/formation_energy.ipynb (1)
522-527: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard cluster selection against empty list and no-match.
clusters[0]raisesIndexErrorwhen no clusters are available, and whenCLUSTER_NAMEmatches nothingclusterbecomesNone, soCompute(cluster=None)and the subsequentcompute.cluster.hostnameon line 528 fail with an opaque error. Fail fast with clear messages.Proposed fix
if CLUSTER_NAME: cluster = next((c for c in clusters if CLUSTER_NAME in c["hostname"]), None) + if cluster is None: + raise RuntimeError(f"No cluster matching '{CLUSTER_NAME}' found.") else: + if not clusters: + raise RuntimeError("No clusters available for the selected account.") cluster = clusters[0]🤖 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 `@other/materials_designer/workflows/formation_energy.ipynb` around lines 522 - 527, The cluster selection logic in the notebook cell needs explicit guards before creating Compute: handle the empty clusters case before using clusters[0], and when CLUSTER_NAME is set, verify the next(...) lookup actually returns a cluster instead of None. Update the selection around the cluster/Compute setup to fail fast with a clear error message when no clusters are available or no hostname match is found, so the later compute.cluster.hostname access does not surface an opaque failure.
🤖 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 `@other/materials_designer/workflows/formation_energy.ipynb`:
- Around line 333-346: The compound_group assignment in formation_energy.ipynb
is using eval with unrestricted builtins, unlike the other sandboxed eval calls.
Update the assign-compound-group lookup to use the same empty-builtins
restriction as query/projection, or replace eval with a literal-only parser if
the value is always a simple literal. Use the compound_group,
formation_energy_subworkflow.get_unit_by_name, and assign-compound-group symbols
to locate the change.
---
Outside diff comments:
In `@other/materials_designer/workflows/formation_energy.ipynb`:
- Around line 522-527: The cluster selection logic in the notebook cell needs
explicit guards before creating Compute: handle the empty clusters case before
using clusters[0], and when CLUSTER_NAME is set, verify the next(...) lookup
actually returns a cluster instead of None. Update the selection around the
cluster/Compute setup to fail fast with a clear error message when no clusters
are available or no hostname match is found, so the later
compute.cluster.hostname access does not surface an opaque failure.
🪄 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: 05ea1c5d-7aec-404d-9491-40b62b791b2d
📒 Files selected for processing (1)
other/materials_designer/workflows/formation_energy.ipynb
Comment on lines
+333
to
+346
| "compound_group = eval(formation_energy_subworkflow.get_unit_by_name(name=\"assign-compound-group\").value)\n", | ||
| "\n", | ||
| "owner_slug = selected_account.entity_cache.get(\"slug\", selected_account.name)\n", | ||
| "\n", | ||
| "elemental_total_energy_holders = {}\n", | ||
| "for symbol in elements:\n", | ||
| " query = eval(query_template, {\"__builtins__\": {}}, {\n", | ||
| " \"ELEMENT_MATERIAL_IDS_BY_SYMBOL\": element_material_ids_by_symbol,\n", | ||
| " \"CURRENT_ELEMENT\": symbol,\n", | ||
| " \"COMPOUND_GROUP\": compound_group,\n", | ||
| " \"JOB_OWNER_SLUG\": owner_slug,\n", | ||
| " \"ELEMENTAL_TE_SOURCE\": ELEMENTAL_TE_SOURCE,\n", | ||
| " })\n", | ||
| " projection = eval(projection_template, {\"__builtins__\": {}})\n", |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 'assign-compound-group' --glob '*.json' --glob '*.py'Repository: mat3ra/api-examples
Length of output: 157
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
import json, pathlib
path = pathlib.Path('other/materials_designer/workflows/formation_energy.ipynb')
nb = json.loads(path.read_text())
for i, cell in enumerate(nb.get('cells', [])):
src = ''.join(cell.get('source', []))
if 'assign-compound-group' in src or 'compound_group = eval' in src:
print(f'CELL {i} type={cell.get("cell_type")}')
print(src)
print('---')
PYRepository: mat3ra/api-examples
Length of output: 1990
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
import json, pathlib, re
path = pathlib.Path('other/materials_designer/workflows/formation_energy.ipynb')
nb = json.loads(path.read_text())
for i, cell in enumerate(nb.get('cells', [])):
src = ''.join(cell.get('source', []))
if re.search(r'assign-compound-group', src):
print(f'CELL {i} type={cell.get("cell_type")}')
print(src)
print('---')
PYRepository: mat3ra/api-examples
Length of output: 1990
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
import json, pathlib, re
path = pathlib.Path('other/materials_designer/workflows/formation_energy.ipynb')
nb = json.loads(path.read_text())
for idx, cell in enumerate(nb.get('cells', [])):
src = ''.join(cell.get('source', []))
if 'compound_group = eval' in src or 'assign-compound-group' in src:
print(f'CELL {idx} ({cell.get("cell_type")})')
print(src)
print('---')
PYRepository: mat3ra/api-examples
Length of output: 1987
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
import json, pathlib
path = pathlib.Path('other/materials_designer/workflows/formation_energy.ipynb')
nb = json.loads(path.read_text())
for idx, cell in enumerate(nb.get('cells', [])):
src = ''.join(cell.get('source', []))
if 'assign-compound-group' in src:
print(f'CELL {idx} ({cell.get("cell_type")})')
# Print only lines around the target for easier inspection
lines = src.splitlines()
for i, line in enumerate(lines, 1):
if 'assign-compound-group' in line or 'compound_group = eval' in line:
start = max(1, i - 3)
end = min(len(lines), i + 3)
for j in range(start, end + 1):
print(f'{j:04d}: {lines[j-1]}')
print('---')
PYRepository: mat3ra/api-examples
Length of output: 643
🏁 Script executed:
#!/bin/bash
rg -n -C 2 'assign-compound-group|compound_group' other/materials_designer/workflows/formation_energy.ipynb other/materials_designer/workflows -g '!*.png'Repository: mat3ra/api-examples
Length of output: 2800
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
import json, pathlib
path = pathlib.Path('other/materials_designer/workflows/formation_energy.ipynb')
nb = json.loads(path.read_text())
for idx, cell in enumerate(nb.get('cells', [])):
src = ''.join(cell.get('source', []))
if 'compound_group' in src:
print(f'CELL {idx} ({cell.get("cell_type")})')
print(src)
print('---')
PYRepository: mat3ra/api-examples
Length of output: 1987
Restrict the compound_group eval. assign-compound-group is executed with full builtins, unlike the other eval calls here. Use the same empty-builtins sandbox, or switch to a literal parser if this value is always a plain literal.
🤖 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 `@other/materials_designer/workflows/formation_energy.ipynb` around lines 333 -
346, The compound_group assignment in formation_energy.ipynb is using eval with
unrestricted builtins, unlike the other sandboxed eval calls. Update the
assign-compound-group lookup to use the same empty-builtins restriction as
query/projection, or replace eval with a literal-only parser if the value is
always a simple literal. Use the compound_group,
formation_energy_subworkflow.get_unit_by_name, and assign-compound-group symbols
to locate the change.
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.
Summary by CodeRabbit