update: add generic convergence NB - #292
Merged
Merged
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:
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 (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a new Jupyter notebook for configurable workflow parameter convergence, updates an existing convergence notebook to use new convergence-series keys and helper, updates the Introduction link, and adds a helper Changes
Sequence DiagramsequenceDiagram
actor User
participant Notebook as Convergence\nNotebook
participant API as APIClient
participant Standata as Standata\nService
participant Cluster as Cluster\nService
participant JobSvc as Job\nSystem
participant Poller as Job\nPoller
participant Results as Result\nProcessor
User->>Notebook: configure inputs (account, material, workflow, convergence, compute)
Notebook->>API: browser auth / init client
Notebook->>API: select account & project
Notebook->>API: load/save material (uploads or Standata fallback)
Notebook->>Standata: load workflow by app/name
Standata-->>Notebook: workflow definition
Notebook->>Standata: inject convergence parameter into subworkflow
Notebook->>Cluster: list & select cluster
Notebook->>JobSvc: create & submit job (project, material, modified workflow, compute)
JobSvc-->>Notebook: job_id
Notebook->>Poller: poll job status (interval)
Poller->>JobSvc: status checks
JobSvc-->>Poller: status updates
Poller-->>Notebook: job complete
Notebook->>Results: call get_convergence_series(client, job_id, subworkflow_index)
Results-->>Notebook: convergence series
Notebook->>User: plot series & expose converged value
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/convergence.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: 7
🤖 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/convergence.ipynb`:
- Around line 429-430: The code assumes scopeTrack and convergence points always
exist; fix by defensively validating finished_job.get("scopeTrack") and the
series returned by subworkflow.convergence_series before indexing; if
scope_track is missing or series is empty, skip indexing (or log a warning)
instead of doing series[-1]; apply the same checks to the other occurrence
around the later cell that also accesses series[-1] so you never index an empty
series.
- Around line 282-283: The code assumes
WorkflowStandata.filter_by_application(...).get_by_name_first_match(...) returns
a config and that Workflow.create(...) yields a workflow with at least one
subworkflow; add explicit checks after calling
WorkflowStandata.filter_by_application(...).get_by_name_first_match(WORKFLOW_SEARCH_TERM)
to verify the returned workflow_config is not None (or raise/return a clear
error), then call Workflow.create(workflow_config) and validate that workflow
and workflow.subworkflows exist and that workflow.subworkflows is non-empty
before accessing workflow.subworkflows[0]; update both occurrences (around the
current calls and the similar code near line 306) to handle missing config or
missing subworkflows with informative errors or fallback logic.
- Line 141: Remove the hard-coded credential-like token string found in the
commented assignment to os.environ["OIDC_ACCESS_TOKEN"] (the quoted token
literal in the notebook cell) and replace it with a non-secret placeholder or a
call that reads from a secure source (e.g., keep only
os.environ["OIDC_ACCESS_TOKEN"] without an inline value or use "<REDACTED>"
placeholder); also ensure you remove any other token-like literals in the same
notebook and, if this token was ever real, rotate/revoke it out-of-band and
scrub it from history using your repo's secret-removal process.
- Around line 338-344: The cluster selection logic can yield None or index into
an empty list; update the block that calls client.clusters.list() and sets
cluster (the code that references CLUSTER_NAME and the cluster variable used to
construct Compute) to explicitly handle no clusters and no match: after calling
client.clusters.list(), if the returned list is empty, raise/log an informative
error and abort; if CLUSTER_NAME is set but next(...) returns None, raise/log an
informative error listing available hostnames or fall back to a clear default
instead of passing None; ensure Compute(cluster=cluster, ...) is only executed
when cluster is a valid object.
- Around line 209-211: The code assumes projects[0] exists after calling
client.projects.list({"isDefault": True, "owner._id": ACCOUNT_ID}); add a guard
that checks the returned projects collection (from client.projects.list) is
non-empty before accessing projects[0], and if empty handle it (raise a clear
error, create a default project, or return early) and avoid assigning PROJECT_ID
from projects[0]["_id"] or printing projects[0]["name"] when no project exists;
update the logic around PROJECT_ID and the print statement to use the guarded
value.
- Around line 138-140: The notebook is unconditionally overriding environment
vars API_PORT, API_SECURE, and API_HOST via os.environ[...] assignments; change
these to conditional defaults so existing configs aren’t clobbered and don’t
force non-TLS: use os.environ.setdefault("API_PORT", "3000") and
os.environ.setdefault("API_HOST", "localhost") (or a config-aware alternative)
and for API_SECURE respect any existing value (e.g.,
os.environ.setdefault("API_SECURE", "true") or only set when explicitly running
in a local-dev mode), referencing the os.environ keys "API_PORT", "API_HOST",
and "API_SECURE" in your changes.
- Line 304: The notebook imports utils.workflow and calls
add_template_param_convergence which do not exist; fix by either adding a new
module utils.workflow with a properly named function
add_template_param_convergence(imported symbol) that implements the expected
behavior used by the notebook, or remove the import line and all calls to
add_template_param_convergence in the notebook; if you add the function, export
it with that exact name and ensure it has the same signature the notebook
expects and include tests or a minimal docstring so callers know how to use it.
🪄 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: e2191563-d271-416c-8838-6d2056d16dfa
📒 Files selected for processing (1)
other/materials_designer/workflows/convergence.ipynb
Comment on lines
+209
to
+211
| "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})\")" |
There was a problem hiding this comment.
Guard against empty default-project lookup before indexing.
projects[0] will raise if the account has no default project.
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})")📝 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})\")" | |
| 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/convergence.ipynb` around lines 209 - 211,
The code assumes projects[0] exists after calling
client.projects.list({"isDefault": True, "owner._id": ACCOUNT_ID}); add a guard
that checks the returned projects collection (from client.projects.list) is
non-empty before accessing projects[0], and if empty handle it (raise a clear
error, create a default project, or return early) and avoid assigning PROJECT_ID
from projects[0]["_id"] or printing projects[0]["name"] when no project exists;
update the logic around PROJECT_ID and the print statement to use the guarded
value.
Comment on lines
+282
to
+283
| "workflow_config = WorkflowStandata.filter_by_application(app.name).get_by_name_first_match(WORKFLOW_SEARCH_TERM)\n", | ||
| "workflow = Workflow.create(workflow_config)\n", |
There was a problem hiding this comment.
Validate workflow resolution and subworkflow existence before use.
Both get_by_name_first_match(...) and workflow.subworkflows[0] assume presence; either can fail with runtime errors for valid user inputs.
Suggested fix
workflow_config = WorkflowStandata.filter_by_application(app.name).get_by_name_first_match(WORKFLOW_SEARCH_TERM)
+if not workflow_config:
+ raise RuntimeError(
+ f"No workflow found for application={app.name}, search_term={WORKFLOW_SEARCH_TERM}"
+ )
workflow = Workflow.create(workflow_config)
workflow.name = f"{MY_WORKFLOW_NAME} {RUN_LABEL}"
@@
convergence_subworkflow = workflow.subworkflows[0]
+if not workflow.subworkflows:
+ raise RuntimeError("Selected workflow has no subworkflows to apply convergence on")
+convergence_subworkflow = workflow.subworkflows[0]Also applies to: 306-306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@other/materials_designer/workflows/convergence.ipynb` around lines 282 - 283,
The code assumes
WorkflowStandata.filter_by_application(...).get_by_name_first_match(...) returns
a config and that Workflow.create(...) yields a workflow with at least one
subworkflow; add explicit checks after calling
WorkflowStandata.filter_by_application(...).get_by_name_first_match(WORKFLOW_SEARCH_TERM)
to verify the returned workflow_config is not None (or raise/return a clear
error), then call Workflow.create(workflow_config) and validate that workflow
and workflow.subworkflows exist and that workflow.subworkflows is non-empty
before accessing workflow.subworkflows[0]; update both occurrences (around the
current calls and the similar code near line 306) to handle missing config or
missing subworkflows with informative errors or fallback logic.
Comment on lines
+338
to
+344
| "clusters = client.clusters.list()\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]\n", | ||
| "\n", | ||
| "compute = Compute(cluster=cluster, queue=QUEUE_NAME, ppn=PPN)\n", |
There was a problem hiding this comment.
Handle empty cluster lists and unmatched cluster filters explicitly.
Current logic can pass None (or index into an empty list), causing later compute/job construction failures.
Suggested fix
clusters = client.clusters.list()
+if not clusters:
+ raise RuntimeError("No compute clusters available for this account")
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 matches CLUSTER_NAME='{CLUSTER_NAME}'")
else:
cluster = clusters[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
| "clusters = client.clusters.list()\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]\n", | |
| "\n", | |
| "compute = Compute(cluster=cluster, queue=QUEUE_NAME, ppn=PPN)\n", | |
| clusters = client.clusters.list() | |
| if not clusters: | |
| raise RuntimeError("No compute clusters available for this account") | |
| 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 matches CLUSTER_NAME='{CLUSTER_NAME}'") | |
| else: | |
| cluster = clusters[0] | |
| compute = Compute(cluster=cluster, queue=QUEUE_NAME, ppn=PPN) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@other/materials_designer/workflows/convergence.ipynb` around lines 338 - 344,
The cluster selection logic can yield None or index into an empty list; update
the block that calls client.clusters.list() and sets cluster (the code that
references CLUSTER_NAME and the cluster variable used to construct Compute) to
explicitly handle no clusters and no match: after calling
client.clusters.list(), if the returned list is empty, raise/log an informative
error and abort; if CLUSTER_NAME is set but next(...) returns None, raise/log an
informative error listing available hostnames or fall back to a clear default
instead of passing None; ensure Compute(cluster=cluster, ...) is only executed
when cluster is a valid object.
Comment on lines
+429
to
+430
| "scope_track = finished_job.get(\"scopeTrack\")\n", | ||
| "series = subworkflow.convergence_series(scope_track)\n", |
There was a problem hiding this comment.
Defensively validate convergence data before indexing.
scopeTrack can be missing and series[-1] crashes when no convergence points are produced.
Suggested fix
scope_track = finished_job.get("scopeTrack")
+if not scope_track:
+ raise RuntimeError("Job does not contain scopeTrack; cannot reconstruct convergence series")
series = subworkflow.convergence_series(scope_track)
+if not series:
+ raise RuntimeError("Convergence series is empty; cannot determine converged parameter value")
@@
converged_value = series[-1]["param"]Also applies to: 460-460
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@other/materials_designer/workflows/convergence.ipynb` around lines 429 - 430,
The code assumes scopeTrack and convergence points always exist; fix by
defensively validating finished_job.get("scopeTrack") and the series returned by
subworkflow.convergence_series before indexing; if scope_track is missing or
series is empty, skip indexing (or log a warning) instead of doing series[-1];
apply the same checks to the other occurrence around the later cell that also
accesses series[-1] so you never index an empty series.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/total_energy_convergence.ipynb`:
- Around line 445-446: The code dereferences series[-1]["parameter"] without
checking series content; before extracting parameter_value, add a guard that
verifies series is non-empty and that the last element contains the "parameter"
key (e.g., check if series and "parameter" in series[-1]); if the check fails,
handle gracefully (raise a clear error, set a default, or skip) and include
context using job_workflow.subworkflows[subworkflow_index].convergence_parameter
and parameter_name to produce a helpful message.
🪄 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: 7d219c91-a7b3-434f-a63e-f5550c7f22c7
📒 Files selected for processing (2)
other/materials_designer/workflows/convergence.ipynbother/materials_designer/workflows/total_energy_convergence.ipynb
✅ Files skipped from review due to trivial changes (1)
- other/materials_designer/workflows/convergence.ipynb
Comment on lines
+445
to
+446
| "parameter_name = job_workflow.subworkflows[subworkflow_index].convergence_parameter\n", | ||
| "parameter_value = series[-1][\"parameter\"]\n", |
There was a problem hiding this comment.
Add a guard before dereferencing the last convergence point.
Line 446 can raise IndexError/KeyError if series is empty or missing "parameter". Add a small validation before extraction.
Proposed fix
parameter_name = job_workflow.subworkflows[subworkflow_index].convergence_parameter
-parameter_value = series[-1]["parameter"]
+if not series:
+ raise ValueError("Convergence series is empty; cannot determine converged parameter.")
+if "parameter" not in series[-1]:
+ raise KeyError("Missing 'parameter' in convergence series item.")
+parameter_value = series[-1]["parameter"]📝 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
| "parameter_name = job_workflow.subworkflows[subworkflow_index].convergence_parameter\n", | |
| "parameter_value = series[-1][\"parameter\"]\n", | |
| parameter_name = job_workflow.subworkflows[subworkflow_index].convergence_parameter | |
| if not series: | |
| raise ValueError("Convergence series is empty; cannot determine converged parameter.") | |
| if "parameter" not in series[-1]: | |
| raise KeyError("Missing 'parameter' in convergence series item.") | |
| parameter_value = series[-1]["parameter"] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@other/materials_designer/workflows/total_energy_convergence.ipynb` around
lines 445 - 446, The code dereferences series[-1]["parameter"] without checking
series content; before extracting parameter_value, add a guard that verifies
series is non-empty and that the last element contains the "parameter" key
(e.g., check if series and "parameter" in series[-1]); if the check fails,
handle gracefully (raise a clear error, set a default, or skip) and include
context using job_workflow.subworkflows[subworkflow_index].convergence_parameter
and parameter_name to produce a helpful message.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
other/materials_designer/workflows/convergence.ipynb (5)
194-196:⚠️ Potential issue | 🟠 MajorGuard empty default-project lookup before using
projects[0].This still assumes at least one default project and will crash for valid accounts with none.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@other/materials_designer/workflows/convergence.ipynb` around lines 194 - 196, The code assumes projects[0] exists when setting PROJECT_ID and printing; guard the default-project lookup by checking the returned projects list from client.projects.list (e.g., the variable projects) before accessing index 0, and handle the empty case (raise a clear error, log/print a helpful message and exit, or create a default project) so PROJECT_ID is only set when projects is non-empty and the print statement is conditional on that same check.
267-268:⚠️ Potential issue | 🟠 MajorHandle missing workflow match before
Workflow.create(...).
get_by_name_first_match(...)can return no result; this should be checked with a clear error before creating the workflow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@other/materials_designer/workflows/convergence.ipynb` around lines 267 - 268, The code calls Workflow.create(...) with workflow_config returned from WorkflowStandata.filter_by_application(app.name).get_by_name_first_match(WORKFLOW_SEARCH_TERM) without checking for a missing match; update the logic to capture the result into workflow_config, verify it is not None (or empty), and if it is missing raise or log a clear, descriptive error (or raise a specific exception) before calling Workflow.create; reference the WorkflowStandata.filter_by_application / get_by_name_first_match call and the Workflow.create invocation so you add the null-check around workflow_config and handle the error path cleanly.
439-439:⚠️ Potential issue | 🟠 MajorAvoid indexing convergence series without emptiness check.
series[-1]will crash when no convergence points are produced. Add a guard and fail with an actionable message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@other/materials_designer/workflows/convergence.ipynb` at line 439, The code accesses series[-1] to set converged_value without checking that series is non-empty; update the logic around converged_value = series[-1]["parameter"] to first guard that series is not empty (e.g., if not series: raise/return an error with an actionable message like "no convergence points produced"), and only then extract the last element's "parameter"; ensure any callers of the surrounding function/logic handle this explicit error path appropriately.
320-326:⚠️ Potential issue | 🟠 MajorValidate cluster selection result before building
Compute.This path can fail on empty cluster lists or unmatched
CLUSTER_NAME(yieldingNone/IndexError).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@other/materials_designer/workflows/convergence.ipynb` around lines 320 - 326, The code assumes clusters exist and that cluster selection succeeded before constructing Compute(cluster=cluster,...); validate the result of client.clusters.list() and the selected cluster: check that the returned list is non-empty and that when CLUSTER_NAME is set the generator found a matching cluster (cluster is not None); if validation fails, raise or log a clear error (including CLUSTER_NAME and number of clusters) or choose a safe fallback instead of letting Compute(...) raise IndexError/TypeError. Ensure these checks occur immediately after computing cluster and before the Compute(...) instantiation.
289-289:⚠️ Potential issue | 🟠 MajorCheck subworkflow existence before
workflow.subworkflows[0].Direct indexing can fail when the selected workflow has no subworkflows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@other/materials_designer/workflows/convergence.ipynb` at line 289, The code assumes a subworkflow exists by directly indexing workflow.subworkflows[0]; update the logic to first check that workflow.subworkflows is defined and non-empty before accessing index 0 (e.g., if not workflow.subworkflows or len(workflow.subworkflows) == 0 then handle the empty case by raising a clear error or skipping processing), and then assign convergence_subworkflow = workflow.subworkflows[0]; use the names workflow.subworkflows and convergence_subworkflow to locate and modify the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@utils/api.py`:
- Around line 272-275: The current code assumes finished_job contains a
"workflow" and that subworkflow_index is valid, causing unclear
KeyError/IndexError; update the helper to (1) verify finished_job is a dict and
contains the "workflow" key and raise a clear ValueError (e.g., "missing
workflow in finished_job for job_id={job_id}") if absent, (2) after creating
job_workflow via Workflow.create(finished_job["workflow"]), check that
subworkflow_index is an int within range 0 <= subworkflow_index <
len(job_workflow.subworkflows) and raise an IndexError or ValueError with an
actionable message if out of range, and (3) only then return
subworkflow.convergence_series(finished_job.get("scopeTrack")), so callers get
explicit, informative errors rather than opaque KeyError/IndexError from
client.jobs.get, Workflow.create, or job_workflow.subworkflows.
---
Duplicate comments:
In `@other/materials_designer/workflows/convergence.ipynb`:
- Around line 194-196: The code assumes projects[0] exists when setting
PROJECT_ID and printing; guard the default-project lookup by checking the
returned projects list from client.projects.list (e.g., the variable projects)
before accessing index 0, and handle the empty case (raise a clear error,
log/print a helpful message and exit, or create a default project) so PROJECT_ID
is only set when projects is non-empty and the print statement is conditional on
that same check.
- Around line 267-268: The code calls Workflow.create(...) with workflow_config
returned from
WorkflowStandata.filter_by_application(app.name).get_by_name_first_match(WORKFLOW_SEARCH_TERM)
without checking for a missing match; update the logic to capture the result
into workflow_config, verify it is not None (or empty), and if it is missing
raise or log a clear, descriptive error (or raise a specific exception) before
calling Workflow.create; reference the WorkflowStandata.filter_by_application /
get_by_name_first_match call and the Workflow.create invocation so you add the
null-check around workflow_config and handle the error path cleanly.
- Line 439: The code accesses series[-1] to set converged_value without checking
that series is non-empty; update the logic around converged_value =
series[-1]["parameter"] to first guard that series is not empty (e.g., if not
series: raise/return an error with an actionable message like "no convergence
points produced"), and only then extract the last element's "parameter"; ensure
any callers of the surrounding function/logic handle this explicit error path
appropriately.
- Around line 320-326: The code assumes clusters exist and that cluster
selection succeeded before constructing Compute(cluster=cluster,...); validate
the result of client.clusters.list() and the selected cluster: check that the
returned list is non-empty and that when CLUSTER_NAME is set the generator found
a matching cluster (cluster is not None); if validation fails, raise or log a
clear error (including CLUSTER_NAME and number of clusters) or choose a safe
fallback instead of letting Compute(...) raise IndexError/TypeError. Ensure
these checks occur immediately after computing cluster and before the
Compute(...) instantiation.
- Line 289: The code assumes a subworkflow exists by directly indexing
workflow.subworkflows[0]; update the logic to first check that
workflow.subworkflows is defined and non-empty before accessing index 0 (e.g.,
if not workflow.subworkflows or len(workflow.subworkflows) == 0 then handle the
empty case by raising a clear error or skipping processing), and then assign
convergence_subworkflow = workflow.subworkflows[0]; use the names
workflow.subworkflows and convergence_subworkflow to locate and modify the code.
🪄 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: 90e3b52f-3b80-4c39-a5e5-fb9c7ce97b88
📒 Files selected for processing (4)
other/materials_designer/workflows/Introduction.ipynbother/materials_designer/workflows/convergence.ipynbother/materials_designer/workflows/total_energy_convergence.ipynbutils/api.py
✅ Files skipped from review due to trivial changes (1)
- other/materials_designer/workflows/Introduction.ipynb
🚧 Files skipped from review as they are similar to previous changes (1)
- other/materials_designer/workflows/total_energy_convergence.ipynb
Comment on lines
+272
to
+275
| finished_job = client.jobs.get(job_id) | ||
| job_workflow = Workflow.create(finished_job["workflow"]) | ||
| subworkflow = job_workflow.subworkflows[subworkflow_index] | ||
| return subworkflow.convergence_series(finished_job.get("scopeTrack")) |
There was a problem hiding this comment.
Validate workflow payload and subworkflow index before indexing.
This helper can fail with unclear KeyError/IndexError when workflow is missing or subworkflow_index is invalid. Add explicit guards and raise actionable errors.
Suggested hardening
def get_convergence_series(client: APIClient, job_id: str, subworkflow_index: int = 0) -> List[dict]:
@@
- finished_job = client.jobs.get(job_id)
- job_workflow = Workflow.create(finished_job["workflow"])
- subworkflow = job_workflow.subworkflows[subworkflow_index]
- return subworkflow.convergence_series(finished_job.get("scopeTrack"))
+ finished_job = client.jobs.get(job_id)
+ workflow_payload = finished_job.get("workflow")
+ if not workflow_payload:
+ raise RuntimeError(f"Job {job_id} does not contain workflow payload")
+
+ job_workflow = Workflow.create(workflow_payload)
+ subworkflows = getattr(job_workflow, "subworkflows", None) or []
+ if not (0 <= subworkflow_index < len(subworkflows)):
+ raise IndexError(
+ f"subworkflow_index={subworkflow_index} is out of range; available: 0..{len(subworkflows)-1}"
+ )
+
+ return subworkflows[subworkflow_index].convergence_series(finished_job.get("scopeTrack"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@utils/api.py` around lines 272 - 275, The current code assumes finished_job
contains a "workflow" and that subworkflow_index is valid, causing unclear
KeyError/IndexError; update the helper to (1) verify finished_job is a dict and
contains the "workflow" key and raise a clear ValueError (e.g., "missing
workflow in finished_job for job_id={job_id}") if absent, (2) after creating
job_workflow via Workflow.create(finished_job["workflow"]), check that
subworkflow_index is an int within range 0 <= subworkflow_index <
len(job_workflow.subworkflows) and raise an IndexError or ValueError with an
actionable message if out of range, and (3) only then return
subworkflow.convergence_series(finished_job.get("scopeTrack")), so callers get
explicit, informative errors rather than opaque KeyError/IndexError from
client.jobs.get, Workflow.create, or job_workflow.subworkflows.
timurbazhirov
approved these changes
Apr 9, 2026
This was referenced May 11, 2026
Merged
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