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:
📝 WalkthroughWalkthroughReplaced manual band-gap assembly in the notebook with a non-scalar Changes
Sequence Diagram(s)sequenceDiagram
participant Notebook
participant APIClient
participant PropsService
participant Visualizer
participant Viewer
Notebook->>APIClient: submit job -> receives `job_id`
Notebook->>APIClient: get_for_job(job_id, PropertyName.non_scalar.band_gaps)
APIClient->>PropsService: request non-scalar "band_gaps" for job
PropsService-->>APIClient: return band_gaps payload
APIClient-->>Notebook: return band_gaps result
Notebook->>Visualizer: visualize_properties(band_gaps, title="Band Gaps")
Visualizer->>Visualizer: prefilter results (keep items with "value" or "values")
Visualizer->>Viewer: render visual output
Viewer-->>Notebook: embedded visualization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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: 2
🤖 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/band_gap.ipynb`:
- Line 594: The line calling client.properties.get_for_job uses an undefined
job_id and will raise NameError; replace the undefined variable with the
notebook's job identifier (use job._id as used elsewhere) so change the call in
the band_gaps_property assignment to pass job._id to get_for_job (refer to the
expression client.properties.get_for_job(...,
property_name=PropertyName.non_scalar.band_gaps) and the job object referenced
as job._id elsewhere in the notebook).
- Around line 592-595: Replace the invalid call to
client.properties.get_for_job(job_id,
property_name=PropertyName.non_scalar.band_gaps) because PropertiesEndpoints
does not expose get_for_job; instead either call
client.properties.get_property(job_id, unit_flowchart_id,
property_name=PropertyName.non_scalar.band_gaps) to fetch the first matching
property (supply the correct unit_flowchart_id), call
client.properties.get_direct_band_gap(job_id, unit_flowchart_id) if you need the
direct band gap specifically, or use
client.properties.list(query={"source.info.jobId": job_id}) to list all
properties for the job and then filter for PropertyName.non_scalar.band_gaps;
keep using PropertyName.non_scalar.band_gaps as the property identifier when
applicable.
| "from mat3ra.prode import PropertyName\n", | ||
| "\n", | ||
| "property_endpoints = client.properties\n", | ||
| "\n", | ||
| "workflow = Workflow.create(job_dict[\"workflow\"])\n", | ||
| "unit_flowchart_id = workflow.subworkflows[BAND_GAP_SWF_INDEX].get_unit_by_name(name=\"pw_nscf\").flowchart_id\n", | ||
| "band_gap_direct = property_endpoints.get_direct_band_gap(job._id, unit_flowchart_id)\n", | ||
| "band_gap_indirect = property_endpoints.get_indirect_band_gap(job._id, unit_flowchart_id)\n", | ||
| "\n", | ||
| "# Minimal ProDe-like property JSON for Prove MVP\n", | ||
| "band_gaps_property = {\n", | ||
| " \"name\": \"band_gaps\",\n", | ||
| " \"values\": [\n", | ||
| " {\"type\": \"direct\", \"value\": float(band_gap_direct), \"units\": \"eV\"},\n", | ||
| " {\"type\": \"indirect\", \"value\": float(band_gap_indirect), \"units\": \"eV\"},\n", | ||
| " ],\n", | ||
| "}\n", | ||
| "\n", | ||
| "render_prove([band_gaps_property], title=\"Band Gaps\")" | ||
| "band_gaps_property = client.properties.get_for_job(job_id, property_name=PropertyName.non_scalar.band_gaps)\n", | ||
| "render_prove(band_gaps_property, title=\"Band Gaps\")" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for PropertyName definition and non_scalar attribute
rg -n "class PropertyName" --type py -A 20Repository: Exabyte-io/api-examples
Length of output: 49
🏁 Script executed:
# Search for get_for_job method definition
rg -n "def get_for_job" --type py -A 10Repository: Exabyte-io/api-examples
Length of output: 49
🏁 Script executed:
# Search for render_prove function definition
rg -n "def render_prove" --type py -A 5Repository: Exabyte-io/api-examples
Length of output: 448
🏁 Script executed:
# Also check the notebook file content around the specified lines
cat -n other/materials_designer/workflows/band_gap.ipynb | head -600 | tail -50Repository: Exabyte-io/api-examples
Length of output: 1517
🏁 Script executed:
# Search for client initialization to understand the API
rg -n "client\s*=" --type py -B 2 -A 2 | head -50Repository: Exabyte-io/api-examples
Length of output: 49
🏁 Script executed:
# Check requirements.txt or setup.py for mat3ra package
fd -e "requirements.txt" -e "setup.py" -e "pyproject.toml" | xargs catRepository: Exabyte-io/api-examples
Length of output: 49
🏁 Script executed:
# Search for PropertyName usage in the notebook to understand context
rg -n "PropertyName" other/materials_designer/workflows/band_gap.ipynbRepository: Exabyte-io/api-examples
Length of output: 238
🏁 Script executed:
# Search for get_for_job calls to see if it's used elsewhere
rg -n "get_for_job" --type pyRepository: Exabyte-io/api-examples
Length of output: 49
🏁 Script executed:
# Read the full notebook to find client initialization
python3 << 'EOF'
import json
with open('other/materials_designer/workflows/band_gap.ipynb', 'r') as f:
nb = json.load(f)
for i, cell in enumerate(nb.get('cells', [])):
if cell.get('cell_type') == 'code':
source = ''.join(cell.get('source', []))
# Look for client initialization
if 'client' in source.lower() and ('=' in source or 'import' in source):
print(f"Cell {i}:\n{source}\n---")
EOFRepository: Exabyte-io/api-examples
Length of output: 2223
🏁 Script executed:
# Search for all requirements files more broadly
find . -name "*requirements*" -o -name "setup.py" -o -name "pyproject.toml" -o -name "setup.cfg" | xargs ls -la 2>/dev/nullRepository: Exabyte-io/api-examples
Length of output: 132
🏁 Script executed:
# Check for any documentation about the API
fd -e "*.md" | xargs grep -l "PropertyName\|get_for_job" 2>/dev/nullRepository: Exabyte-io/api-examples
Length of output: 51
🏁 Script executed:
cat pyproject.tomlRepository: Exabyte-io/api-examples
Length of output: 2441
🏁 Script executed:
# Also search for any documentation mentioning PropertyName or band_gaps
grep -r "band_gaps\|PropertyName" . --include="*.md" --include="*.txt" --include="*.rst" 2>/dev/null | head -20Repository: Exabyte-io/api-examples
Length of output: 49
🌐 Web query:
mat3ra.prode PropertyName non_scalar band_gaps API documentation
💡 Result:
In Mat3ra’s Properties Directory, the non-scalar “Band Gaps” property is identified in the JSON schema by the property name enum value:
"band_gaps"(this is the canonical property key you should map to inPropertyName/ property definitions). [1]
Schema highlights (from Mat3ra “List of Schemas”):
type: "object"properties.name.enum: ["band_gaps"]properties.values: array of band-gap entries ($ref: ../../core/reusable/band_gap.json)properties.eigenvalues: optional array withkpoint,weight, and per-spinenergies/occupationsrequired: ["name"][1]
Where this is documented:
- Conceptual/docs page: Properties Directory → Non-scalar → Band Gaps. [2]
- The
mat3ra-prodepackage is the property-definitions library used by the Mat3ra ecosystem (wherePropertyNamewould be defined/used). [3]
Sources: [1] [2] [3]
🌐 Web query:
mat3ra APIClient properties get_for_job method
💡 Result:
In the current Mat3ra Python API client (mat3ra-api-client), APIClient.properties is a PropertiesEndpoints instance, and it does not provide a get_for_job method. (raw.githubusercontent.com)
What you can use instead
Get all properties produced by a given job (filter by source.info.jobId) via the generic list() method:
props = client.properties.list(query={"source.info.jobId": job_id})EntityEndpoint.list() sends the query as a Mongo-style JSON filter. (raw.githubusercontent.com)
Existing helper methods on client.properties
PropertiesEndpoints includes helpers like:
get_property(job_id, unit_flowchart_id, property_name)(returns the first matching property)get_direct_band_gap(job_id, unit_flowchart_id)get_indirect_band_gap(job_id, unit_flowchart_id)(raw.githubusercontent.com)
Citations:
- 1: https://raw.githubusercontent.com/Exabyte-io/api-client/main/src/py/mat3ra/api_client/client.py
- 2: https://raw.githubusercontent.com/Exabyte-io/api-client/main/src/py/mat3ra/api_client/endpoints/entity.py
- 3: https://raw.githubusercontent.com/Exabyte-io/api-client/main/src/py/mat3ra/api_client/endpoints/properties.py
client.properties.get_for_job() method does not exist in the current mat3ra-api-client.
The method call client.properties.get_for_job(job_id, property_name=PropertyName.non_scalar.band_gaps) is incorrect. The PropertiesEndpoints class does not provide a get_for_job method. Use one of these alternatives instead:
client.properties.get_property(job_id, unit_flowchart_id, property_name=PropertyName.non_scalar.band_gaps)— retrieves the first matching propertyclient.properties.get_direct_band_gap(job_id, unit_flowchart_id)— retrieves the direct band gapclient.properties.list(query={"source.info.jobId": job_id})— lists all properties for a job
The PropertyName.non_scalar.band_gaps attribute path is valid.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@other/materials_designer/workflows/band_gap.ipynb` around lines 592 - 595,
Replace the invalid call to client.properties.get_for_job(job_id,
property_name=PropertyName.non_scalar.band_gaps) because PropertiesEndpoints
does not expose get_for_job; instead either call
client.properties.get_property(job_id, unit_flowchart_id,
property_name=PropertyName.non_scalar.band_gaps) to fetch the first matching
property (supply the correct unit_flowchart_id), call
client.properties.get_direct_band_gap(job_id, unit_flowchart_id) if you need the
direct band gap specifically, or use
client.properties.list(query={"source.info.jobId": job_id}) to list all
properties for the job and then filter for PropertyName.non_scalar.band_gaps;
keep using PropertyName.non_scalar.band_gaps as the property identifier when
applicable.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pyproject.toml (1)
12-12: Replace git-commit pins with available PyPI releasesBoth
mat3ra-api-clientandmat3ra-prodeare pinned to unreleased development commits (dated 2026-02-18) that are ahead of the latest tagged and PyPI-published versions (both at2026.1.23.post0). Using VCS pins creates several practical problems:
- pip-tools lock file:
pip-compiledoes not resolve or lock the transitive dependencies of VCS-based deps, so anyrequirements.txtgenerated by the project'spip-toolsdev tooling will silently leave those transitive deps unpinned.- Upgrade friction: Commit SHAs provide no semantic indication of what version/feature is included. If upstream rebases or force-pushes, the SHA becomes unreachable.
- Colab users:
pip install "mat3ra-api-examples[colab]"will attempt to resolve these git URLs at install time, which is slower and requires git at the pip level.Since stable releases exist on PyPI, migrate to those versions. If the development commits provide critical features not yet released, add a brief inline comment documenting the target PyPI version to restore, and open a follow-up ticket to update once those commits are released.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 12, Replace the VCS pins in pyproject.toml for mat3ra-api-client and mat3ra-prode (the lines containing "mat3ra-api-client @ git+https://..." and the mat3ra-prode git URL) with the corresponding released PyPI versions (e.g., ==2026.1.23.post0) so pip-tools can lock transitive dependencies; if those specific git commits are required before a release, leave the PyPI version and add a short inline comment referencing the missing feature/commit and create a follow-up ticket to update the package once the release is published.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyproject.toml`:
- Line 11: The comment on the pyproject.toml line references file:// file URIs
but the actual dependency is a VCS specifier using git+https://; update or
remove the misleading comment so it accurately describes the dependency type
(e.g., mention PEP 508 VCS specifiers or the git+https:// URL) and ensure any
guidance matches the dependency string git+https:// rather than file:/// URIs.
---
Nitpick comments:
In `@pyproject.toml`:
- Line 12: Replace the VCS pins in pyproject.toml for mat3ra-api-client and
mat3ra-prode (the lines containing "mat3ra-api-client @ git+https://..." and the
mat3ra-prode git URL) with the corresponding released PyPI versions (e.g.,
==2026.1.23.post0) so pip-tools can lock transitive dependencies; if those
specific git commits are required before a release, leave the PyPI version and
add a short inline comment referencing the missing feature/commit and create a
follow-up ticket to update the package once the release is published.
| # otherwise on 3.21.1 we encountered negative number of atoms during supercell generation | ||
| "ase>=3.25.0", | ||
| "mat3ra-api-client>=2026.1.23.post0", | ||
| # Use a valid PEP 508 URL for a local file dependency: file URIs must be absolute and include three slashes |
There was a problem hiding this comment.
Misleading comment — references file:// URIs but the dependency is a VCS URL
The comment on Line 11 says "file URIs must be absolute and include three slashes", which describes file:/// local-path dependencies. The actual dependency on Line 12 is a git+https:// VCS URL — a completely different PEP 508 specifier type. The comment is inaccurate and could confuse future editors.
✏️ Suggested fix
- # Use a valid PEP 508 URL for a local file dependency: file URIs must be absolute and include three slashes
- "mat3ra-api-client @ git+https://github.com/Exabyte-io/api-client.git@a329f363893ee96832b5c1a3c514ade28f0839cb",
+ # Pinned to a specific commit until a new PyPI release is available; replace once mat3ra-api-client >= <target_version> is published
+ "mat3ra-api-client @ git+https://github.com/Exabyte-io/api-client.git@a329f363893ee96832b5c1a3c514ade28f0839cb",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyproject.toml` at line 11, The comment on the pyproject.toml line references
file:// file URIs but the actual dependency is a VCS specifier using
git+https://; update or remove the misleading comment so it accurately describes
the dependency type (e.g., mention PEP 508 VCS specifiers or the git+https://
URL) and ensure any guidance matches the dependency string git+https:// rather
than file:/// URIs.
VsevolodX
changed the title
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 `@utils/visualize.py`:
- Around line 406-408: After filtering with results = [r for r in results if
"value" in r or "values" in r], add an early-return guard that checks if the
filtered results list is empty; if so emit a clear warning (using the module
logger or warnings.warn) explaining that all items were dropped because they
lacked "value"/"values" and return early (e.g., None or an empty display
structure) to avoid rendering an empty panel. Locate the filter using the
variable name results and implement the warning + return immediately after that
list comprehension.
| # Only keep the properties we can display | ||
| results = [r for r in results if "value" in r or "values" in r] | ||
|
|
There was a problem hiding this comment.
Add an early-return guard and a warning for the empty-after-filter case.
Items without "value" or "values" are silently dropped. If all results are filtered out the viewer still renders (displaying an empty panel), giving the caller no feedback. This is easy to hit when prototyping: passing a freshly-fetched property dict that uses a different key name (e.g. "data") produces a blank viewer with no diagnostic output.
🛡️ Proposed fix
# Only keep the properties we can display
results = [r for r in results if "value" in r or "values" in r]
+
+ if not results:
+ print("render_prove: no displayable properties found (each item must contain 'value' or 'values').")
+ return📝 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.
| # Only keep the properties we can display | |
| results = [r for r in results if "value" in r or "values" in r] | |
| # Only keep the properties we can display | |
| results = [r for r in results if "value" in r or "values" in r] | |
| if not results: | |
| print("render_prove: no displayable properties found (each item must contain 'value' or 'values').") | |
| return |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@utils/visualize.py` around lines 406 - 408, After filtering with results = [r
for r in results if "value" in r or "values" in r], add an early-return guard
that checks if the filtered results list is empty; if so emit a clear warning
(using the module logger or warnings.warn) explaining that all items were
dropped because they lacked "value"/"values" and return early (e.g., None or an
empty display structure) to avoid rendering an empty panel. Locate the filter
using the variable name results and implement the warning + return immediately
after that list comprehension.
There was a problem hiding this comment.
🤖 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/band_gap.ipynb`:
- Around line 592-595: Check whether client.properties.get_for_job is actually
present in the updated mat3ra-api-client (it may still be missing from
PropertiesEndpoints which currently exposes get_property, get_direct_band_gap,
get_indirect_band_gap, and list); run the suggested searches (rg/fd) against the
installed/vendored package to confirm the presence of get_for_job, and if it is
absent, change the notebook to use an available API: call client.properties.list
or client.properties.get_property (or get_direct/indirect_band_gap as
appropriate) and filter by job_id to obtain band gap entries, replacing
client.properties.get_for_job with the chosen existing method and updating the
call site (band_gaps_property and render_prove usage) accordingly.
There was a problem hiding this comment.
🤖 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/band_gap.ipynb`:
- Around line 591-595: Confirm that mat3ra.prode.PropertyName and
client.properties.get_for_job(job_id, property_name=...) are available in the
pinned dependencies, then protect the visualize_properties call against a
None/empty result: call client.properties.get_for_job and if the result is None
or empty (e.g., falsy or len==0), log or handle the missing property and skip
calling visualize_properties; otherwise call
visualize_properties(band_gaps_property, title="Band Gaps"). Use the exact
symbols client.properties.get_for_job, PropertyName.non_scalar.band_gaps, and
visualize_properties to locate the code and add the guard.
In `@utils/visualize.py`:
- Around line 406-408: After filtering "results = [r for r in results if "value"
in r or "values" in r]" add an empty-results guard to avoid rendering an empty
viewer: check "if not results:" and return an early, explicit diagnostic (e.g.,
raise ValueError("No displayable results after filtering") or return a
structured error/None depending on the surrounding API) so callers receive a
clear message instead of an empty viewer; update any callers or tests that
expect the previous behavior if necessary.
1 participant
Summary by CodeRabbit