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:
📝 WalkthroughWalkthroughAdds an end-to-end Equation of State (EOS) workflow: new EOS notebook, EOS helper utilities, small API job helper updates, and an Introduction notebook TOC update linking the new workflow. Changes are additions and markdown updates; no public API removals. Changes
Sequence DiagramsequenceDiagram
actor User
participant Browser as Browser / Jupyter
participant APIClient as APIClient
participant MaterialStore as Material Store
participant WorkflowSvc as Workflow Service
participant JobSvc as Job Service
participant Compute as Compute Cluster
participant Results as Results/Plotter
User->>Browser: configure EOS params
Browser->>APIClient: authenticate (OIDC)
APIClient-->>Browser: session token
Browser->>MaterialStore: load base material
MaterialStore-->>Browser: material object
loop for each scale
Browser->>Browser: generate scaled material (scale_material)
Browser->>APIClient: save/reuse scaled material + metadata
APIClient-->>Browser: material ID
end
Browser->>WorkflowSvc: load/select EOS workflow
WorkflowSvc-->>Browser: workflow object
Browser->>JobSvc: create jobs for scaled materials
JobSvc-->>Browser: job IDs
Browser->>JobSvc: submit_jobs(job IDs)
JobSvc->>Compute: submit jobs
Compute-->>JobSvc: job status updates
loop poll
Browser->>JobSvc: poll job status
JobSvc-->>Browser: statuses & results
end
Browser->>Results: extract energies & volumes
Results-->>User: plot energy vs volume & fit EOS
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 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/equation_of_state.ipynb`:
- Around line 442-446: The saved_workflow returned by get_or_create_workflow
(and wrapped by Workflow.create as saved_workflow) is a cleaned/persisted copy
that strips unit-level special context; when calling create_jobs_for_materials
you must pass the original in-memory workflow (the variable named workflow) so
edits from section 4.3 (k-grid and other per-unit settings) are preserved.
Locate the calls to create_jobs_for_materials that currently use saved_workflow
(and any similar uses around the other occurrence at lines ~521-524) and change
them to use the original workflow variable instead; keep
get_or_create_workflow/get_or_create_workflow response handling for persistence
but do not use saved_workflow for job creation.
- Around line 635-653: The plot is incorrectly showing E/V instead of E(V);
remove the computed "energy_per_volume" column and update the plot call to use
"total_energy" (use plot_df and px.line with y="total_energy" instead of
"energy_per_volume"), adjust fig title and yaxis_title to reflect "Total Energy
(eV)" and ensure hover_data still includes "total_energy"; use the existing
symbols successful_df, plot_df, fig, and px.line when making these edits.
In `@utils/api.py`:
- Around line 297-309: The loop inside create_jobs_for_materials currently
accepts any response from create_job and silently flattens lists, breaking the
documented 1:1 contract; change the handling so that for each material the
response must be exactly one job: call create_job (function create_job), then if
the response is a list require len(response) == 1 and use that single item, if
the response is None or a list with length != 1 raise a clear exception (or
return an error) to fail fast; update the code paths that append to the jobs
list (jobs.extend / jobs.append) to only append the single validated job for the
current material to preserve strict material->job correspondence.
In `@utils/equation_of_state.py`:
- Around line 67-81: The function build_eos_candidate_records currently zips
materials and scale_factors which silently truncates mismatched inputs; before
iterating, add a length check comparing len(materials) and len(scale_factors)
and raise a clear exception (e.g., ValueError) if they differ, mentioning both
lengths and the function name to help debug; keep the rest of the function logic
intact so records are only built when the two sequences are the same length.
🪄 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: 1caa4013-ad02-4ee1-abf6-bc1e343b24a7
📒 Files selected for processing (4)
other/materials_designer/workflows/Introduction.ipynbother/materials_designer/workflows/equation_of_state.ipynbutils/api.pyutils/equation_of_state.py
utils/api.py
Outdated
Comment on lines
+297
to
+309
| response = create_job( | ||
| api_client=api_client, | ||
| materials=[material], | ||
| workflow=workflow, | ||
| project_id=project_id, | ||
| owner_id=owner_id, | ||
| prefix=job_name, | ||
| compute=compute, | ||
| ) | ||
| if isinstance(response, list): | ||
| jobs.extend(response) | ||
| else: | ||
| jobs.append(response) |
There was a problem hiding this comment.
Fail fast unless each material produces exactly one job.
create_jobs_for_materials() is documented as 1:1, but this branch silently flattens any list returned by create_job(). If one iteration yields 0 or >1 jobs, downstream code that zips materials with jobs will misassociate IDs, statuses, and results.
Proposed fix
response = create_job(
api_client=api_client,
materials=[material],
workflow=workflow,
project_id=project_id,
owner_id=owner_id,
prefix=job_name,
compute=compute,
)
- if isinstance(response, list):
- jobs.extend(response)
- else:
- jobs.append(response)
+ created_jobs = response if isinstance(response, list) else [response]
+ if len(created_jobs) != 1:
+ raise ValueError(
+ f"Expected exactly one job for material index {index}, got {len(created_jobs)}."
+ )
+ jobs.append(created_jobs[0])
return jobs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@utils/api.py` around lines 297 - 309, The loop inside
create_jobs_for_materials currently accepts any response from create_job and
silently flattens lists, breaking the documented 1:1 contract; change the
handling so that for each material the response must be exactly one job: call
create_job (function create_job), then if the response is a list require
len(response) == 1 and use that single item, if the response is None or a list
with length != 1 raise a clear exception (or return an error) to fail fast;
update the code paths that append to the jobs list (jobs.extend / jobs.append)
to only append the single validated job for the current material to preserve
strict material->job correspondence.
Comment on lines
+67
to
+81
| def build_eos_candidate_records(materials: Sequence[Material], scale_factors: Sequence[float]) -> List[Dict[str, Any]]: | ||
| """ | ||
| Builds summary records for scaled EOS materials. | ||
| """ | ||
| records = [] | ||
| for scale_factor, material in zip(scale_factors, materials): | ||
| records.append( | ||
| { | ||
| "scale_factor": scale_factor, | ||
| "material_name": get_material_label(material), | ||
| "volume": material.lattice.cell_volume, | ||
| "atoms": material.basis.number_of_atoms, | ||
| } | ||
| ) | ||
| return records |
There was a problem hiding this comment.
Reject mismatched EOS inputs instead of truncating them.
If materials and scale_factors drift out of sync, this helper silently drops trailing items and produces an incomplete EOS table. Add a length check before iterating.
Proposed fix
def build_eos_candidate_records(materials: Sequence[Material], scale_factors: Sequence[float]) -> List[Dict[str, Any]]:
"""
Builds summary records for scaled EOS materials.
"""
+ if len(materials) != len(scale_factors):
+ raise ValueError("materials and scale_factors must have the same length.")
+
records = []
for scale_factor, material in zip(scale_factors, materials):
records.append(
{
"scale_factor": scale_factor,🧰 Tools
🪛 Ruff (0.15.7)
[warning] 72-72: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@utils/equation_of_state.py` around lines 67 - 81, The function
build_eos_candidate_records currently zips materials and scale_factors which
silently truncates mismatched inputs; before iterating, add a length check
comparing len(materials) and len(scale_factors) and raise a clear exception
(e.g., ValueError) if they differ, mentioning both lengths and the function name
to help debug; keep the rest of the function logic intact so records are only
built when the two sequences are the same length.
utils/equation_of_state.py
Outdated
| return "Material" | ||
|
|
||
|
|
||
| def get_isotropic_strain_matrix(scale_factor: float) -> Matrix3x3Schema: |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
utils/equation_of_state.py (1)
47-53:⚠️ Potential issue | 🟡 MinorPrevent silent truncation when EOS inputs are mismatched.
At Line 52,
zip(scale_factors, materials)drops trailing items when lengths differ. Add an explicit length check and usestrict=Trueso mismatches fail fast with a clear error.Suggested patch
def build_eos_candidate_records(materials: Sequence[Material], scale_factors: Sequence[float]) -> List[Dict[str, Any]]: """ Builds summary records for scaled EOS materials. """ + if len(materials) != len(scale_factors): + raise ValueError( + f"build_eos_candidate_records: materials ({len(materials)}) and " + f"scale_factors ({len(scale_factors)}) must have the same length." + ) + records = [] - for scale_factor, material in zip(scale_factors, materials): + for scale_factor, material in zip(scale_factors, materials, strict=True): records.append( { "scale_factor": scale_factor, "material_name": get_material_label(material), "volume": material.lattice.cell_volume, "atoms": material.basis.number_of_atoms, } ) return records#!/bin/bash # Verify Python-version compatibility before adopting zip(..., strict=True) # Expected: Python >= 3.10 in project constraints. set -euo pipefail fd -HI 'pyproject.toml|setup.cfg|setup.py|tox.ini|Pipfile|.python-version' rg -n --iglob 'pyproject.toml' --iglob 'setup.cfg' --iglob 'setup.py' --iglob 'tox.ini' --iglob 'Pipfile' --iglob '.python-version' \ 'requires-python|python_requires|target-version|minversion|python_version'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/equation_of_state.py` around lines 47 - 53, The function build_eos_candidate_records currently iterates with zip(scale_factors, materials) which silently drops items if the sequences differ in length; update the function to first assert or raise a clear ValueError when len(scale_factors) != len(materials) and then iterate using zip(scale_factors, materials, strict=True) so mismatched inputs fail fast with a descriptive error; reference the parameters scale_factors and materials and the function name build_eos_candidate_records when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@utils/equation_of_state.py`:
- Around line 47-53: The function build_eos_candidate_records currently iterates
with zip(scale_factors, materials) which silently drops items if the sequences
differ in length; update the function to first assert or raise a clear
ValueError when len(scale_factors) != len(materials) and then iterate using
zip(scale_factors, materials, strict=True) so mismatched inputs fail fast with a
descriptive error; reference the parameters scale_factors and materials and the
function name build_eos_candidate_records when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68865ac0-04be-4e49-bfc1-05caac5e9d2c
📒 Files selected for processing (1)
utils/equation_of_state.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
other/materials_designer/workflows/equation_of_state.ipynb (1)
565-577:⚠️ Potential issue | 🟠 MajorEnforce exactly one job per material in this loop.
Line 576 silently accepts multi-item responses and assumes non-empty lists. That can desynchronize material/job records and mislabel results.
Proposed fix
for index, material in enumerate(saved_materials): job_name = f"{job_prefix} | scale={SCALE_FACTORS[index]:.4f}" job = create_job( api_client=client, materials=[material], workflow=workflow, project_id=project_id, owner_id=ACCOUNT_ID, prefix=job_name, compute=compute.to_dict(), ) - jobs.append(job if not isinstance(job, list) else job[0]) + created_jobs = job if isinstance(job, list) else [job] + if len(created_jobs) != 1: + raise ValueError( + f"Expected exactly one job for material index {index}, got {len(created_jobs)}." + ) + jobs.append(created_jobs[0])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@other/materials_designer/workflows/equation_of_state.ipynb` around lines 565 - 577, The loop over saved_materials can receive lists from create_job and currently accepts multi-item responses, causing misalignment; change the handling in the loop that calls create_job (with symbols saved_materials, SCALE_FACTORS, create_job, jobs, job_name) to enforce exactly one job per material: validate the return value is a single job (not a list or empty), raise or log an error if create_job returns a list or None/empty, and only append that single job to jobs (do not silently pick job[0]); include clear error context mentioning the material index and job_name so failures are explicit and won't desynchronize records.
🧹 Nitpick comments (1)
utils/api.py (1)
211-229: Aligncreate_jobdocstring with the declared return type.The function is now typed as
Union[dict, List[dict]], but the docstring still says it returnslist[dict]. Please update theReturnssection to match the actual contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/api.py` around lines 211 - 229, The docstring for create_job is out of sync with its type signature: update the Returns section to reflect the declared return type Union[dict, List[dict]] (i.e., explain that create_job may return a single job dict or a list of job dicts depending on whether a single material or multiple materials were passed), and include brief notes about the circumstances when a dict vs a list[dict] is returned; update the Returns description and example lines in the create_job docstring to match the function signature.
🤖 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/equation_of_state.ipynb`:
- Around line 207-209: The code directly indexes projects[0] after calling
client.projects.list, which will raise if no default project exists; update the
logic around projects, project_id and the print statement to check if the
returned list is empty (e.g., if not projects) and fail fast with a clear error
message (or raise an exception/exit) before attempting to access projects[0];
ensure the error message includes the ACCOUNT_ID and suggests creating a default
project so the subsequent indexing and use of project_id is safe.
- Around line 491-516: The cluster-selection code using the clusters list and
CLUSTER_NAME can fail when clusters is empty or when CLUSTER_NAME doesn't match
any hostname; update the block (around the Compute import and the logic that
assigns cluster from clusters and CLUSTER_NAME) to first check if clusters is
empty and raise a clear exception or return early, then if CLUSTER_NAME is set
use next(...) but handle None by raising a descriptive error (or falling back to
clusters[0] only if you decide that's safe), and ensure any downstream use of
cluster assumes a valid object; include clear messages mentioning CLUSTER_NAME
and the available hostnames to aid debugging.
---
Duplicate comments:
In `@other/materials_designer/workflows/equation_of_state.ipynb`:
- Around line 565-577: The loop over saved_materials can receive lists from
create_job and currently accepts multi-item responses, causing misalignment;
change the handling in the loop that calls create_job (with symbols
saved_materials, SCALE_FACTORS, create_job, jobs, job_name) to enforce exactly
one job per material: validate the return value is a single job (not a list or
empty), raise or log an error if create_job returns a list or None/empty, and
only append that single job to jobs (do not silently pick job[0]); include clear
error context mentioning the material index and job_name so failures are
explicit and won't desynchronize records.
---
Nitpick comments:
In `@utils/api.py`:
- Around line 211-229: The docstring for create_job is out of sync with its type
signature: update the Returns section to reflect the declared return type
Union[dict, List[dict]] (i.e., explain that create_job may return a single job
dict or a list of job dicts depending on whether a single material or multiple
materials were passed), and include brief notes about the circumstances when a
dict vs a list[dict] is returned; update the Returns description and example
lines in the create_job docstring to match the function signature.
🪄 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: 2bd44768-2174-48bc-bff4-247a6a2b8033
📒 Files selected for processing (3)
other/materials_designer/workflows/equation_of_state.ipynbpyproject.tomlutils/api.py
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
Comment on lines
+207
to
+209
| "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 missing default project before indexing.
projects[0] will crash when no default project exists for the selected owner. Add an explicit check and fail with a clear message.
Proposed fix
projects = client.projects.list({"isDefault": True, "owner._id": ACCOUNT_ID})
-project_id = projects[0]["_id"]
+if not projects:
+ raise ValueError(f"No default project found for owner '{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/equation_of_state.ipynb` around lines 207
- 209, The code directly indexes projects[0] after calling client.projects.list,
which will raise if no default project exists; update the logic around projects,
project_id and the print statement to check if the returned list is empty (e.g.,
if not projects) and fail fast with a clear error message (or raise an
exception/exit) before attempting to access projects[0]; ensure the error
message includes the ACCOUNT_ID and suggests creating a default project so the
subsequent indexing and use of project_id is safe.
Comment on lines
+491
to
+516
| "clusters = client.clusters.list()\n", | ||
| "print(f\"Available clusters: {[c['hostname'] for c in clusters]}\")" | ||
| ] | ||
| }, | ||
| { | ||
| "cell_type": "markdown", | ||
| "id": "35", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "### 5.2. Create compute configuration for the jobs" | ||
| ] | ||
| }, | ||
| { | ||
| "cell_type": "code", | ||
| "execution_count": null, | ||
| "id": "36", | ||
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "from mat3ra.ide.compute import Compute\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]\n", | ||
| "\n", |
There was a problem hiding this comment.
Handle empty cluster list and unmatched CLUSTER_NAME explicitly.
This block can fail when clusters is empty or when CLUSTER_NAME does not match any hostname, leading to downstream attribute errors.
Proposed fix
clusters = client.clusters.list()
print(f"Available clusters: {[c['hostname'] for c in clusters]}")
+if not clusters:
+ raise ValueError("No 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 ValueError(f"No cluster matched CLUSTER_NAME='{CLUSTER_NAME}'.")
else:
cluster = clusters[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@other/materials_designer/workflows/equation_of_state.ipynb` around lines 491
- 516, The cluster-selection code using the clusters list and CLUSTER_NAME can
fail when clusters is empty or when CLUSTER_NAME doesn't match any hostname;
update the block (around the Compute import and the logic that assigns cluster
from clusters and CLUSTER_NAME) to first check if clusters is empty and raise a
clear exception or return early, then if CLUSTER_NAME is set use next(...) but
handle None by raising a descriptive error (or falling back to clusters[0] only
if you decide that's safe), and ensure any downstream use of cluster assumes a
valid object; include clear messages mentioning CLUSTER_NAME and the available
hostnames to aid debugging.
utils/equation_of_state.py
Outdated
| ) | ||
|
|
||
|
|
||
| def get_material_label(material: Material) -> str: |
Member
There was a problem hiding this comment.
We can move it to made.Material itself or remove
utils/equation_of_state.py
Outdated
| return "Material" | ||
|
|
||
|
|
||
| def scale_material( |
utils/equation_of_state.py
Outdated
| "scale_factor": scale_factor, | ||
| "material_name": get_material_label(material), | ||
| "volume": material.lattice.cell_volume, | ||
| "atoms": material.basis.number_of_atoms, |
timurbazhirov
approved these changes
Apr 1, 2026
timurbazhirov
approved these changes
Apr 2, 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
Documentation