Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Jupyter notebook that runs an end-to-end Mat3ra DFT valence band offset (VBO) workflow and updates job-creation logic to read Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Notebook
participant AuthService
participant APIClient
participant Standata
participant JobService
User->>Notebook: open/run notebook with params
Notebook->>AuthService: start OIDC/browser auth
AuthService-->>Notebook: return token/session
Notebook->>APIClient: init with token
Notebook->>APIClient: load interface (uploads)
APIClient-->>Notebook: return interface data
Notebook->>Notebook: extract left/right parts, prepare materials
Notebook->>APIClient: save materials (interface, left, right)
APIClient-->>Notebook: return material IDs
Notebook->>Standata: search & load VBO workflow/template
Standata-->>Notebook: return workflow definition
Notebook->>Notebook: assign DFT model, edit pw_scf/pw_bands inputs
Notebook->>JobService: build compute config & create job (materials + workflow)
JobService-->>Notebook: return job ID
Notebook->>JobService: poll job status
JobService-->>Notebook: job completed
Notebook->>APIClient: fetch `valence_band_offset` and other properties
APIClient-->>Notebook: return results
Notebook->>User: display VBO value and visualizations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.9)other/materials_designer/workflows/Introduction.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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@other/materials_designer/workflows/valence_band_offset.ipynb`:
- Around line 240-242: The code assumes client.projects.list(...) returns a
non-empty list and directly indexes projects[0], causing IndexError when no
default project exists; update the logic around projects (result of
client.projects.list) to check for an empty list before accessing projects[0],
e.g., if projects is empty raise a clear RuntimeError or print a user-friendly
message and exit, otherwise set project_id = projects[0]["_id"] and proceed;
reference the client.projects.list call and the project_id / projects[0] usage
when making this change.
- Around line 631-632: The code assumes client.properties.get_for_job(job_id,
property_name=PropertyName.scalar.valence_band_offset.value) returns a non-empty
list and directly accesses [0], causing crashes when the VBO property is
missing; update the block around client.properties.get_for_job and the vbo_value
usage to check the returned list (or None) before indexing, handle the empty
case by logging or printing a clear "VBO not available" message or fallback, and
only format/print vbo_value['value'] when vbo_value exists and contains the
'value' key.
- Around line 518-522: The cluster selection logic using CLUSTER_NAME can yield
cluster = None or raise on clusters[0]; before constructing the Compute object
(or any downstream use), validate that clusters is non-empty and that cluster is
not None: if CLUSTER_NAME is set and no matching hostname is found, raise a
clear error or fallback to a safe default; if CLUSTER_NAME is not set and
clusters is empty, raise a descriptive exception instead of indexing
clusters[0]. Ensure the check is placed immediately after the cluster assignment
and reference the variables cluster, clusters, and CLUSTER_NAME so the flow
prevents passing None into Compute creation.
- Around line 150-156: Remove the hardcoded credential assigned to
OIDC_ACCESS_TOKEN and replace it with a secure retrieval mechanism: stop setting
os.environ["OIDC_ACCESS_TOKEN"] to a literal string, instead read the token from
a non-committed secret source (e.g., environment provided at runtime, a .env
loaded by python-dotenv, or a secrets manager) and fail fast with a clear error
if the token is missing; update the initialization code around
os.environ["OIDC_ACCESS_TOKEN"] (and any related test/demo scaffolding) so no
plain-text token is stored in the notebook or repository and ensure secrets are
documented to be injected at runtime or via CI secret variables.
🪄 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: aca9d91e-9d0b-4548-9900-89348b84142f
📒 Files selected for processing (2)
other/materials_designer/workflows/valence_band_offset.ipynbutils/api.py
Comment on lines
+240
to
+242
| "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.
Guard default-project lookup before indexing.
This crashes with IndexError when no default project exists for the selected account.
🛠 Suggested patch
projects = client.projects.list({"isDefault": True, "owner._id": ACCOUNT_ID})
+if not projects:
+ raise RuntimeError(f"No default project found for account {ACCOUNT_ID}")
project_id = projects[0]["_id"]
print(f"✅ Using project: {projects[0]['name']} ({project_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(f\"No default project found for account {ACCOUNT_ID}\")\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 the current code and only fix it if needed.
In `@other/materials_designer/workflows/valence_band_offset.ipynb` around lines
240 - 242, The code assumes client.projects.list(...) returns a non-empty list
and directly indexes projects[0], causing IndexError when no default project
exists; update the logic around projects (result of client.projects.list) to
check for an empty list before accessing projects[0], e.g., if projects is empty
raise a clear RuntimeError or print a user-friendly message and exit, otherwise
set project_id = projects[0]["_id"] and proceed; reference the
client.projects.list call and the project_id / projects[0] usage when making
this change.
Comment on lines
+518
to
+522
| "if CLUSTER_NAME:\n", | ||
| " cluster = next((c for c in clusters if CLUSTER_NAME in c[\"hostname\"]), None)\n", | ||
| "else:\n", | ||
| " cluster = clusters[0]\n", | ||
| "\n", |
There was a problem hiding this comment.
Validate cluster selection result before creating Compute.
If CLUSTER_NAME does not match any hostname (or clusters is empty), this can pass None downstream or raise on clusters[0].
🛠 Suggested patch
if CLUSTER_NAME:
cluster = next((c for c in clusters if CLUSTER_NAME in c["hostname"]), None)
else:
- cluster = clusters[0]
+ cluster = clusters[0] if clusters else None
+
+if cluster is None:
+ available = [c["hostname"] for c in clusters]
+ raise RuntimeError(f"Requested cluster '{CLUSTER_NAME}' not found. Available: {available}")📝 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
| "if CLUSTER_NAME:\n", | |
| " cluster = next((c for c in clusters if CLUSTER_NAME in c[\"hostname\"]), None)\n", | |
| "else:\n", | |
| " cluster = clusters[0]\n", | |
| "\n", | |
| "if CLUSTER_NAME:\n", | |
| " cluster = next((c for c in clusters if CLUSTER_NAME in c[\"hostname\"]), None)\n", | |
| "else:\n", | |
| " cluster = clusters[0] if clusters else None\n", | |
| "\n", | |
| "if cluster is None:\n", | |
| " available = [c[\"hostname\"] for c in clusters]\n", | |
| " raise RuntimeError(f\"Requested cluster '{CLUSTER_NAME}' not found. Available: {available}\")\n", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@other/materials_designer/workflows/valence_band_offset.ipynb` around lines
518 - 522, The cluster selection logic using CLUSTER_NAME can yield cluster =
None or raise on clusters[0]; before constructing the Compute object (or any
downstream use), validate that clusters is non-empty and that cluster is not
None: if CLUSTER_NAME is set and no matching hostname is found, raise a clear
error or fallback to a safe default; if CLUSTER_NAME is not set and clusters is
empty, raise a descriptive exception instead of indexing clusters[0]. Ensure the
check is placed immediately after the cluster assignment and reference the
variables cluster, clusters, and CLUSTER_NAME so the flow prevents passing None
into Compute creation.
Comment on lines
+631
to
+632
| "vbo_value = client.properties.get_for_job(job_id, property_name=PropertyName.scalar.valence_band_offset.value)[0]\n", | ||
| "print(f\"Valence Band Offset (VBO) value: {vbo_value['value']:.3f} eV\")\n", |
There was a problem hiding this comment.
Handle missing VBO property defensively.
Direct [0] indexing fails when the property is absent (e.g., failed/partial workflow), causing a late crash.
🛠 Suggested patch
-vbo_value = client.properties.get_for_job(job_id, property_name=PropertyName.scalar.valence_band_offset.value)[0]
+vbo_results = client.properties.get_for_job(
+ job_id, property_name=PropertyName.scalar.valence_band_offset.value
+)
+if not vbo_results:
+ raise RuntimeError(f"No valence_band_offset property found for job {job_id}")
+vbo_value = vbo_results[0]
print(f"Valence Band Offset (VBO) value: {vbo_value['value']:.3f} eV")📝 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
| "vbo_value = client.properties.get_for_job(job_id, property_name=PropertyName.scalar.valence_band_offset.value)[0]\n", | |
| "print(f\"Valence Band Offset (VBO) value: {vbo_value['value']:.3f} eV\")\n", | |
| "vbo_results = client.properties.get_for_job(\n", | |
| " job_id, property_name=PropertyName.scalar.valence_band_offset.value\n", | |
| ")\n", | |
| "if not vbo_results:\n", | |
| " raise RuntimeError(f\"No valence_band_offset property found for job {job_id}\")\n", | |
| "vbo_value = vbo_results[0]\n", | |
| "print(f\"Valence Band Offset (VBO) value: {vbo_value['value']:.3f} eV\")\n", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@other/materials_designer/workflows/valence_band_offset.ipynb` around lines
631 - 632, The code assumes client.properties.get_for_job(job_id,
property_name=PropertyName.scalar.valence_band_offset.value) returns a non-empty
list and directly accesses [0], causing crashes when the VBO property is
missing; update the block around client.properties.get_for_job and the vbo_value
usage to check the returned list (or None) before indexing, handle the empty
case by logging or printing a clear "VBO not available" message or fallback, and
only format/print vbo_value['value'] when vbo_value exists and contains the
'value' key.
There was a problem hiding this comment.
♻️ Duplicate comments (4)
other/materials_designer/workflows/valence_band_offset.ipynb (4)
244-246:⚠️ Potential issue | 🟠 MajorGuard default-project lookup before indexing.
Line 245 assumes
projectsis non-empty; this raisesIndexErrorwhen no default project is returned.🛠 Suggested fix
projects = client.projects.list({"isDefault": True, "owner._id": ACCOUNT_ID}) +if not projects: + raise RuntimeError(f"No default project found for account {ACCOUNT_ID}") project_id = projects[0]["_id"] print(f"✅ Using project: {projects[0]['name']} ({project_id})")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@other/materials_designer/workflows/valence_band_offset.ipynb` around lines 244 - 246, The code assumes projects is non-empty when assigning project_id and printing projects[0]; add a guard to check whether the returned projects list is empty before indexing. In the block around the projects = client.projects.list(...) call, verify len(projects) (or truthiness of projects) and handle the empty case (e.g., raise a clear error, log a message, or create/fallback to a project) instead of using projects[0]; only set project_id and call print(f"...{projects[0]['name']}...") after confirming projects contains at least one element.
535-545:⚠️ Potential issue | 🟠 MajorValidate cluster selection before constructing
Compute.Line 538 can fail when
clustersis empty, and line 541 can receiveNoneifCLUSTER_NAMEdoesn’t match any hostname.🛠 Suggested fix
if CLUSTER_NAME: cluster = next((c for c in clusters if CLUSTER_NAME in c["hostname"]), None) else: - cluster = clusters[0] + cluster = clusters[0] if clusters else None + +if cluster is None: + available = [c["hostname"] for c in clusters] + raise RuntimeError( + f"Requested cluster '{CLUSTER_NAME}' not found or no clusters available. Available: {available}" + ) compute = Compute(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@other/materials_designer/workflows/valence_band_offset.ipynb` around lines 535 - 545, The cluster selection code assumes `clusters` has at least one entry and that a match for `CLUSTER_NAME` will be found; validate both before constructing `Compute`: first check `clusters` is non-empty and raise or exit with a clear message if empty, then if `CLUSTER_NAME` is set ensure `cluster = next(...)` returned a non-None value (or fall back to a safe default) and raise/log an informative error if not, only then pass `cluster` into the `Compute(...)` constructor and the subsequent print.
648-649:⚠️ Potential issue | 🟠 MajorHandle missing VBO property defensively before
[0]access.Line 648 assumes
get_for_job(...)returns a non-empty list and that the first item containsvalue.🛠 Suggested fix
-vbo_value = client.properties.get_for_job(job_id, property_name=PropertyName.scalar.valence_band_offset.value)[0] -print(f"Valence Band Offset (VBO) value: {vbo_value['value']:.3f} eV") +vbo_results = client.properties.get_for_job( + job_id, property_name=PropertyName.scalar.valence_band_offset.value +) +if not vbo_results: + raise RuntimeError(f"No valence_band_offset property found for job {job_id}") +vbo_value = vbo_results[0] +if "value" not in vbo_value: + raise RuntimeError(f"valence_band_offset property for job {job_id} has no 'value' field") +print(f"Valence Band Offset (VBO) value: {vbo_value['value']:.3f} eV")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@other/materials_designer/workflows/valence_band_offset.ipynb` around lines 648 - 649, The code assumes client.properties.get_for_job(job_id, property_name=PropertyName.scalar.valence_band_offset.value) returns a non-empty list and directly indexes [0]; update the logic in the block using client.properties.get_for_job and vbo_value to first capture the returned list, check if it's empty (and handle that case by logging, raising a clear error, or setting a default), then safely access the first item and its 'value' (use a defensive lookup like .get('value') and validate it's numeric) before formatting the print message; reference PropertyName.scalar.valence_band_offset and the vbo_value variable when making these changes.
154-160:⚠️ Potential issue | 🔴 CriticalRemove the hardcoded OIDC token from source immediately.
Line 159 commits a credential literal, which is a secret exposure risk and must not live in versioned notebooks.
🔒 Suggested fix
import os -os.environ["API_PORT"] = "3000" -os.environ["API_SECURE"] = "false" -os.environ["API_HOST"] = "localhost" -os.environ["OIDC_ACCESS_TOKEN"] = "EfDmUd-Q_J7Sk-LJYGUyImTR-hJHmH2OzSJjciD-Aju" +os.environ.setdefault("API_PORT", "3000") +os.environ.setdefault("API_SECURE", "false") +os.environ.setdefault("API_HOST", "localhost") +if "OIDC_ACCESS_TOKEN" not in os.environ: + raise RuntimeError("OIDC_ACCESS_TOKEN is not set. Authenticate first.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@other/materials_designer/workflows/valence_band_offset.ipynb` around lines 154 - 160, Remove the hardcoded credential assigned to os.environ["OIDC_ACCESS_TOKEN"] in the notebook and instead read the token from a secure source (e.g., os.environ.get("OIDC_ACCESS_TOKEN") populated outside the repo, a secrets manager, or a CI/CD secret), delete the committed literal from the notebook history (rotate/revoke the exposed token), and ensure any local example uses a placeholder or fetch function rather than embedding the real token.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@other/materials_designer/workflows/valence_band_offset.ipynb`:
- Around line 244-246: The code assumes projects is non-empty when assigning
project_id and printing projects[0]; add a guard to check whether the returned
projects list is empty before indexing. In the block around the projects =
client.projects.list(...) call, verify len(projects) (or truthiness of projects)
and handle the empty case (e.g., raise a clear error, log a message, or
create/fallback to a project) instead of using projects[0]; only set project_id
and call print(f"...{projects[0]['name']}...") after confirming projects
contains at least one element.
- Around line 535-545: The cluster selection code assumes `clusters` has at
least one entry and that a match for `CLUSTER_NAME` will be found; validate both
before constructing `Compute`: first check `clusters` is non-empty and raise or
exit with a clear message if empty, then if `CLUSTER_NAME` is set ensure
`cluster = next(...)` returned a non-None value (or fall back to a safe default)
and raise/log an informative error if not, only then pass `cluster` into the
`Compute(...)` constructor and the subsequent print.
- Around line 648-649: The code assumes client.properties.get_for_job(job_id,
property_name=PropertyName.scalar.valence_band_offset.value) returns a non-empty
list and directly indexes [0]; update the logic in the block using
client.properties.get_for_job and vbo_value to first capture the returned list,
check if it's empty (and handle that case by logging, raising a clear error, or
setting a default), then safely access the first item and its 'value' (use a
defensive lookup like .get('value') and validate it's numeric) before formatting
the print message; reference PropertyName.scalar.valence_band_offset and the
vbo_value variable when making these changes.
- Around line 154-160: Remove the hardcoded credential assigned to
os.environ["OIDC_ACCESS_TOKEN"] in the notebook and instead read the token from
a secure source (e.g., os.environ.get("OIDC_ACCESS_TOKEN") populated outside the
repo, a secrets manager, or a CI/CD secret), delete the committed literal from
the notebook history (rotate/revoke the exposed token), and ensure any local
example uses a placeholder or fetch function rather than embedding the real
token.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46bc50c6-e4cd-40e0-b7f8-74690a3a9530
📒 Files selected for processing (1)
other/materials_designer/workflows/valence_band_offset.ipynb
| @@ -0,0 +1,654 @@ | |||
| { | |||
Member
There was a problem hiding this comment.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
other/materials_designer/workflows/Introduction.ipynb (1)
102-108: Consider removing the trailing empty code cell.Line 103 introduces an empty executable cell with no content or outputs. If it’s not intentionally reserved, removing it keeps the notebook cleaner for users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@other/materials_designer/workflows/Introduction.ipynb` around lines 102 - 108, Remove the trailing empty executable notebook cell (cell id "1") that has an empty "source" and no outputs; locate the code cell object with "cell_type": "code" and an empty "source" array and delete that cell entry from the notebook JSON to keep the notebook tidy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@other/materials_designer/workflows/Introduction.ipynb`:
- Around line 102-108: Remove the trailing empty executable notebook cell (cell
id "1") that has an empty "source" and no outputs; locate the code cell object
with "cell_type": "code" and an empty "source" array and delete that cell entry
from the notebook JSON to keep the notebook tidy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19f6a328-9ece-48ea-9eaf-78cd8f547b47
📒 Files selected for processing (1)
other/materials_designer/workflows/Introduction.ipynb
timurbazhirov
approved these changes
Apr 11, 2026
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
New Features
Bug Fixes
Documentation