feature/SOF 7894 Feature: create Notebooks Utils - #310
Merged
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR restructures utilities into a layered package at src/py/mat3ra/notebooks_utils (primitive, core, ipython, pyodide), migrating and splitting prior monolithic utils into modular I/O, auth, plotting, visualization, package-install, and job helpers, and updating packaging and CI. ChangesPackage Restructuring and Layer Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
| return result | ||
|
|
||
|
|
||
| async def select_coordination_threshold_emscripten(distribution: Dict[int, int], default_threshold: int) -> int: |
Member
There was a problem hiding this comment.
Should go to prompt.py and maybe into core?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
♻️ Duplicate comments (1)
src/py/mat3ra/notebooks_utils/__init__.py (1)
14-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid eager
ipythonimport in package root exports.Line 14 eagerly imports
.ipython.ui; importingmat3ra.notebooks_utilscan fail in base installs without notebook extras.Proposed fix (lazy export)
-from .ipython.ui import display_JSON @@ __all__ = [ "display_JSON", @@ ] + +def __getattr__(name): + if name == "display_JSON": + from .ipython.ui import display_JSON as _display_json + return _display_json + raise AttributeError(f"module {__name__!r} has no attribute {name!r}")#!/bin/bash # Verify that __init__ eagerly imports ipython layer while base deps are empty python - <<'PY' import pathlib, tomllib py = tomllib.loads(pathlib.Path("pyproject.toml").read_text()) print("Base dependencies:", py["project"]["dependencies"]) print("Optional groups:", sorted(py["project"]["optional-dependencies"].keys())) print("\n__init__.py imports:") print(pathlib.Path("src/py/mat3ra/notebooks_utils/__init__.py").read_text()) PY # Check likely optional notebook imports in ipython/ui.py rg -n '^(from|import)\s+(IPython|ipywidgets|plotly|matplotlib|pandas)\b' src/py/mat3ra/notebooks_utils/ipython/ui.py🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/__init__.py` around lines 14 - 31, The package __init__ currently does a top-level import of .ipython.ui (display_JSON) which causes import failures when notebook extras are not installed; remove the eager "from .ipython.ui import display_JSON" and instead implement a lazy export: either add a package-level __getattr__ in src/py/mat3ra/notebooks_utils/__init__.py that imports .ipython.ui and returns display_JSON on first access, or define a small wrapper function named display_JSON that performs "from .ipython import ui; return ui.display_JSON(...)" at call time; keep the rest of __all__ unchanged so consumers can still import display_JSON but the IPython-dependent module is only imported when actually used.
🟠 Major comments (22)
src/py/mat3ra/notebooks_utils/pyodide/io.py-58-67 (1)
58-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
get_data_pyodideignores thekeyparameter when retrieving data.The function always retrieves
data_from_hostregardless of whatkeyis passed. It should likely retrieve the specific key from thedata_from_hostdict.🐛 Proposed fix
def get_data_pyodide(key: str, globals_dict: Optional[Dict] = None): """ Load data from the host environment into globals()[key] variable. Args: key (str): Global variable name to store the received data. globals_dict (dict, optional): globals() dictionary of the current scope. """ if globals_dict is not None: - globals_dict[key] = globals_dict.get("data_from_host", None) + data_from_host = globals_dict.get("data_from_host", {}) + globals_dict[key] = data_from_host.get(key) if isinstance(data_from_host, dict) else None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/pyodide/io.py` around lines 58 - 67, get_data_pyodide currently always pulls "data_from_host" instead of using the provided key; change it so it looks up the requested key inside the host payload (e.g. data_from_host.get(key)) and assigns that value into the target globals dict (or globals() if globals_dict is None). Update the function get_data_pyodide to first obtain the host payload from globals_dict.get("data_from_host") (or globals().get("data_from_host") when globals_dict is None), then set globals_dict[key] (or globals()[key]) to payload.get(key, None) so the passed key is respected.src/py/mat3ra/notebooks_utils/pyodide/ui.py-46-58 (1)
46-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLoop exits on invalid input instead of retrying.
The
breakon line 55 causes the loop to exit even when the user enters an invalid value. The intent appears to be to keep prompting until valid input is received.🐛 Proposed fix
while True: try: value_str = await input(prompt_text) # type: ignore value = int(value_str) if value in coordination_numbers: coordination_threshold = value break else: print(f"Invalid value. Please enter one of these coordination numbers: {coordination_numbers}") - break + # Continue looping to allow retry except ValueError: print(f"Please enter a valid integer value from: {coordination_numbers}") return coordination_threshold🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/pyodide/ui.py` around lines 46 - 58, The loop currently breaks on both valid and invalid inputs because of the extraneous break in the else branch; update the input loop (the block using prompt_text, value_str, coordination_numbers, coordination_threshold) so it only breaks when a valid integer from coordination_numbers is entered (i.e., keep the break in the if value in coordination_numbers branch) and remove the break from the else branch so the loop continues retrying on invalid values; ensure coordination_threshold is returned only after a valid selection is made.src/py/mat3ra/notebooks_utils/pyodide/api/auth.py-5-15 (1)
5-15:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle missing
apiConfigto preventAttributeError.If
data_from_hostlacks an"apiConfig"key,apiConfigwill beNone, and the subsequent.get()calls on lines 10–13 will raiseAttributeError: 'NoneType' object has no attribute 'get'.🛡️ Proposed fix
async def authenticate_jupyterlite(data_from_host: dict) -> None: - apiConfig = data_from_host.get("apiConfig") + apiConfig = data_from_host.get("apiConfig", {}) os.environ.update(data_from_host.get("environ", {})) + if not apiConfig: + return os.environ.update( dict( - ACCOUNT_ID=apiConfig.get("accountId"), # type: ignore - AUTH_TOKEN=apiConfig.get("authToken"), # type: ignore - ORGANIZATION_ID=apiConfig.get("organizationId", ""), # type: ignore - CLUSTERS=json.dumps(apiConfig.get("clusters", [])), # type: ignore + ACCOUNT_ID=apiConfig.get("accountId", ""), + AUTH_TOKEN=apiConfig.get("authToken", ""), + ORGANIZATION_ID=apiConfig.get("organizationId", ""), + CLUSTERS=json.dumps(apiConfig.get("clusters", [])), ) )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/pyodide/api/auth.py` around lines 5 - 15, authenticate_jupyterlite currently assumes data_from_host["apiConfig"] exists and calls apiConfig.get(...), which will raise AttributeError if apiConfig is None; update the function to defensively handle missing or non-dict apiConfig by retrieving it with data_from_host.get("apiConfig") and defaulting to an empty dict (or validating its type) before using .get, then proceed to update os.environ using that safe dict when setting ACCOUNT_ID, AUTH_TOKEN, ORGANIZATION_ID, and CLUSTERS; reference authenticate_jupyterlite and the apiConfig variable when making this change.src/py/mat3ra/notebooks_utils/primitive/logger.py-26-33 (1)
26-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize
VERBOSEto a real boolean before branching.Line 26 can return strings (e.g.,
"false"), which are truthy and incorrectly enable logging.Proposed fix
- should_log = caller_globals.get("VERBOSE", os.environ.get("VERBOSE", True)) + raw_verbose = caller_globals.get("VERBOSE", os.environ.get("VERBOSE", True)) + if isinstance(raw_verbose, str): + should_log = raw_verbose.strip().lower() in {"1", "true", "yes", "on"} + else: + should_log = bool(raw_verbose)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/primitive/logger.py` around lines 26 - 33, The VERBOSE value read from caller_globals or os.environ may be a string (e.g., "false") so normalize it to a real boolean before using should_log; update the logic that sets should_log (the code reading caller_globals.get("VERBOSE", os.environ.get("VERBOSE", True))) to coerce strings like "false"/"0"/"no" to False and "true"/"1"/"yes" to True (case‑insensitive) so subsequent branches around should_log, and the printing behavior that uses level and message, behave correctly.pyproject.toml-41-42 (1)
41-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the
workflowsextra self-dependency typo.Line 41 uses
mar3ra-notebooks-utils[api](with 'r'); should bemat3ra-notebooks-utils[api](with 't') to match the project name.Proposed fix
- "mar3ra-notebooks-utils[api]", + "mat3ra-notebooks-utils[api]",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` around lines 41 - 42, Replace the misspelled extra dependency string "mar3ra-notebooks-utils[api]" with the correct project package name "mat3ra-notebooks-utils[api]" in pyproject.toml so the workflows extra references the proper self-dependency; locate the dependency list that contains the quoted string and update that token only.src/py/mat3ra/notebooks_utils/ipython/ui.py-29-33 (1)
29-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
display_JSONbreaks for string/scalar inputs and can inject unsafe JS.At Line 29–33 and Line 45–49, non-dict/list values are embedded directly into script source. A plain string becomes invalid JS (
renderjson(abc)) and crafted content can break out of the call.Proposed fix
def display_JSON(obj, interactive_viewer: bool = use_interactive_JSON_viewer, level: int = 2) -> None: @@ - if isinstance(obj, (dict, list)): - json_str = json.dumps(obj) - else: - json_str = obj + json_payload = json.dumps(obj) @@ - f"<script>{js} " - f"renderjson.set_show_to_level({str(level)}); " + f"<script>{js} " + f"const data = JSON.parse({json.dumps(json_payload)}); " + f"renderjson.set_show_to_level({level}); " f'renderjson.set_icons("▸","▾"); ' - f'document.getElementById("{id}").appendChild(renderjson({json_str}))</script>' + f'document.getElementById("{id}").appendChild(renderjson(data))</script>' ) )Also applies to: 45-49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/ipython/ui.py` around lines 29 - 33, The display_JSON function currently embeds non-dict/list values directly into the generated script which produces invalid JS for strings and allows injection; always serialize the value with json.dumps (so strings become quoted JSON string literals and other scalars become valid JSON literals) and use that serialized JSON in the renderjson/JS snippet instead of raw obj; apply the same change to the other occurrence (the second block that builds a script around renderjson) so every value passed into renderjson is json.dumps(value) to ensure valid, safe JS embedding.src/py/mat3ra/notebooks_utils/ipython/packages.py-36-38 (1)
36-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing notebook
namecan crash package selection.At Line 37,
re.search(cfg.get("name"), notebook_name_pattern)raisesTypeErrorwhennameis absent/null in config.Proposed fix
- matching_notebook_requirements_list = [ - cfg for cfg in requirements_dict.get("notebooks", []) if re.search(cfg.get("name"), notebook_name_pattern) - ] + matching_notebook_requirements_list = [] + for cfg in requirements_dict.get("notebooks", []): + pattern = cfg.get("name") + if not pattern: + continue + if re.search(pattern, notebook_name_pattern): + matching_notebook_requirements_list.append(cfg)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/ipython/packages.py` around lines 36 - 38, matching_notebook_requirements_list can crash when a config lacks "name"; before calling re.search use a safe guard: retrieve name = cfg.get("name") and skip configs where name is falsy or not a string (or use cfg.get("name", "")), then call re.search(name, notebook_name_pattern); update the comprehension or replace it with an explicit loop that filters out missing/None names so re.search is never called with None (refer to matching_notebook_requirements_list, requirements_dict, and notebook_name_pattern).src/py/mat3ra/notebooks_utils/ipython/io.py-19-29 (1)
19-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnsafe JS string interpolation in download script.
At Line 20–21,
contentandfilenameare inserted into template literals without escaping. Backticks or${...}in data can break the script or inject code.Proposed fix
- js_code = f""" - var content = `{content_str}`; - var filename = `{filename}`; + js_code = f""" + var content = {json.dumps(content_str)}; + var filename = {json.dumps(filename)}; var blob = new Blob([content], {{ type: 'application/json' }}); var link = document.createElement('a'); - link.href = window.URL.createObjectURL(blob); + var objectUrl = window.URL.createObjectURL(blob); + link.href = objectUrl; link.download = filename; document.body.appendChild(link); link.click(); document.body.removeChild(link); + window.URL.revokeObjectURL(objectUrl); """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/ipython/io.py` around lines 19 - 29, The JS template in the js_code assignment injects raw content_str and filename into backtick template literals (var content = `{content_str}`; var filename = `{filename}`;) which is unsafe; change it to embed safely-escaped string literals by serializing the Python values into JS string literals (e.g., use json.dumps(content_str) and json.dumps(filename) when building js_code) or alternatively pass them through encodeURIComponent/decodeURIComponent, so the generated lines become something like var content = <serialized_content>; var filename = <serialized_filename>; ensuring backticks, ${}, newlines and quotes are escaped; update the code that builds js_code in ipython/io.py (the js_code variable) accordingly.src/py/mat3ra/notebooks_utils/ipython/ui.py-141-143 (1)
141-143:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
get_viewer_htmldirectly interpolates untrusted values into HTML.At Line 141–143,
title,div_id, and style text are inserted raw, which allows HTML/attribute injection when callers pass user-derived values.Proposed fix
+import html @@ def get_viewer_html(div_id, width, height=None, title="Viewer", custom_styles=""): @@ + safe_title = html.escape(str(title)) + safe_div_id = html.escape(str(div_id), quote=True) + safe_styles = html.escape(f"{size_style} {custom_styles}".strip(), quote=True) return f""" - <h2>{title}</h2> - <div id="{div_id}" style="{size_style} {custom_styles}"></div> + <h2>{safe_title}</h2> + <div id="{safe_div_id}" style="{safe_styles}"></div> """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/ipython/ui.py` around lines 141 - 143, The HTML generation in get_viewer_html interpolates untrusted title, div_id, size_style, and custom_styles directly; fix it by HTML-escaping text content and validating/escaping attribute values: call html.escape(title) for the <h2> text, validate div_id against a safe pattern (e.g. only [A-Za-z0-9_-]) and fallback or html.escape(div_id) for the id attribute, and do not insert raw style strings—either construct size_style/custom_styles from a safe dict/whitelist of CSS properties or fully escape them before interpolation; update the get_viewer_html implementation to use these sanitized/validated values when composing the returned HTML string.src/py/mat3ra/notebooks_utils/ipython/plot/_plotly.py-137-143 (1)
137-143:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPlot the marker at the selected point’s actual z value.
This always uses
np.min(z_matrix)for the marker height, even whenoptimal_pointrefers to some other(x, y)coordinate. That can place the 3D annotation at the wrong location. Derivez_optfrom the same grid cell asoptimal_point, or accept(x, y, z)explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/ipython/plot/_plotly.py` around lines 137 - 143, The marker z value is wrong because z_opt is taken as np.min(z_matrix) instead of the z at the selected (x,y); update the logic in the block that handles optimal_point so z_opt is derived from the corresponding grid cell (or allow optimal_point to be an (x,y,z) triple). For example, if you have x_vals and y_vals grids, compute ix = np.abs(x_vals - x_opt).argmin() and iy = np.abs(y_vals - y_opt).argmin() (or use np.searchsorted if arrays are monotonic) and set z_opt = z_matrix[ix, iy] (matching z_matrix’s indexing), then call fig.add_trace(go.Scatter3d(..., z=[z_opt], ...)); alternatively accept optimal_point as (x,y,z) and use the provided z directly.src/py/mat3ra/notebooks_utils/ipython/plot/_plotly.py-100-116 (1)
100-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast when
value_attris missing, and clear output before printing.If
value_attris absent,getattr(..., None)flows intoprint_format.format(step, value)and crashes with aTypeErrorfrom{:.4f}instead of telling the caller the attribute lookup failed. Also, the current print happens right beforeclear_output(wait=True), so the progress line is immediately erased.Proposed fix
if callable(value_getter): value = value_getter() elif value_attr: - value = getattr(dynamic_object, value_attr, None) + if not hasattr(dynamic_object, value_attr): + raise AttributeError(f"{type(dynamic_object).__name__} has no attribute '{value_attr}'") + value = getattr(dynamic_object, value_attr) else: raise ValueError("Either value_getter (function) or value_attr (object attribute) must be provided.") + + if value is None: + raise ValueError(f"Retrieved value for '{value_attr}' is None") steps.append(step) values.append(value) - print(print_format.format(step, value)) + clear_output(wait=True) + print(print_format.format(step, value)) figure.data[0].x = steps figure.data[0].y = values - clear_output(wait=True) render_figure(figure)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/ipython/plot/_plotly.py` around lines 100 - 116, When extracting the metric value in the loop, fail fast if value_attr is provided but missing on dynamic_object: replace the getattr(..., None) flow in the branch that checks value_attr with an explicit check (hasattr or getattr with an explicit sentinel) and raise a clear AttributeError including value_attr if the attribute is absent; also move clear_output(wait=True) to before the print(print_format.format(...)) so the printed progress line is not immediately erased. Ensure you reference the same symbols: value_getter, value_attr, dynamic_object, print_format, clear_output, render_figure, steps, values, and figure when making the change.src/py/mat3ra/notebooks_utils/ipython/plot/_plotly.py-21-59 (1)
21-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the point arrays before indexing.
create_scatter_plot_2dassumesx_values,y_values,hover_texts, andtrace_namesare all the same length. Any mismatch currently fails mid-loop with anIndexError, which is hard to diagnose from notebook code. Fail fast with aValueErrorat the top of the helper.Proposed fix
def create_scatter_plot_2d( x_values: List[Union[float, int]], y_values: List[Union[float, int]], hover_texts: List[str], settings: Dict[str, Any], trace_names: Optional[List[str]] = None, ) -> go.Figure: @@ - data = [] + expected_len = len(x_values) + if len(y_values) != expected_len or len(hover_texts) != expected_len: + raise ValueError("x_values, y_values, and hover_texts must have the same length") + if trace_names is not None and len(trace_names) != expected_len: + raise ValueError("trace_names must have the same length as x_values") + + data = [] for i in range(len(x_values)):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/ipython/plot/_plotly.py` around lines 21 - 59, In create_scatter_plot_2d validate that x_values, y_values, and hover_texts all have the same length and, if trace_names is provided, that it matches that same length; if any lengths differ raise a ValueError with a clear message (e.g. indicate the mismatched lengths and the expected length) before entering the loop so the function fails fast; update the checks at the top of create_scatter_plot_2d (referencing the function name and the trace_names parameter) and keep the rest of the plotting logic unchanged.src/py/mat3ra/notebooks_utils/core/entity/job/api.py-110-118 (1)
110-118:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis only creates one job for non-multimaterial workflows.
The docstring says this "Creates jobs for each material", but both branches build a single config and call
api_client.jobs.create(config)once. In the non-multimaterial path, every material aftermaterial_dicts[0]is silently dropped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/core/entity/job/api.py` around lines 110 - 118, The non-multimaterial branch currently builds a single config using only material_dicts[0] and calls api_client.jobs.create once, dropping the remaining materials; change it to iterate over material_dicts when is_multimaterial is False (or when not is_multimaterial) and create a separate config per material (set config["_material"] to {"_id": m["_id"]} for each m) and call api_client.jobs.create for each generated config (preserving the compute assignment), rather than returning after a single create; reference the variables is_multimaterial, material_dicts, config, compute and the call api_client.jobs.create to locate where to add the loop and per-material creation.src/py/mat3ra/notebooks_utils/io.py-17-20 (1)
17-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate the backend result from
get_data().Both branches discard the delegated return value, so every caller gets
Noneeven when data was loaded successfully.💡 Suggested fix
if ENVIRONMENT == EnvironmentsEnum.PYODIDE: - get_data_pyodide(key, globals_dict) + return get_data_pyodide(key, globals_dict) elif ENVIRONMENT == EnvironmentsEnum.PYTHON: - get_data_python(key, globals_dict) + return get_data_python(key, globals_dict)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/io.py` around lines 17 - 20, The get_data function currently calls get_data_pyodide(key, globals_dict) or get_data_python(key, globals_dict) but discards their return values, causing callers to always receive None; modify get_data to return the delegated call's result (i.e., return get_data_pyodide(...) when ENVIRONMENT == EnvironmentsEnum.PYODIDE and return get_data_python(...) when ENVIRONMENT == EnvironmentsEnum.PYTHON) so the backend-loaded data or error is propagated to callers.src/py/mat3ra/notebooks_utils/core/entity/job/api.py-99-100 (1)
99-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid mutating the caller's workflow dict.
When
workflowis already adict,pop("_id", None)removes_idfrom the original object, so reusing that workflow after this call sees modified state.💡 Suggested fix
- job_workflow_dict = workflow.to_dict() if isinstance(workflow, Workflow) else workflow + job_workflow_dict = workflow.to_dict() if isinstance(workflow, Workflow) else dict(workflow) job_workflow_dict.pop("_id", None)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/core/entity/job/api.py` around lines 99 - 100, The code mutates the caller's dict when workflow is already a dict: change the assignment for job_workflow_dict so that when workflow is a dict you create a shallow copy (e.g., via dict(workflow) or workflow.copy()) before calling job_workflow_dict.pop("_id", None); keep the existing path using Workflow.to_dict() unchanged so only the dict branch is copied and the original caller object is not modified (refer to variables job_workflow_dict and workflow and the Workflow class).src/py/mat3ra/notebooks_utils/auth.py-21-27 (1)
21-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep JupyterLite auth inside the Pyodide branch.
In a normal Python notebook, any preexisting
data_from_hostglobal will route throughauthenticate_jupyterlite()here even though the runtime is not Pyodide.💡 Suggested fix
if ENVIRONMENT == EnvironmentsEnum.PYODIDE: get_data("data_from_host", globals_dict) - data_from_host = globals_dict.get("data_from_host") - if data_from_host: - await authenticate_jupyterlite(data_from_host) - elif ACCESS_TOKEN_ENV_VAR not in os.environ or force: + data_from_host = globals_dict.get("data_from_host") + if data_from_host: + await authenticate_jupyterlite(data_from_host) + return + + if ACCESS_TOKEN_ENV_VAR not in os.environ or force: await authenticate_oidc()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/auth.py` around lines 21 - 27, The current flow calls authenticate_jupyterlite when a global data_from_host exists even outside Pyodide; restrict the JupyterLite/auth-via-host logic to the Pyodide branch by moving the get_data, data_from_host check, and await authenticate_jupyterlite(...) inside the if ENVIRONMENT == EnvironmentsEnum.PYODIDE block (keep using get_data, globals_dict and data_from_host), and only if not in Pyodide fall through to the ACCESS_TOKEN_ENV_VAR/force check that calls await authenticate_oidc(); ensure references to ENVIRONMENT, EnvironmentsEnum.PYODIDE, get_data, globals_dict, data_from_host, authenticate_jupyterlite, ACCESS_TOKEN_ENV_VAR, force, and authenticate_oidc are preserved.src/py/mat3ra/notebooks_utils/core/entity/job/api.py-57-71 (1)
57-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop polling when there is nothing left to poll.
If
job_idsis empty, or the API returns no matching jobs,statusesbecomes[]and this returnsTrue, so the polling decorator never terminates.💡 Suggested fix
def wait_for_jobs_to_finish_async(endpoint: JobEndpoints, job_ids: List[str]) -> bool: @@ + if not job_ids: + return False + statuses = get_jobs_statuses_by_ids(endpoint, job_ids) + if not statuses: + raise ValueError("No jobs were found for the provided job IDs.") + counts = Counter(statuses) @@ - return not statuses or any(status in active_statuses for status in statuses) + return any(status in active_statuses for status in statuses)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/core/entity/job/api.py` around lines 57 - 71, The function currently treats an empty statuses list as "still active" causing polling to never stop; update the logic after calling get_jobs_statuses_by_ids so that if job_ids is empty or statuses is empty it returns False (stop polling). Concretely, in the block that computes statuses and active_statuses (referencing get_jobs_statuses_by_ids and the active_statuses set), add an explicit check like "if not statuses: return False" before evaluating any(status in active_statuses for status in statuses) so only non-empty statuses can keep polling.src/py/mat3ra/notebooks_utils/core/io.py-65-69 (1)
65-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate URL schemes and add timeout to prevent unauthorized file access and network hangs.
urllib.request.urlopen()supportsfile://,ftp://, and other non-HTTP schemes, allowing a notebook-supplied URL to read local files or access unintended protocols. The function also lacks a timeout, which can cause indefinite hangs on unresponsive servers.Restrict to HTTP(S) schemes and add a timeout parameter:
Suggested fix
+from urllib.parse import urlparse + def read_from_url_python(url: str, as_bytes: bool = False): - with urllib.request.urlopen(url) as response: + parsed = urlparse(url) + if parsed.scheme not in {"http", "https"}: + raise ValueError(f"Unsupported URL scheme: {parsed.scheme!r}") + + with urllib.request.urlopen(url, timeout=10) as response: body = response.read() if as_bytes: return body return body.decode("utf-8")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/core/io.py` around lines 65 - 69, Validate the provided url's scheme using urllib.parse.urlparse and only allow "http" or "https" (raise a ValueError for other schemes such as "file" or "ftp"), add a timeout parameter (e.g., timeout: float = 10.0) to the function signature, and pass that timeout into urllib.request.urlopen(url, timeout=timeout) to avoid indefinite hangs; update references to the url, as_bytes, and urllib.request.urlopen call in the function to enforce these changes.src/py/mat3ra/notebooks_utils/core/entity/property/api.py-19-23 (1)
19-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid unnecessary job fetch on non-fermi property requests.
At Line 19,
client.jobs.get(job_id)is called even when no fermi enrichment is needed. This adds avoidable latency and an extra failure point on the common path.Proposed fix
- job = client.jobs.get(job_id) properties = client.properties.get_for_job(job_id, property_name) if property_name not in FERMI_ENERGY_PROPERTIES: return properties + job = client.jobs.get(job_id)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/core/entity/property/api.py` around lines 19 - 23, The code currently always calls client.jobs.get(job_id) (job = client.jobs.get) before checking whether property_name is in FERMI_ENERGY_PROPERTIES, adding latency and failure surface; change the control flow so you first call client.properties.get_for_job(job_id, property_name) and only if property_name is in FERMI_ENERGY_PROPERTIES call get_fermi_energy_flowchart_id(job) — which means moving or deferring the client.jobs.get(job_id) call until after the FERMI_ENERGY_PROPERTIES check (or fetching the job lazily inside get_fermi_energy_flowchart_id) so non-fermi property requests avoid the extra job fetch.src/py/mat3ra/notebooks_utils/core/entity/material/io.py-105-108 (1)
105-108:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPer-item fallback is needed when metadata parsing fails.
At Line 105, the current try/except is all-or-nothing: one invalid item downgrades parsing for the entire batch. That can silently drop build metadata for valid entries.
Proposed fix
- try: - materials = [MaterialWithBuildMetadata.create(item) for item in data_from_host] - except Exception: - materials = [Material.create(item) for item in data_from_host] + materials = [] + for item in data_from_host: + try: + materials.append(MaterialWithBuildMetadata.create(item)) + except Exception: + materials.append(Material.create(item))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/core/entity/material/io.py` around lines 105 - 108, The current try/except around the whole batch causes a single bad item to force all entries to be parsed with Material.create; instead iterate over data_from_host and for each item attempt MaterialWithBuildMetadata.create(item) and on exception fall back to Material.create(item) (optionally logging the error), assigning the resulting list to materials so valid items keep their build metadata while only the failing items are downgraded.src/py/mat3ra/notebooks_utils/core/entity/workflow/api.py-38-39 (1)
38-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle missing bank workflow and fix return contract mismatch.
At Line 38, direct
[0]access can throwIndexErrorwhen nosystemNamematch exists. Also, the function is annotated/documented as returningdictbut currently returns a workflow ID string.Proposed fix
-def copy_bank_workflow_by_system_name(endpoint: BankWorkflowEndpoints, system_name: str, account_id: str) -> dict: +def copy_bank_workflow_by_system_name(endpoint: BankWorkflowEndpoints, system_name: str, account_id: str) -> str: @@ - bank_workflow_id = endpoint.list({"systemName": system_name})[0]["_id"] - return endpoint.copy(bank_workflow_id, account_id)["_id"] + bank_workflows = endpoint.list({"systemName": system_name}) + if not bank_workflows: + raise ValueError(f"Bank workflow with systemName '{system_name}' was not found") + copied = endpoint.copy(bank_workflows[0]["_id"], account_id) + return copied["_id"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/core/entity/workflow/api.py` around lines 38 - 39, The code directly indexes endpoint.list({"systemName": system_name})[0] which will raise IndexError when no bank workflow exists and then returns endpoint.copy(... )["_id"] (a string) despite the function being documented/annotated to return a dict; change it to first capture the list result from endpoint.list, check if it's empty and raise a clear exception or return a suitable dict/None, and then call endpoint.copy(bank_workflow_id, account_id) and return the full dict from that call (not just ["_id"]) so the function's return contract matches its annotation; refer to identifiers bank_workflow_id, endpoint.list and endpoint.copy when making the fix.src/py/mat3ra/notebooks_utils/core/entity/material/io.py-137-140 (1)
137-140:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard single-file loading against missing folders and invalid JSON.
At Line 137,
os.listdir(folder_path)can raise if the folder is missing; at Line 140, invalid JSON will raise and stop the lookup instead of continuing to fallback matching.Proposed fix
def load_material_from_folder(folder_path: str, name: str, verbose: bool = True) -> Optional[Any]: @@ name_lower = name.lower() resulting_material = None + + if not os.path.isdir(folder_path): + log(f"Folder '{folder_path}' does not exist.", SeverityLevelEnum.ERROR, force_verbose=verbose) + return None for filename in sorted(os.listdir(folder_path)): if filename.endswith(".json") and name_lower in os.path.splitext(filename)[0].lower(): - with open(os.path.join(folder_path, filename), "r") as file: - data = json.load(file) + file_path = os.path.join(folder_path, filename) + try: + with open(file_path, "r") as file: + data = json.load(file) + except (json.JSONDecodeError, OSError) as error: + log( + f"Skipping invalid JSON file '{file_path}': {error}", + SeverityLevelEnum.WARNING, + force_verbose=verbose, + ) + continue try: resulting_material = MaterialWithBuildMetadata.create(data) except Exception: resulting_material = Material.create(data) break🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/core/entity/material/io.py` around lines 137 - 140, The loop over sorted(os.listdir(folder_path)) should be guarded: verify folder_path exists/isdir or wrap os.listdir in a try/except (OSError/FileNotFoundError) and skip the loop if missing; while opening/parsing each file (the with open(...)/json.load call that uses folder_path, filename and name_lower) catch JSONDecodeError and file I/O errors, log or ignore them and continue to the next file rather than letting an exception abort the lookup, and only return/accept data when json.load succeeds without error.
🟡 Minor comments (8)
src/py/mat3ra/notebooks_utils/pyodide/io.py-87-91 (1)
87-91:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winText mode write will fail if
file_contentis bytes.When
modedoesn't contain"b"(text mode), butfile_contentisbytes(e.g., fromio.BytesIO), thefile.write()call will raiseTypeError: write() argument must be str, not bytes.🛡️ Proposed fix
if "b" in mode and isinstance(file_content, str): file_content = file_content.encode("utf-8") + elif "b" not in mode and isinstance(file_content, bytes): + file_content = file_content.decode("utf-8") with open(file_name, mode) as file: file.write(file_content)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/pyodide/io.py` around lines 87 - 91, The write path can still pass bytes into text mode; update the code around file_content/mode handling so both conversions are handled: if "b" in mode and isinstance(file_content, str) then encode to UTF-8 (existing), else if "b" not in mode and isinstance(file_content, (bytes, bytearray)) then decode to UTF-8 before opening/writing; reference the variables file_content, mode, file_name and the open(...).write(...) call so the write receives a str in text mode and bytes in binary mode.src/py/mat3ra/notebooks_utils/pyodide/packages/install.py-32-38 (1)
32-38:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
micropipmay beNoneeven whensys.platform == "emscripten".If
micropipimport failed (line 10 sets it toNone), callingmicropip.install()on line 36 raisesAttributeError. While typicallymicropipshould be available on emscripten, defensive checking would be safer.🛡️ Proposed fix
async def install_init(): - if sys.platform != "emscripten": + if sys.platform != "emscripten" or micropip is None: return await micropip.install(PYODIDE_INIT_PACKAGES)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/pyodide/packages/install.py` around lines 32 - 38, The install_init function assumes micropip is present but micropip can be None even on emscripten; update install_init to guard against that by checking micropip is not None before calling micropip.install(PYODIDE_INIT_PACKAGES) and before importing modules, and if micropip is None either return early or log a warning/error; refer to the install_init function and the symbols micropip, PYODIDE_INIT_PACKAGES, PYODIDE_INIT_MODULES, and importlib.import_module when making the change.src/py/mat3ra/notebooks_utils/pyodide/packages/install.py-41-63 (1)
41-63:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd guard for
micropipbeingNone.Similar to
install_init, this function should verifymicropipis available before callingmicropip.install().🛡️ Proposed fix
async def install_package_pyodide(pkg: str, verbose: bool = True): """ Install a package in a Pyodide environment. ... """ + if micropip is None: + raise RuntimeError("micropip is not available; cannot install packages outside Pyodide") + if pkg.startswith("nodeps:"):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/pyodide/packages/install.py` around lines 41 - 63, install_package_pyodide currently calls micropip.install without ensuring micropip is available; add a guard before the call to micropip.install in install_package_pyodide that checks if the module-level micropip is None and if so either raise a clear RuntimeError or log an error and return (matching install_init behavior) so you don't await on None; reference the micropip symbol and the install_package_pyodide function and place the guard immediately before the existing await micropip.install(...) call.src/py/mat3ra/notebooks_utils/README.md-7-9 (1)
7-9:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd fence languages to satisfy markdownlint MD040.
Several fenced blocks are missing a language tag, which triggers lint warnings.
Proposed fix
-``` +```text primitive/ → core/ → ipython/ → pyodide/-
+text
core/api/ — Platform credentials and OIDC auth.
...-``` +```text ipython/ui.py — Generic cell output: display_JSON, image grid, viewer HTML/JS. ...-
+text
pyodide/io.py — JS data bridge: kernel↔host shared state, file writes.
...-``` +```text primitive/ → (nothing from this package) ...</details> Also applies to: 26-34, 40-47, 53-59, 65-71 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@src/py/mat3ra/notebooks_utils/README.mdaround lines 7 - 9, The README.md
has multiple fenced code blocks lacking language tags (e.g., the lines
containing "primitive/ → core/ → ipython/ → pyodide/" and the blocks
starting with "core/api/", "ipython/ui.py", "pyodide/io.py", and the
"primitive/" summary); update each triple-backtick fence to include a language
(use "text") for every fenced block in those sections (also apply the same
change for the other affected ranges mentioned: 26-34, 40-47, 53-59, 65-71) so
markdownlint MD040 warnings are resolved.</details> </blockquote></details> <details> <summary>src/py/mat3ra/notebooks_utils/ipython/ui.py-112-117 (1)</summary><blockquote> `112-117`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Guard `max_columns` against zero/negative values.** At Line 112, `100 / max_columns` raises on `0` and produces invalid layouts for negatives. <details> <summary>Proposed fix</summary> ```diff def create_responsive_image_grid(image_tuples, max_columns=3): @@ + if max_columns < 1: + raise ValueError("max_columns must be >= 1") items = [🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/ipython/ui.py` around lines 112 - 117, The code computes column_width using 100 / max_columns which will crash or produce invalid CSS when max_columns is zero or negative; update the logic around where column_width and the GridBox layout are created (references: column_width variable, max_columns, and the widgets.GridBox/widgets.Layout call) to guard max_columns by clamping it to a minimum of 1 (e.g., if max_columns <= 0 then set safe_max = 1) before performing the division and constructing grid_template_columns so the layout string is always valid.src/py/mat3ra/notebooks_utils/ipython/entity/material/visualize.py-121-130 (1)
121-130:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
material_entry["material"]type at the boundary.At Line 121–130, non-
Materialvalues are accepted and fail later in rendering/conversion paths with less actionable errors.Proposed fix
elif isinstance(material_entry, dict) and "material" in material_entry: material = material_entry["material"] + if not isinstance(material, Material): + raise ValueError("material_entry['material'] must be a Material instance") properties = MaterialViewProperties( title=material_entry.get("title", default_properties.title), repetitions=material_entry.get("repetitions", default_properties.repetitions), rotation=material_entry.get("rotation", default_properties.rotation), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/ipython/entity/material/visualize.py` around lines 121 - 130, The code accepts material_entry["material"] without type-checking which causes later cryptic failures; update the branch that handles dict entries to validate that material_entry["material"] is an instance of the expected Material class (or acceptable material type) before creating properties—use isinstance(material_entry["material"], Material) and if it fails raise a TypeError (or ValueError) with a clear message referencing the key and expected type; keep constructing MaterialViewProperties (title, repetitions, rotation) from material_entry as before and return material and properties only after the type check to fail fast.src/py/mat3ra/notebooks_utils/core/io.py-32-33 (1)
32-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn an empty collection when the uploads folder is missing.
The success path returns a list, but this branch falls through with
None. That type flip will break callers that iterate over the result or expectglobals_dict[key]to be populated consistently.💡 Suggested fix
except FileNotFoundError: print("No data found in the 'uploads' folder.") + if globals_dict is not None: + globals_dict[key] = [] + return []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/core/io.py` around lines 32 - 33, The except FileNotFoundError handler currently prints a message and returns None; change it to return an empty list so the function's success path (which yields a list that callers iterate over or assign into globals_dict[key]) keeps a consistent type. Update the except block in the function in io.py that reads the 'uploads' folder to return [] (and keep or log the print) instead of falling through to None.src/py/mat3ra/notebooks_utils/core/entity/property/api.py-90-91 (1)
90-91:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate subworkflow/unit indices before deep access.
At Line 90, invalid indices currently raise raw
IndexError/KeyError. Converting this to a clearValueErrorwill make caller failures actionable.Proposed fix
- unit_flowchart_id = job["workflow"]["subworkflows"][subworkflow_index]["units"][unit_index]["flowchartId"] + try: + unit_flowchart_id = job["workflow"]["subworkflows"][subworkflow_index]["units"][unit_index]["flowchartId"] + except (KeyError, IndexError, TypeError) as error: + raise ValueError( + f"Invalid workflow indices: subworkflow_index={subworkflow_index}, unit_index={unit_index}" + ) from error return endpoint.get_property(job["_id"], unit_flowchart_id, property_name)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/core/entity/property/api.py` around lines 90 - 91, The code directly indexes into job["workflow"]["subworkflows"][subworkflow_index]["units"][unit_index] which can raise raw IndexError/KeyError; add explicit validation before computing unit_flowchart_id: verify "workflow" and "subworkflows" keys exist, that subworkflow_index is within range of job["workflow"]["subworkflows"], and that unit_index is within range of that subworkflow's "units"; if any check fails raise a ValueError with a clear message including job["_id"], subworkflow_index, unit_index and property_name; only after these validations compute unit_flowchart_id and call endpoint.get_property(job["_id"], unit_flowchart_id, property_name).
🧹 Nitpick comments (3)
src/py/mat3ra/notebooks_utils/pyodide/runtime.py (1)
11-15: ⚡ Quick winCatch
ImportErrorinstead of bareException.Per static analysis, catching
Exceptionis overly broad and can mask unexpected errors. Since this is guarding an import,ImportErroris the appropriate exception type.♻️ Proposed fix
try: from IPython.display import HTML, display # type: ignore -except Exception: +except ImportError: HTML = None display = None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/pyodide/runtime.py` around lines 11 - 15, Replace the overly-broad except Exception in the import guard with except ImportError so only import failures are caught; specifically update the try/except around "from IPython.display import HTML, display" (symbols HTML and display) to catch ImportError (which also covers ModuleNotFoundError) and leave the fallback assignments HTML = None and display = None unchanged.pyproject.toml (1)
104-120: ⚡ Quick winAlign Ruff target version with project Python baseline.
Black targets
py310while Ruff is stillpy38; keep these in sync withrequires-python = ">=3.10"to avoid inconsistent linting behavior.Proposed fix
[tool.ruff] @@ -target-version = "py38" +target-version = "py310"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` around lines 104 - 120, The Ruff configuration under [tool.ruff] currently sets target-version = "py38" which is inconsistent with the Black setting target-version = ['py310'] and the project's requires-python >=3.10; update the Ruff target-version (the target-version entry in the [tool.ruff] block) to "py310" (or the same list form used by Black) so both linters target the same Python baseline and avoid inconsistent lint rules.src/py/mat3ra/notebooks_utils/ui.py (1)
26-26: ⚡ Quick winExport the routed selector in
__all__.Line 26 omits
select_coordination_threshold, so wildcard imports miss the new top-level API.💡 Suggested change
-__all__ = ["dataframe_to_html", "display_JSON"] +__all__ = ["dataframe_to_html", "display_JSON", "select_coordination_threshold"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/ui.py` at line 26, The module-level __all__ currently only exports "dataframe_to_html" and "display_JSON" but omits the new top-level function select_coordination_threshold; update the __all__ list to include "select_coordination_threshold" so wildcard imports expose the routed selector. Locate the __all__ definition in src/py/mat3ra/notebooks_utils/ui.py and add the symbol "select_coordination_threshold" alongside the existing exported names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c39b2708-86a6-490b-a390-2a818b1f3e99
📒 Files selected for processing (70)
pyproject.tomlsrc/py/mat3ra/__init__.pysrc/py/mat3ra/notebooks_utils/README.mdsrc/py/mat3ra/notebooks_utils/__init__.pysrc/py/mat3ra/notebooks_utils/auth.pysrc/py/mat3ra/notebooks_utils/core/__init__.pysrc/py/mat3ra/notebooks_utils/core/api/__init__.pysrc/py/mat3ra/notebooks_utils/core/api/auth.pysrc/py/mat3ra/notebooks_utils/core/api/settings.jsonsrc/py/mat3ra/notebooks_utils/core/api/settings.pysrc/py/mat3ra/notebooks_utils/core/entity/__init__.pysrc/py/mat3ra/notebooks_utils/core/entity/compute/__init__.pysrc/py/mat3ra/notebooks_utils/core/entity/compute/api.pysrc/py/mat3ra/notebooks_utils/core/entity/job/__init__.pysrc/py/mat3ra/notebooks_utils/core/entity/job/analysis.pysrc/py/mat3ra/notebooks_utils/core/entity/job/api.pysrc/py/mat3ra/notebooks_utils/core/entity/material/__init__.pysrc/py/mat3ra/notebooks_utils/core/entity/material/analysis.pysrc/py/mat3ra/notebooks_utils/core/entity/material/api.pysrc/py/mat3ra/notebooks_utils/core/entity/material/io.pysrc/py/mat3ra/notebooks_utils/core/entity/property/__init__.pysrc/py/mat3ra/notebooks_utils/core/entity/property/api.pysrc/py/mat3ra/notebooks_utils/core/entity/property/job.pysrc/py/mat3ra/notebooks_utils/core/entity/workflow/__init__.pysrc/py/mat3ra/notebooks_utils/core/entity/workflow/api.pysrc/py/mat3ra/notebooks_utils/core/io.pysrc/py/mat3ra/notebooks_utils/io.pysrc/py/mat3ra/notebooks_utils/ipython/__init__.pysrc/py/mat3ra/notebooks_utils/ipython/_collab.pysrc/py/mat3ra/notebooks_utils/ipython/entity/__init__.pysrc/py/mat3ra/notebooks_utils/ipython/entity/material/__init__.pysrc/py/mat3ra/notebooks_utils/ipython/entity/material/plot.pysrc/py/mat3ra/notebooks_utils/ipython/entity/material/visualize.pysrc/py/mat3ra/notebooks_utils/ipython/entity/property/__init__.pysrc/py/mat3ra/notebooks_utils/ipython/entity/property/visualize.pysrc/py/mat3ra/notebooks_utils/ipython/entity/workflow/__init__.pysrc/py/mat3ra/notebooks_utils/ipython/entity/workflow/visualize.pysrc/py/mat3ra/notebooks_utils/ipython/io.pysrc/py/mat3ra/notebooks_utils/ipython/packages.pysrc/py/mat3ra/notebooks_utils/ipython/plot/__init__.pysrc/py/mat3ra/notebooks_utils/ipython/plot/_matplotlib.pysrc/py/mat3ra/notebooks_utils/ipython/plot/_plotly.pysrc/py/mat3ra/notebooks_utils/ipython/ui.pysrc/py/mat3ra/notebooks_utils/ipython/web/renderjson.csssrc/py/mat3ra/notebooks_utils/ipython/web/renderjson.jssrc/py/mat3ra/notebooks_utils/material.pysrc/py/mat3ra/notebooks_utils/packages.pysrc/py/mat3ra/notebooks_utils/plot.pysrc/py/mat3ra/notebooks_utils/primitive/__init__.pysrc/py/mat3ra/notebooks_utils/primitive/enums.pysrc/py/mat3ra/notebooks_utils/primitive/environment.pysrc/py/mat3ra/notebooks_utils/primitive/logger.pysrc/py/mat3ra/notebooks_utils/primitive/prompt.pysrc/py/mat3ra/notebooks_utils/pyodide/__init__.pysrc/py/mat3ra/notebooks_utils/pyodide/api/__init__.pysrc/py/mat3ra/notebooks_utils/pyodide/api/auth.pysrc/py/mat3ra/notebooks_utils/pyodide/io.pysrc/py/mat3ra/notebooks_utils/pyodide/packages/__init__.pysrc/py/mat3ra/notebooks_utils/pyodide/packages/install.pysrc/py/mat3ra/notebooks_utils/pyodide/packages/torch.pysrc/py/mat3ra/notebooks_utils/pyodide/runtime.pysrc/py/mat3ra/notebooks_utils/pyodide/ui.pysrc/py/mat3ra/notebooks_utils/settings.pysrc/py/mat3ra/notebooks_utils/ui.pytests/py/unit/__init__.pytests/py/unit/test_jupyterlite_interrupts.pyutils/api.pyutils/generic.pyutils/jupyterlite.pyutils/visualize.py
💤 Files with no reviewable changes (5)
- utils/generic.py
- utils/visualize.py
- src/py/mat3ra/notebooks_utils/core/api/settings.py
- utils/jupyterlite.py
- utils/api.py
VsevolodX
changed the title
VsevolodX
changed the title
| display(js) | ||
|
|
||
|
|
||
| def process_material_entry( |
Member
There was a problem hiding this comment.
Should be a local function to this file
timurbazhirov
approved these changes
May 15, 2026
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/py/mat3ra/notebooks_utils/pyodide/ui.py (1)
8-8: ⚡ Quick winAlign return type with actual
Nonereturn path.This function can return
None(Line 26), but the signature/docstring declareAny. Please make the return contract explicit withOptional[Any]to avoid misleading callers and type checkers.Proposed fix
async def ui_prompt_select_array_element_by_index_pyodide( array: List[Any], element_name: str = "element", prompt_head: Optional[str] = None -) -> Any: +) -> Optional[Any]: @@ - Returns: - Any: The selected element from the array. + Returns: + Optional[Any]: The selected element from the array, or None for invalid selection.Also applies to: 18-20, 25-26
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/pyodide/ui.py` at line 8, Several functions in ui.py currently annotate their return type as "-> Any" but may return None; update each function signature to "-> Optional[Any]" and add "from typing import Optional" to the module imports, then update corresponding docstrings/type mentions to reflect Optional[Any]; ensure any type checks or callers expecting a non-None value handle None appropriately.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/py/mat3ra/notebooks_utils/core/prompt.py`:
- Around line 15-30: The loop currently breaks on an out-of-range numeric
choice, causing the function to return default_threshold even when that default
isn't valid; change the control flow in the async prompt logic so invalid
numeric choices and ValueError both retry instead of breaking: inside the while
True around input(prompt_text) (referring to coordination_threshold,
coordination_numbers, default_threshold and the await input(...) call) remove
the break in the else branch and replace it with a continue (and likewise ensure
the except ValueError branch continues), and optionally strip the input before
int conversion so the loop only exits when a valid int present in
coordination_numbers is entered.
In `@src/py/mat3ra/notebooks_utils/io.py`:
- Around line 17-20: The current branches for ENVIRONMENT in functions like
get_data (calling get_data_pyodide/get_data_python) and the corresponding
save_data paths silently do nothing for unexpected ENVIRONMENT values; update
both decision blocks to raise a clear exception (e.g., RuntimeError or
ValueError) when ENVIRONMENT is not a member of EnvironmentsEnum, so failures
surface immediately—locate the ENVIRONMENT checks around calls to
get_data_pyodide/get_data_python and save_data_pyodide/save_data_python and
replace the silent no-op else with an explicit error that includes the
unexpected ENVIRONMENT value.
In `@src/py/mat3ra/notebooks_utils/ipython/packages/install.py`:
- Around line 36-38: The list comprehension building
matching_notebook_requirements_list can raise if a requirement cfg lacks "name"
or has an invalid regex; update the construction of
matching_notebook_requirements_list to skip entries where cfg.get("name") is
falsy and to safely test the regex inside a try/except catching re.error (or
pre-compile with try/except) before calling re.search with
notebook_name_pattern; reference the existing variables cfg, requirements_dict,
matching_notebook_requirements_list, and notebook_name_pattern so the fix
filters out missing names and ignores/continues on invalid regexes instead of
letting them crash the import.
- Around line 15-18: The unconditional print call print('To install packages,
run `pip install ".[all]"` in the terminal') ignores the verbose flag; update
the stub so the message is only emitted when verbose is True (or use the module
logger and check verbose before logging). Locate the stub in
src/py/mat3ra/notebooks_utils/ipython/packages/install.py that contains the
print(...) line and wrap it with an if verbose: guard (or replace with
process-aware logging controlled by the verbose parameter) so verbose=False
suppresses the output.
---
Nitpick comments:
In `@src/py/mat3ra/notebooks_utils/pyodide/ui.py`:
- Line 8: Several functions in ui.py currently annotate their return type as "->
Any" but may return None; update each function signature to "-> Optional[Any]"
and add "from typing import Optional" to the module imports, then update
corresponding docstrings/type mentions to reflect Optional[Any]; ensure any type
checks or callers expecting a non-None value handle None appropriately.
🪄 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: f506898b-bc47-434b-8f86-a2bf1d59d500
📒 Files selected for processing (9)
pyproject.tomlsrc/py/mat3ra/notebooks_utils/__init__.pysrc/py/mat3ra/notebooks_utils/core/prompt.pysrc/py/mat3ra/notebooks_utils/io.pysrc/py/mat3ra/notebooks_utils/ipython/packages/__init__.pysrc/py/mat3ra/notebooks_utils/ipython/packages/install.pysrc/py/mat3ra/notebooks_utils/packages.pysrc/py/mat3ra/notebooks_utils/pyodide/ui.pysrc/py/mat3ra/notebooks_utils/ui.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/py/mat3ra/notebooks_utils/ui.py
- src/py/mat3ra/notebooks_utils/packages.py
- pyproject.toml
Comment on lines
+15
to
+30
| coordination_threshold = default_threshold | ||
| coordination_numbers = list(distribution.keys()) | ||
| prompt_text = f"\nCoordination numbers distribution: {distribution}" f"\nEnter coordination threshold value: " | ||
| while True: | ||
| try: | ||
| value_str = await input(prompt_text) # type: ignore | ||
| value = int(value_str) | ||
| if value in coordination_numbers: | ||
| coordination_threshold = value | ||
| break | ||
| else: | ||
| print(f"Invalid value. Please enter one of these coordination numbers: {coordination_numbers}") | ||
| break | ||
| except ValueError: | ||
| print(f"Please enter a valid integer value from: {coordination_numbers}") | ||
| return coordination_threshold |
There was a problem hiding this comment.
Retry on out-of-range values instead of silently returning default.
At Line 27, an invalid numeric choice exits the loop and returns default_threshold, which can produce unintended or invalid output (especially if default_threshold is not in distribution).
Proposed fix
async def select_coordination_threshold_emscripten(distribution: Dict[int, int], default_threshold: int) -> int:
@@
- coordination_threshold = default_threshold
- coordination_numbers = list(distribution.keys())
+ coordination_threshold = default_threshold
+ coordination_numbers = list(distribution.keys())
+ if default_threshold not in distribution:
+ raise ValueError(
+ f"default_threshold ({default_threshold}) must be one of: {coordination_numbers}"
+ )
@@
if value in coordination_numbers:
coordination_threshold = value
break
else:
print(f"Invalid value. Please enter one of these coordination numbers: {coordination_numbers}")
- break
+ continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/py/mat3ra/notebooks_utils/core/prompt.py` around lines 15 - 30, The loop
currently breaks on an out-of-range numeric choice, causing the function to
return default_threshold even when that default isn't valid; change the control
flow in the async prompt logic so invalid numeric choices and ValueError both
retry instead of breaking: inside the while True around input(prompt_text)
(referring to coordination_threshold, coordination_numbers, default_threshold
and the await input(...) call) remove the break in the else branch and replace
it with a continue (and likewise ensure the except ValueError branch continues),
and optionally strip the input before int conversion so the loop only exits when
a valid int present in coordination_numbers is entered.
Comment on lines
+17
to
+20
| if ENVIRONMENT == EnvironmentsEnum.PYODIDE: | ||
| get_data_pyodide(key, globals_dict) | ||
| elif ENVIRONMENT == EnvironmentsEnum.PYTHON: | ||
| get_data_python(key, globals_dict) |
There was a problem hiding this comment.
Fail fast on unsupported environments instead of silently doing nothing.
Line 17 and Line 31 currently allow silent no-op behavior when ENVIRONMENT is unexpected. This can mask configuration bugs and drop I/O operations without error.
Proposed fix
def get_data(key: str, globals_dict: Optional[Dict] = None):
@@
if ENVIRONMENT == EnvironmentsEnum.PYODIDE:
get_data_pyodide(key, globals_dict)
elif ENVIRONMENT == EnvironmentsEnum.PYTHON:
get_data_python(key, globals_dict)
+ else:
+ raise ValueError(f"Unsupported environment: {ENVIRONMENT}")
@@
def set_data(key: str, value: Any):
@@
if ENVIRONMENT == EnvironmentsEnum.PYODIDE:
set_data_pyodide(key, value)
elif ENVIRONMENT == EnvironmentsEnum.PYTHON:
set_data_python(key, value)
+ else:
+ raise ValueError(f"Unsupported environment: {ENVIRONMENT}")Also applies to: 31-34
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/py/mat3ra/notebooks_utils/io.py` around lines 17 - 20, The current
branches for ENVIRONMENT in functions like get_data (calling
get_data_pyodide/get_data_python) and the corresponding save_data paths silently
do nothing for unexpected ENVIRONMENT values; update both decision blocks to
raise a clear exception (e.g., RuntimeError or ValueError) when ENVIRONMENT is
not a member of EnvironmentsEnum, so failures surface immediately—locate the
ENVIRONMENT checks around calls to get_data_pyodide/get_data_python and
save_data_pyodide/save_data_python and replace the silent no-op else with an
explicit error that includes the unexpected ENVIRONMENT value.
Comment on lines
+15
to
+18
| # NOTE: in a regular Python environment packages should be installed via pip, | ||
| # not programmatically from config.yml. Direct user to do so. | ||
| print('To install packages, run `pip install ".[all]"` in the terminal') | ||
|
|
There was a problem hiding this comment.
Respect the verbose flag in the Python install stub.
Line 17 prints unconditionally, so verbose=False has no effect.
Proposed fix
- print('To install packages, run `pip install ".[all]"` in the terminal')
+ if verbose:
+ print('To install packages, run `pip install ".[all]"` in the terminal')📝 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
| # NOTE: in a regular Python environment packages should be installed via pip, | |
| # not programmatically from config.yml. Direct user to do so. | |
| print('To install packages, run `pip install ".[all]"` in the terminal') | |
| # NOTE: in a regular Python environment packages should be installed via pip, | |
| # not programmatically from config.yml. Direct user to do so. | |
| if verbose: | |
| print('To install packages, run `pip install ".[all]"` in the terminal') | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/py/mat3ra/notebooks_utils/ipython/packages/install.py` around lines 15 -
18, The unconditional print call print('To install packages, run `pip install
".[all]"` in the terminal') ignores the verbose flag; update the stub so the
message is only emitted when verbose is True (or use the module logger and check
verbose before logging). Locate the stub in
src/py/mat3ra/notebooks_utils/ipython/packages/install.py that contains the
print(...) line and wrap it with an if verbose: guard (or replace with
process-aware logging controlled by the verbose parameter) so verbose=False
suppresses the output.
Comment on lines
+36
to
+38
| matching_notebook_requirements_list = [ | ||
| cfg for cfg in requirements_dict.get("notebooks", []) if re.search(cfg.get("name"), notebook_name_pattern) | ||
| ] |
There was a problem hiding this comment.
Guard notebook regex matching to avoid hard crashes on malformed config.
Line 37 can raise when name is missing or invalid regex, which breaks package list construction for all notebooks.
Proposed fix
- matching_notebook_requirements_list = [
- cfg for cfg in requirements_dict.get("notebooks", []) if re.search(cfg.get("name"), notebook_name_pattern)
- ]
+ matching_notebook_requirements_list = []
+ for cfg in requirements_dict.get("notebooks", []):
+ pattern = cfg.get("name")
+ if not pattern:
+ continue
+ try:
+ if re.search(pattern, notebook_name_pattern):
+ matching_notebook_requirements_list.append(cfg)
+ except re.error as exc:
+ raise ValueError(f"Invalid notebook regex pattern: {pattern!r}") from exc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/py/mat3ra/notebooks_utils/ipython/packages/install.py` around lines 36 -
38, The list comprehension building matching_notebook_requirements_list can
raise if a requirement cfg lacks "name" or has an invalid regex; update the
construction of matching_notebook_requirements_list to skip entries where
cfg.get("name") is falsy and to safely test the regex inside a try/except
catching re.error (or pre-compile with try/except) before calling re.search with
notebook_name_pattern; reference the existing variables cfg, requirements_dict,
matching_notebook_requirements_list, and notebook_name_pattern so the fix
filters out missing names and ignores/continues on invalid regexes instead of
letting them crash the import.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/py/mat3ra/notebooks_utils/core/entity/job/api.py`:
- Around line 19-24: The code uses next(...) on job_endpoint.list_files(job_id)
which will raise StopIteration if no matching file is found and it opens
urllib.request.urlopen(signed_url) without a context manager; update the logic
around job_endpoint.list_files/job_id to handle the "not found" case explicitly
(e.g., use next((f for f in files if filename_on_cloud in f["key"]), None) and
raise a clear exception or return a helpful error) and wrap the
urllib.request.urlopen(signed_url) response in a context manager (with
urllib.request.urlopen(signed_url) as server_response) before reading to ensure
the response is always closed; refer to variables/function names file_metadata,
signed_url, filename_on_disk when making the changes.
- Around line 78-82: The code assumes material_dicts has at least one item when
setting config["_material"] and config["_materials"], which will raise
IndexError if the list is empty; update the logic around is_multimaterial and
material_dicts to validate that material_dicts is non-empty before accessing
material_dicts[0], and if empty either raise a clear ValueError (or return a
meaningful default such as None or an empty list) and adjust config accordingly
(e.g., set config["_material"]=None and config["_materials"]=[] or raise),
ensuring both the single-material and multimaterial branches in this block
handle the empty list safely.
In `@src/py/mat3ra/notebooks_utils/ipython/entity/material/visualize.py`:
- Around line 121-123: In the branch that handles dict entries (the block
beginning with "elif isinstance(material_entry, dict) and 'material' in
material_entry:"), validate that material_entry["material"] is an instance of
the expected Material class before creating MaterialViewProperties; if it is
not, raise a clear TypeError (or ValueError) with a message referencing
material_entry and the expected Material type so downstream code in
MaterialViewProperties and visualization functions fails fast with a clear
error. Ensure you reference the actual Material type used by your codebase
(e.g., Material) and perform the isinstance check immediately after assigning
material = material_entry["material"].
- Around line 22-37: The function get_material_image currently assumes
repetitions has exactly three elements when building supercell_matrix (used by
make_supercell), which leads to IndexError on bad input; add an input validation
at the start of get_material_image (after to_ase) that ensures repetitions is a
sequence of length 3 with integer (or castable to int) positive values (or
explicitly allow padding behavior if desired), and if not raise a clear
ValueError explaining the expected shape (e.g., "repetitions must be a sequence
of three positive integers"); ensure downstream code uses the
validated/normalized repetitions when constructing
supercell_matrix/material_repeat.
In `@src/py/mat3ra/notebooks_utils/ipython/ui.py`:
- Around line 93-117: The function create_responsive_image_grid computes
column_width using 100 / max_columns without validating max_columns, which can
raise ZeroDivisionError or produce invalid layouts for negative values; add a
guard at the start of create_responsive_image_grid to coerce or validate
max_columns (e.g., if max_columns is None or <= 0, set to 1 or raise a clear
ValueError), then use the sanitized value when computing column_width and
grid_template_columns so column_width and the GridBox layout are always valid.
- Around line 29-49: The code injects raw obj into the inlined <script> which
enables script injection; always serialize/escape the payload with json.dumps
before embedding and parse it in the browser instead of inserting raw text.
Concretely, ensure json_str is produced by json.dumps(obj) for all branches
(replace the else branch that sets json_str = obj), and change the injected call
to pass a safe string and call JSON.parse(...) inside the script when invoking
renderjson (use the existing id/renderjson usage); this guarantees proper JSON
escaping and prevents injection from obj content.
In `@src/py/mat3ra/notebooks_utils/job.py`:
- Line 16: The docstring describing which job statuses are considered finished
is missing "queued" and must match the runtime logic that treats "queued" as
active; update the docstring text in src/py/mat3ra/notebooks_utils/job.py so the
finished-status definition lists "pre-submission", "submitted", "queued", and
"active" consistently (or rephrase to say a job is finished if it is not in
"pre-submission"/"submitted"/"queued"/"active"), ensuring the wording aligns
with the runtime check that treats "queued" as an active state.
- Around line 22-36: The current return expression returns True when statuses is
empty which causes non-terminating polling; change the logic so an empty
statuses list stops polling by returning False when not statuses (e.g., insert
or replace the final return with: if not statuses: return False; otherwise
return any(status in active_statuses for status in statuses)). This touches the
variables statuses and active_statuses in the job polling code in
src/py/mat3ra/notebooks_utils/job.py — update the final return to explicitly
handle the empty-case before evaluating active_statuses.
🪄 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: 5ff08975-1c36-4421-819f-1d250ca044a9
📒 Files selected for processing (7)
src/py/mat3ra/notebooks_utils/auth.pysrc/py/mat3ra/notebooks_utils/core/api/auth.pysrc/py/mat3ra/notebooks_utils/core/entity/job/api.pysrc/py/mat3ra/notebooks_utils/ipython/entity/material/visualize.pysrc/py/mat3ra/notebooks_utils/ipython/ui.pysrc/py/mat3ra/notebooks_utils/job.pysrc/py/mat3ra/notebooks_utils/packages.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/py/mat3ra/notebooks_utils/packages.py
Comment on lines
+19
to
+24
| files = job_endpoint.list_files(job_id) | ||
| file_metadata = next(f for f in files if filename_on_cloud in f["key"]) | ||
| signed_url = file_metadata["signedUrl"] | ||
| server_response = urllib.request.urlopen(signed_url) | ||
| with open(filename_on_disk, "wb") as outp: | ||
| outp.write(server_response.read()) |
There was a problem hiding this comment.
Potential StopIteration if file not found, and response not closed.
next()without a default raisesStopIterationif no file matches, which is confusing for callers.- The
urlopenresponse should use a context manager to ensure proper cleanup.
Proposed fix
- file_metadata = next(f for f in files if filename_on_cloud in f["key"])
- signed_url = file_metadata["signedUrl"]
- server_response = urllib.request.urlopen(signed_url)
- with open(filename_on_disk, "wb") as outp:
- outp.write(server_response.read())
+ file_metadata = next((f for f in files if filename_on_cloud in f["key"]), None)
+ if file_metadata is None:
+ raise FileNotFoundError(f"File '{filename_on_cloud}' not found in job {job_id}")
+ signed_url = file_metadata["signedUrl"]
+ with urllib.request.urlopen(signed_url) as server_response:
+ with open(filename_on_disk, "wb") as outp:
+ outp.write(server_response.read())🧰 Tools
🪛 Ruff (0.15.12)
[error] 22-22: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/py/mat3ra/notebooks_utils/core/entity/job/api.py` around lines 19 - 24,
The code uses next(...) on job_endpoint.list_files(job_id) which will raise
StopIteration if no matching file is found and it opens
urllib.request.urlopen(signed_url) without a context manager; update the logic
around job_endpoint.list_files/job_id to handle the "not found" case explicitly
(e.g., use next((f for f in files if filename_on_cloud in f["key"]), None) and
raise a clear exception or return a helpful error) and wrap the
urllib.request.urlopen(signed_url) response in a context manager (with
urllib.request.urlopen(signed_url) as server_response) before reading to ensure
the response is always closed; refer to variables/function names file_metadata,
signed_url, filename_on_disk when making the changes.
Comment on lines
+78
to
+82
| if is_multimaterial: | ||
| config["_material"] = {"_id": material_dicts[0]["_id"]} | ||
| config["_materials"] = [{"_id": m["_id"]} for m in material_dicts] | ||
| else: | ||
| config["_material"] = {"_id": material_dicts[0]["_id"]} |
There was a problem hiding this comment.
Empty materials list causes IndexError.
If materials is empty, accessing material_dicts[0] at lines 79 or 82 will raise an IndexError. Consider validating the input.
Proposed fix
def create_job(
api_client: APIClient,
materials: List[Union[dict, Material]],
...
) -> Union[dict, List[dict]]:
+ if not materials:
+ raise ValueError("At least one material is required")
material_dicts = [m.to_dict() if isinstance(m, Material) else m for m in materials]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/py/mat3ra/notebooks_utils/core/entity/job/api.py` around lines 78 - 82,
The code assumes material_dicts has at least one item when setting
config["_material"] and config["_materials"], which will raise IndexError if the
list is empty; update the logic around is_multimaterial and material_dicts to
validate that material_dicts is non-empty before accessing material_dicts[0],
and if empty either raise a clear ValueError (or return a meaningful default
such as None or an empty list) and adjust config accordingly (e.g., set
config["_material"]=None and config["_materials"]=[] or raise), ensuring both
the single-material and multimaterial branches in this block handle the empty
list safely.
Comment on lines
+22
to
+37
| def get_material_image(material: Material, title: str, rotation="0x,0y,0z", repetitions=[1, 1, 1]): | ||
| """ | ||
| Returns an image of the material structure with the specified title. | ||
|
|
||
| Args: | ||
| material (Material): Material object to visualize. | ||
| title (str): Title of the image. | ||
| rotation (str): Rotation of the image. | ||
| repetitions (list): Repetitions alongside a,b,c lattice vectors. | ||
|
|
||
| Returns: | ||
| tuple: Tuple containing the image bytes and the title. | ||
| """ | ||
| ase_atoms = to_ase(material) | ||
| supercell_matrix = [[repetitions[0], 0, 0], [0, repetitions[1], 0], [0, 0, repetitions[2]]] | ||
| material_repeat = make_supercell(ase_atoms, supercell_matrix) |
There was a problem hiding this comment.
Validate repetitions shape before indexing.
Line 36 assumes exactly three elements. Invalid input currently fails later with IndexError/bad supercell behavior.
Proposed fix
def get_material_image(material: Material, title: str, rotation="0x,0y,0z", repetitions=[1, 1, 1]):
@@
+ if len(repetitions) != 3 or any(not isinstance(x, int) or x <= 0 for x in repetitions):
+ raise ValueError("`repetitions` must be a list of three positive integers")
+
ase_atoms = to_ase(material)🧰 Tools
🪛 Ruff (0.15.12)
[warning] 22-22: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/py/mat3ra/notebooks_utils/ipython/entity/material/visualize.py` around
lines 22 - 37, The function get_material_image currently assumes repetitions has
exactly three elements when building supercell_matrix (used by make_supercell),
which leads to IndexError on bad input; add an input validation at the start of
get_material_image (after to_ase) that ensures repetitions is a sequence of
length 3 with integer (or castable to int) positive values (or explicitly allow
padding behavior if desired), and if not raise a clear ValueError explaining the
expected shape (e.g., "repetitions must be a sequence of three positive
integers"); ensure downstream code uses the validated/normalized repetitions
when constructing supercell_matrix/material_repeat.
Comment on lines
+121
to
+123
| elif isinstance(material_entry, dict) and "material" in material_entry: | ||
| material = material_entry["material"] | ||
| properties = MaterialViewProperties( |
There was a problem hiding this comment.
Validate material_entry["material"] type early.
This branch accepts any object under "material", then downstream code fails with less clear errors. Reject non-Material values here.
Proposed fix
elif isinstance(material_entry, dict) and "material" in material_entry:
material = material_entry["material"]
+ if not isinstance(material, Material):
+ raise ValueError('`material_entry["material"]` must be a Material instance')
properties = MaterialViewProperties(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/py/mat3ra/notebooks_utils/ipython/entity/material/visualize.py` around
lines 121 - 123, In the branch that handles dict entries (the block beginning
with "elif isinstance(material_entry, dict) and 'material' in material_entry:"),
validate that material_entry["material"] is an instance of the expected Material
class before creating MaterialViewProperties; if it is not, raise a clear
TypeError (or ValueError) with a message referencing material_entry and the
expected Material type so downstream code in MaterialViewProperties and
visualization functions fails fast with a clear error. Ensure you reference the
actual Material type used by your codebase (e.g., Material) and perform the
isinstance check immediately after assigning material =
material_entry["material"].
Comment on lines
+29
to
+49
| if isinstance(obj, (dict, list)): | ||
| json_str = json.dumps(obj) | ||
| else: | ||
| json_str = obj | ||
|
|
||
| id = str(uuid.uuid4()) | ||
|
|
||
| with open(os.path.join(_WEB_DIR, "renderjson.css")) as fp: | ||
| css = fp.read() | ||
|
|
||
| with open(os.path.join(_WEB_DIR, "renderjson.js")) as fp: | ||
| js = fp.read() | ||
|
|
||
| display(HTML(f'<style>{css}</style><div id="{id}"></div>')) | ||
| display( | ||
| HTML( | ||
| f"<script>{js} " | ||
| f"renderjson.set_show_to_level({str(level)}); " | ||
| f'renderjson.set_icons("▸","▾"); ' | ||
| f'document.getElementById("{id}").appendChild(renderjson({json_str}))</script>' | ||
| ) |
There was a problem hiding this comment.
Escape/validate string JSON before injecting it into <script>.
Line 32 passes raw obj through, and Line 48 injects it directly into script content. A crafted string can break rendering or inject script.
Proposed fix
- if isinstance(obj, (dict, list)):
- json_str = json.dumps(obj)
- else:
- json_str = obj
+ if isinstance(obj, str):
+ try:
+ parsed_obj = json.loads(obj)
+ except json.JSONDecodeError as exc:
+ raise ValueError("`obj` must be a dict/list or a valid JSON string") from exc
+ elif isinstance(obj, (dict, list)):
+ parsed_obj = obj
+ else:
+ raise TypeError("`obj` must be a dict/list or a valid JSON string")
+ json_str = json.dumps(parsed_obj)🧰 Tools
🪛 Ruff (0.15.12)
[error] 34-34: Variable id is shadowing a Python builtin
(A001)
[warning] 46-46: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/py/mat3ra/notebooks_utils/ipython/ui.py` around lines 29 - 49, The code
injects raw obj into the inlined <script> which enables script injection; always
serialize/escape the payload with json.dumps before embedding and parse it in
the browser instead of inserting raw text. Concretely, ensure json_str is
produced by json.dumps(obj) for all branches (replace the else branch that sets
json_str = obj), and change the injected call to pass a safe string and call
JSON.parse(...) inside the script when invoking renderjson (use the existing
id/renderjson usage); this guarantees proper JSON escaping and prevents
injection from obj content.
Comment on lines
+93
to
+117
| def create_responsive_image_grid(image_tuples, max_columns=3): | ||
| """ | ||
| Create a responsive image grid. Limits the grid to a maximum of three columns. | ||
|
|
||
| Args: | ||
| image_tuples (list): List of tuples where each tuple contains an image and a title. | ||
| max_columns (int): Maximum number of columns in the grid. | ||
| """ | ||
| items = [ | ||
| widgets.VBox( | ||
| [ | ||
| widgets.Label(value=title, layout=widgets.Layout(height="30px", align_self="center")), | ||
| create_image_widget(image, object_fit="contain"), | ||
| ], | ||
| layout=widgets.Layout(align_items="center", padding="0px 0px 10px 0px"), | ||
| ) | ||
| for image, title in image_tuples | ||
| ] | ||
|
|
||
| column_width = f"minmax(100px, {100 / max_columns}%)" | ||
| grid = widgets.GridBox( | ||
| items, | ||
| layout=widgets.Layout( | ||
| grid_template_columns=f"repeat({max_columns}, {column_width})", | ||
| grid_gap="10px", |
There was a problem hiding this comment.
Guard max_columns against zero/negative values.
Line 112 divides by max_columns without validation, which raises ZeroDivisionError for 0 and produces invalid layout for negatives.
Proposed fix
def create_responsive_image_grid(image_tuples, max_columns=3):
@@
- items = [
+ if max_columns <= 0:
+ raise ValueError("max_columns must be greater than 0")
+
+ items = [📝 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
| def create_responsive_image_grid(image_tuples, max_columns=3): | |
| """ | |
| Create a responsive image grid. Limits the grid to a maximum of three columns. | |
| Args: | |
| image_tuples (list): List of tuples where each tuple contains an image and a title. | |
| max_columns (int): Maximum number of columns in the grid. | |
| """ | |
| items = [ | |
| widgets.VBox( | |
| [ | |
| widgets.Label(value=title, layout=widgets.Layout(height="30px", align_self="center")), | |
| create_image_widget(image, object_fit="contain"), | |
| ], | |
| layout=widgets.Layout(align_items="center", padding="0px 0px 10px 0px"), | |
| ) | |
| for image, title in image_tuples | |
| ] | |
| column_width = f"minmax(100px, {100 / max_columns}%)" | |
| grid = widgets.GridBox( | |
| items, | |
| layout=widgets.Layout( | |
| grid_template_columns=f"repeat({max_columns}, {column_width})", | |
| grid_gap="10px", | |
| def create_responsive_image_grid(image_tuples, max_columns=3): | |
| """ | |
| Create a responsive image grid. Limits the grid to a maximum of three columns. | |
| Args: | |
| image_tuples (list): List of tuples where each tuple contains an image and a title. | |
| max_columns (int): Maximum number of columns in the grid. | |
| """ | |
| if max_columns <= 0: | |
| raise ValueError("max_columns must be greater than 0") | |
| items = [ | |
| widgets.VBox( | |
| [ | |
| widgets.Label(value=title, layout=widgets.Layout(height="30px", align_self="center")), | |
| create_image_widget(image, object_fit="contain"), | |
| ], | |
| layout=widgets.Layout(align_items="center", padding="0px 0px 10px 0px"), | |
| ) | |
| for image, title in image_tuples | |
| ] | |
| column_width = f"minmax(100px, {100 / max_columns}%)" | |
| grid = widgets.GridBox( | |
| items, | |
| layout=widgets.Layout( | |
| grid_template_columns=f"repeat({max_columns}, {column_width})", | |
| grid_gap="10px", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/py/mat3ra/notebooks_utils/ipython/ui.py` around lines 93 - 117, The
function create_responsive_image_grid computes column_width using 100 /
max_columns without validating max_columns, which can raise ZeroDivisionError or
produce invalid layouts for negative values; add a guard at the start of
create_responsive_image_grid to coerce or validate max_columns (e.g., if
max_columns is None or <= 0, set to 1 or raise a clear ValueError), then use the
sanitized value when computing column_width and grid_template_columns so
column_width and the GridBox layout are always valid.
| def wait_for_jobs_to_finish_async(endpoint: JobEndpoints, job_ids: List[str]) -> bool: | ||
| """ | ||
| Waits for jobs to finish and prints their statuses. | ||
| A job is considered finished if it is not in "pre-submission", "submitted", or "active" status. |
There was a problem hiding this comment.
Keep docstring status definition aligned with runtime logic.
Line 16 omits queued, but Line 35 treats it as active. Please update the docstring to match behavior.
Also applies to: 35-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/py/mat3ra/notebooks_utils/job.py` at line 16, The docstring describing
which job statuses are considered finished is missing "queued" and must match
the runtime logic that treats "queued" as active; update the docstring text in
src/py/mat3ra/notebooks_utils/job.py so the finished-status definition lists
"pre-submission", "submitted", "queued", and "active" consistently (or rephrase
to say a job is finished if it is not in
"pre-submission"/"submitted"/"queued"/"active"), ensuring the wording aligns
with the runtime check that treats "queued" as an active state.
Comment on lines
+22
to
+36
| statuses = get_jobs_statuses_by_ids(endpoint, job_ids) | ||
| counts = Counter(statuses) | ||
| headers = ["TIME", "SUBMITTED-JOBS", "ACTIVE-JOBS", "FINISHED-JOBS", "ERRORED-JOBS"] | ||
| now = datetime.datetime.now().strftime("%Y-%m-%d-%H:%M:%S") | ||
| row = [ | ||
| now, | ||
| counts.get("submitted", 0) + counts.get("queued", 0), | ||
| counts.get("active", 0), | ||
| counts.get("finished", 0), | ||
| counts.get("error", 0), | ||
| ] | ||
| pretty_print([row], headers, tablefmt="grid", stralign="center") | ||
|
|
||
| active_statuses = {"pre-submission", "submitted", "queued", "active"} | ||
| return not statuses or any(status in active_statuses for status in statuses) |
There was a problem hiding this comment.
Avoid non-terminating polling on empty status results.
Line 36 currently returns True when statuses is empty, which can keep the polling loop running indefinitely (e.g., empty job_ids or no matched jobs).
Suggested fix
`@interruptible_polling_loop`()
def wait_for_jobs_to_finish_async(endpoint: JobEndpoints, job_ids: List[str]) -> bool:
@@
- statuses = get_jobs_statuses_by_ids(endpoint, job_ids)
+ if not job_ids:
+ return False
+
+ statuses = get_jobs_statuses_by_ids(endpoint, job_ids)
@@
- return not statuses or any(status in active_statuses for status in statuses)
+ return any(status in active_statuses for status in statuses)📝 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
| statuses = get_jobs_statuses_by_ids(endpoint, job_ids) | |
| counts = Counter(statuses) | |
| headers = ["TIME", "SUBMITTED-JOBS", "ACTIVE-JOBS", "FINISHED-JOBS", "ERRORED-JOBS"] | |
| now = datetime.datetime.now().strftime("%Y-%m-%d-%H:%M:%S") | |
| row = [ | |
| now, | |
| counts.get("submitted", 0) + counts.get("queued", 0), | |
| counts.get("active", 0), | |
| counts.get("finished", 0), | |
| counts.get("error", 0), | |
| ] | |
| pretty_print([row], headers, tablefmt="grid", stralign="center") | |
| active_statuses = {"pre-submission", "submitted", "queued", "active"} | |
| return not statuses or any(status in active_statuses for status in statuses) | |
| if not job_ids: | |
| return False | |
| statuses = get_jobs_statuses_by_ids(endpoint, job_ids) | |
| counts = Counter(statuses) | |
| headers = ["TIME", "SUBMITTED-JOBS", "ACTIVE-JOBS", "FINISHED-JOBS", "ERRORED-JOBS"] | |
| now = datetime.datetime.now().strftime("%Y-%m-%d-%H:%M:%S") | |
| row = [ | |
| now, | |
| counts.get("submitted", 0) + counts.get("queued", 0), | |
| counts.get("active", 0), | |
| counts.get("finished", 0), | |
| counts.get("error", 0), | |
| ] | |
| pretty_print([row], headers, tablefmt="grid", stralign="center") | |
| active_statuses = {"pre-submission", "submitted", "queued", "active"} | |
| return any(status in active_statuses for status in statuses) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/py/mat3ra/notebooks_utils/job.py` around lines 22 - 36, The current
return expression returns True when statuses is empty which causes
non-terminating polling; change the logic so an empty statuses list stops
polling by returning False when not statuses (e.g., insert or replace the final
return with: if not statuses: return False; otherwise return any(status in
active_statuses for status in statuses)). This touches the variables statuses
and active_statuses in the job polling code in
src/py/mat3ra/notebooks_utils/job.py — update the final return to explicitly
handle the empty-case before evaluating active_statuses.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/py/mat3ra/notebooks_utils/core/entity/job/api.py (2)
67-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
material_dictsbefore indexing.This remains unresolved: accessing
material_dicts[0]without a guard can raiseIndexErroron empty input.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/core/entity/job/api.py` around lines 67 - 73, The code constructs config using material_dicts[0] without validating material_dicts; add a guard before the config creation (in the function that builds the config where material_dicts is available) to check if material_dicts is non-empty and raise a clear exception (e.g., ValueError) or handle the empty case appropriately, and then use material_dicts[0]["_id"] only after that check so you never index an empty list.
17-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle missing file matches and network resources safely.
This remains unresolved: no-match lookups can raise
StopIteration, and the URL response is not managed with a context manager (plus no scheme guard before opening).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/core/entity/job/api.py` around lines 17 - 22, The code calling job_endpoint.list_files and using next(...) to find file_metadata can raise StopIteration and then opens signed_url with urllib.request.urlopen without a context manager or scheme check; modify the logic in this routine to (1) safely search files from job_endpoint.list_files(job_id) using a loop or next(files_iter, None) and raise a clear exception or return a helpful error if no matching file is found, (2) validate the extracted signed_url begins with "http://" or "https://" before attempting to open it, and (3) open the URL with a context manager (with urllib.request.urlopen(signed_url) as server_response:) and handle urllib.error.HTTPError/URLError or generic exceptions to avoid leaking network resources before writing server_response.read() to filename_on_disk.
🧹 Nitpick comments (1)
pyproject.toml (1)
103-130: ⚡ Quick winAlign Ruff target version with the project Python baseline.
Ruff still targets Python 3.8 while project/runtime tooling is set to 3.10, which creates inconsistent compatibility checks.
Proposed fix
[tool.ruff] ... -target-version = "py38" +target-version = "py310"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` around lines 103 - 130, The Ruff configuration under [tool.ruff] sets target-version = "py38" which conflicts with the project's Python baseline (see [tool.black] python 3.10 and [tool.mypy] python_version = "3.10"); update the [tool.ruff] target-version to "py310" (or match the same canonical value used elsewhere) so Ruff's checks align with the project's Python version and avoid inconsistent diagnostics; edit the target-version key in the [tool.ruff] section to the matching value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyproject.toml`:
- Around line 36-42: The workflows list contains a typo: replace the incorrect
package name "mar3ra-notebooks-utils[api]" with the correct
"mat3ra-notebooks-utils[api]" so the workflows extra matches the other entries
and pip install .[workflows] works; update the entry in the workflows array
accordingly.
In `@src/py/mat3ra/notebooks_utils/core/entity/job/api.py`:
- Around line 64-65: The code mutates the caller-provided job_workflow_dict by
calling job_workflow_dict.pop("_id", None); fix this by making a shallow copy
(e.g., local_workflow = dict(job_workflow_dict)) and perform the pop and
subsequent reads (such as computing is_multimaterial =
local_workflow.get("isMultiMaterial", False)) on that copy so the original
dictionary is not modified; update references in the surrounding function (where
job_workflow_dict is used) to use the copied variable for mutation and
inspection.
---
Duplicate comments:
In `@src/py/mat3ra/notebooks_utils/core/entity/job/api.py`:
- Around line 67-73: The code constructs config using material_dicts[0] without
validating material_dicts; add a guard before the config creation (in the
function that builds the config where material_dicts is available) to check if
material_dicts is non-empty and raise a clear exception (e.g., ValueError) or
handle the empty case appropriately, and then use material_dicts[0]["_id"] only
after that check so you never index an empty list.
- Around line 17-22: The code calling job_endpoint.list_files and using
next(...) to find file_metadata can raise StopIteration and then opens
signed_url with urllib.request.urlopen without a context manager or scheme
check; modify the logic in this routine to (1) safely search files from
job_endpoint.list_files(job_id) using a loop or next(files_iter, None) and raise
a clear exception or return a helpful error if no matching file is found, (2)
validate the extracted signed_url begins with "http://" or "https://" before
attempting to open it, and (3) open the URL with a context manager (with
urllib.request.urlopen(signed_url) as server_response:) and handle
urllib.error.HTTPError/URLError or generic exceptions to avoid leaking network
resources before writing server_response.read() to filename_on_disk.
---
Nitpick comments:
In `@pyproject.toml`:
- Around line 103-130: The Ruff configuration under [tool.ruff] sets
target-version = "py38" which conflicts with the project's Python baseline (see
[tool.black] python 3.10 and [tool.mypy] python_version = "3.10"); update the
[tool.ruff] target-version to "py310" (or match the same canonical value used
elsewhere) so Ruff's checks align with the project's Python version and avoid
inconsistent diagnostics; edit the target-version key in the [tool.ruff] section
to the matching value.
🪄 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: 7d34cfaa-ce67-45fc-9419-57df82335205
📒 Files selected for processing (4)
.github/workflows/cicd.ymlpyproject.tomlsrc/py/mat3ra/notebooks_utils/core/entity/job/api.pysrc/py/mat3ra/notebooks_utils/job.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/py/mat3ra/notebooks_utils/job.py
Comment on lines
+36
to
42
| workflows = [ | ||
| "mat3ra-notebooks-utils[materials]", | ||
| "mat3ra-wode", | ||
| "mat3ra-prode", | ||
| "mat3ra-ide", | ||
| "mat3ra-api-client", | ||
| "mat3ra-standata" | ||
| "mar3ra-notebooks-utils[api]", | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n pyproject.toml | sed -n '30,50p'Repository: Exabyte-io/api-examples
Length of output: 690
Fix the misspelled package name in the workflows extra.
mar3ra-notebooks-utils[api] is a typo (should be mat3ra-notebooks-utils[api]), matching all other instances of this package in the file. This will break pip install .[workflows].
Proposed fix
workflows = [
"mat3ra-notebooks-utils[materials]",
"mat3ra-wode",
"mat3ra-prode",
"mat3ra-ide",
- "mar3ra-notebooks-utils[api]",
+ "mat3ra-notebooks-utils[api]",
]📝 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
| workflows = [ | |
| "mat3ra-notebooks-utils[materials]", | |
| "mat3ra-wode", | |
| "mat3ra-prode", | |
| "mat3ra-ide", | |
| "mat3ra-api-client", | |
| "mat3ra-standata" | |
| "mar3ra-notebooks-utils[api]", | |
| ] | |
| workflows = [ | |
| "mat3ra-notebooks-utils[materials]", | |
| "mat3ra-wode", | |
| "mat3ra-prode", | |
| "mat3ra-ide", | |
| "mat3ra-notebooks-utils[api]", | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyproject.toml` around lines 36 - 42, The workflows list contains a typo:
replace the incorrect package name "mar3ra-notebooks-utils[api]" with the
correct "mat3ra-notebooks-utils[api]" so the workflows extra matches the other
entries and pip install .[workflows] works; update the entry in the workflows
array accordingly.
Comment on lines
+64
to
+65
| job_workflow_dict.pop("_id", None) | ||
| is_multimaterial = job_workflow_dict.get("isMultiMaterial", False) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/py/mat3ra/notebooks_utils/core/entity/job/api.py | head -100Repository: Exabyte-io/api-examples
Length of output: 3700
🏁 Script executed:
# Search for calls to create_job function to understand caller patterns
rg "create_job" --type py -B 2 -A 2Repository: Exabyte-io/api-examples
Length of output: 1373
🏁 Script executed:
# Search for test files related to create_job
fd -i "test.*create.*job\|.*create.*job.*test" --type fRepository: Exabyte-io/api-examples
Length of output: 49
🏁 Script executed:
# Look for test files in general
find . -type f -name "*test*.py" | head -20Repository: Exabyte-io/api-examples
Length of output: 156
🏁 Script executed:
# Check if there are tests for the job API module
find . -path "*/tests/*" -type f -name "*.py" | xargs grep -l "create_job\|job.api"Repository: Exabyte-io/api-examples
Length of output: 51
🏁 Script executed:
# Check what's in the tests directory
find tests -type f -name "*.py" 2>/dev/nullRepository: Exabyte-io/api-examples
Length of output: 138
🏁 Script executed:
# Look at the job.py wrapper more carefully to understand the decorator
cat -n src/py/mat3ra/notebooks_utils/job.py | head -50Repository: Exabyte-io/api-examples
Length of output: 2091
🏁 Script executed:
# Check if there are any other mutations of job_workflow_dict in the function
cat -n src/py/mat3ra/notebooks_utils/core/entity/job/api.py | sed -n '40,82p'Repository: Exabyte-io/api-examples
Length of output: 1665
🏁 Script executed:
# Check if Workflow.to_dict() or Material.to_dict() might return a dict with _id
rg "to_dict\|from_dict" --type py src/py/mat3ra/notebooks_utils/ -A 3 | head -40Repository: Exabyte-io/api-examples
Length of output: 49
Avoid mutating caller-provided workflow dictionaries.
The function directly mutates job_workflow_dict via pop("_id", None) on line 64. Callers who reuse this dictionary will experience unexpected side effects. Create a shallow copy before mutation:
Proposed fix
- job_workflow_dict.pop("_id", None)
- is_multimaterial = job_workflow_dict.get("isMultiMaterial", False)
+ workflow_dict = dict(job_workflow_dict)
+ workflow_dict.pop("_id", None)
+ is_multimaterial = workflow_dict.get("isMultiMaterial", False)
config: dict = {
"_project": {"_id": project_id},
- "workflow": job_workflow_dict,
+ "workflow": workflow_dict,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/py/mat3ra/notebooks_utils/core/entity/job/api.py` around lines 64 - 65,
The code mutates the caller-provided job_workflow_dict by calling
job_workflow_dict.pop("_id", None); fix this by making a shallow copy (e.g.,
local_workflow = dict(job_workflow_dict)) and perform the pop and subsequent
reads (such as computing is_multimaterial =
local_workflow.get("isMultiMaterial", False)) on that copy so the original
dictionary is not modified; update references in the surrounding function (where
job_workflow_dict is used) to use the copied variable for mutation and
inspection.
Merged
Merged
This was referenced Aug 6, 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
Refactor
Documentation
Tests