Open
Conversation
for more information, see https://pre-commit.ci
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMonolithic Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Caller
participant API as stk.build.sqs_structures
participant Parser as sqsgenerator.parse_config
participant Worker as WorkerThread
participant Core as sqsgenerator.core.optimize
participant Pack as SqsResultPack
Client->>API: call sqs_structures(Atoms, composition, ...)
API->>Parser: parse configuration
alt parse ok
API->>Worker: start optimizer thread (with stop callback)
Worker->>Core: run optimize(config, callback)
Core-->>Worker: optimization result
Worker->>API: signal completion
API->>Pack: wrap optimizer result
API-->>Client: return SqsResultPack
else parse failure
Parser-->>API: raise ValueError
API-->>Client: raise ValueError
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.gh.lumenfield.work/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/structuretoolkit/build/sqs/__init__.py (1)
19-35: Optional:__all__is not sorted.Per static analysis (RUF022),
__all__entries should follow isort-style sorting for consistency.♻️ Sorted `__all__`
__all__ = [ "Composition", "Element", "IterationMode", "Prec", - "ResultType", "Shell", "ShellRadii", "ShellWeights", "Site", + "SqsResult", "SroParameter", - "SqsResult", "SqsResultInteract", "SqsResultSplit", "SublatticeMode", "sqs_structures", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/structuretoolkit/build/sqs/__init__.py` around lines 19 - 35, The __all__ list in this module is not alphabetically sorted; update the __all__ variable to use isort-style (alphabetical) ordering so exports are consistent and satisfy RUF022. Locate the __all__ declaration (the list containing "Composition", "Element", ..., "sqs_structures") and reorder its string entries alphabetically (preserving exact symbol names and casing), then run the project's linters/formatters to confirm the change.src/structuretoolkit/build/sqs/_interface.py (1)
284-288: Exception in worker thread is silently lost.If
sqs_optimizeraises an exception, it's swallowed by the thread andoptimization_resultremainsNone, resulting in a genericRuntimeError. Consider capturing and re-raising the actual exception for better debugging.♻️ Proposed enhancement to preserve exception info
+ optimization_exception: BaseException | None = None + def _optimize(): + nonlocal optimization_exception try: result_local = sqs_optimize(config, log_level=level, callback=_callback) - stop_event.set() nonlocal optimization_result optimization_result = result_local + except BaseException as e: + optimization_exception = e + finally: + stop_event.set()Then at line 300:
if optimization_result is None: + if optimization_exception is not None: + raise RuntimeError("Optimization failed") from optimization_exception raise RuntimeError("Optimization failed to produce a result.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/structuretoolkit/build/sqs/_interface.py` around lines 284 - 288, The worker thread function _optimize currently calls sqs_optimize and assigns optimization_result but swallows exceptions; change _optimize to catch Exception as e, store that exception (e) or its traceback into a shared variable (e.g., optimization_result or a new optimization_exception) and ensure stop_event.set() is still called, then in the main thread where optimization_result is inspected re-raise the captured exception (or raise RuntimeError from the captured exception) so the actual error/traceback from sqs_optimize is preserved; reference _optimize, sqs_optimize, optimization_result and stop_event when implementing this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/structuretoolkit/build/sqs/__init__.py`:
- Around line 1-16: The import of ResultType in
src/structuretoolkit/build/sqs/__init__.py is invalid because ResultType is not
defined in _types.py; either remove ResultType from the import list in
__init__.py (and from any __all__ export there) or add a proper ResultType
definition to _types.py if it's required; update usages referencing ResultType
(search for ResultType in the package) to use the new type or appropriate
existing type to avoid broken references.
In `@src/structuretoolkit/build/sqs/_interface.py`:
- Around line 99-118: The overload for sqs_structures has a type mismatch:
objective is annotated as list[float] but given a default 0.0; update the
overload signature for sqs_structures so the objective parameter matches the
implementation (accepts list[float] | float with default 0.0) — i.e., change the
overload's objective type to list[float] | float = 0.0 (or alternatively provide
a list default like [0.0] if you prefer list-only), ensuring the overload and
the implementation (which declares objective: list[float] | float = 0.0) are
consistent.
- Around line 292-298: The loop waiting on stop_event can block forever if the
worker thread (t running sqs_optimize) crashes before setting stop_event; change
the wait to a timed wait so the main thread wakes periodically and re-checks
thread liveness. Update the loop around t.is_alive() and stop_event.wait() to
call stop_event.wait(timeout=...) (e.g., 0.5–1s) or replace with
t.join(timeout=...) so that if t.is_alive() becomes False the loop breaks; keep
the existing except/finally behavior (including t.join()) but ensure the timed
wait lets the main thread detect a dead worker and exit instead of hanging.
In `@src/structuretoolkit/build/sqs/_types.py`:
- Line 131: The Composition type fails because Element is currently defined as a
list object rather than a type alias; change the Element symbol into a proper
type alias (e.g., a Literal union or Typed alias) so Composition = dict[Element
| Literal["sites"], Union[int, Site]] is valid; update the Element declaration
(and imports if necessary) to use typing/typing_extensions TypeAlias or
Literal/Union types and ensure Site and Composition use those type names
consistently.
- Around line 9-125: Element is currently a runtime list literal, not a type, so
using it in annotations like Composition causes errors; replace it with a proper
type definition and keep the runtime list separate: create a runtime constant
(e.g., ELEMENT_SYMBOLS) holding the list of symbols and define Element as a type
alias or a Literal union (e.g., Element: TypeAlias = Literal["0","H","He",...])
or simply Element: TypeAlias = str if you don't need strict literals, then
update usages (e.g., Composition) to reference the new Element type alias
instead of the list object.
- Around line 1-195: Define the missing exported type alias ResultType in this
module by adding a TypeAlias that points to the existing union SqsResult (e.g.
ResultType: TypeAlias = SqsResult) so imports from __init__.py succeed; place
the declaration near the other aliases (after SqsResult) and ensure the name
ResultType is available at module level for export.
---
Nitpick comments:
In `@src/structuretoolkit/build/sqs/__init__.py`:
- Around line 19-35: The __all__ list in this module is not alphabetically
sorted; update the __all__ variable to use isort-style (alphabetical) ordering
so exports are consistent and satisfy RUF022. Locate the __all__ declaration
(the list containing "Composition", "Element", ..., "sqs_structures") and
reorder its string entries alphabetically (preserving exact symbol names and
casing), then run the project's linters/formatters to confirm the change.
In `@src/structuretoolkit/build/sqs/_interface.py`:
- Around line 284-288: The worker thread function _optimize currently calls
sqs_optimize and assigns optimization_result but swallows exceptions; change
_optimize to catch Exception as e, store that exception (e) or its traceback
into a shared variable (e.g., optimization_result or a new
optimization_exception) and ensure stop_event.set() is still called, then in the
main thread where optimization_result is inspected re-raise the captured
exception (or raise RuntimeError from the captured exception) so the actual
error/traceback from sqs_optimize is preserved; reference _optimize,
sqs_optimize, optimization_result and stop_event when implementing this change.
🪄 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: 529c7ba7-59c5-4ed0-90a3-6e9e805cb137
📒 Files selected for processing (6)
.ci_support/environment.ymlsrc/structuretoolkit/build/sqs.pysrc/structuretoolkit/build/sqs/__init__.pysrc/structuretoolkit/build/sqs/_interface.pysrc/structuretoolkit/build/sqs/_types.pytests/test_sqs.py
💤 Files with no reviewable changes (1)
- src/structuretoolkit/build/sqs.py
Comment on lines
+1
to
+195
| import numpy as np | ||
| from ase import Atoms | ||
| from typing import Literal, TypeAlias, Protocol, overload, Union | ||
|
|
||
| Shell: TypeAlias = int | ||
|
|
||
| SublatticeMode = Literal["split", "interact"] | ||
| IterationMode = Literal["random", "systematic"] | ||
| Element = [ | ||
| "0", | ||
| "H", | ||
| "He", | ||
| "Li", | ||
| "Be", | ||
| "B", | ||
| "C", | ||
| "N", | ||
| "O", | ||
| "F", | ||
| "Ne", | ||
| "Na", | ||
| "Mg", | ||
| "Al", | ||
| "Si", | ||
| "P", | ||
| "S", | ||
| "Cl", | ||
| "Ar", | ||
| "K", | ||
| "Ca", | ||
| "Sc", | ||
| "Ti", | ||
| "V", | ||
| "Cr", | ||
| "Mn", | ||
| "Fe", | ||
| "Co", | ||
| "Ni", | ||
| "Cu", | ||
| "Zn", | ||
| "Ga", | ||
| "Ge", | ||
| "As", | ||
| "Se", | ||
| "Br", | ||
| "Kr", | ||
| "Rb", | ||
| "Sr", | ||
| "Y", | ||
| "Zr", | ||
| "Nb", | ||
| "Mo", | ||
| "Tc", | ||
| "Ru", | ||
| "Rh", | ||
| "Pd", | ||
| "Ag", | ||
| "Cd", | ||
| "In", | ||
| "Sn", | ||
| "Sb", | ||
| "Te", | ||
| "I", | ||
| "Xe", | ||
| "Cs", | ||
| "Ba", | ||
| "La", | ||
| "Ce", | ||
| "Pr", | ||
| "Nd", | ||
| "Pm", | ||
| "Sm", | ||
| "Eu", | ||
| "Gd", | ||
| "Tb", | ||
| "Dy", | ||
| "Ho", | ||
| "Er", | ||
| "Tm", | ||
| "Yb", | ||
| "Lu", | ||
| "Hf", | ||
| "Ta", | ||
| "W", | ||
| "Re", | ||
| "Os", | ||
| "Ir", | ||
| "Pt", | ||
| "Au", | ||
| "Hg", | ||
| "Tl", | ||
| "Pb", | ||
| "Bi", | ||
| "Po", | ||
| "At", | ||
| "Rn", | ||
| "Fr", | ||
| "Ra", | ||
| "Ac", | ||
| "Th", | ||
| "Pa", | ||
| "U", | ||
| "Np", | ||
| "Pu", | ||
| "Am", | ||
| "Cm", | ||
| "Bk", | ||
| "Cf", | ||
| "Es", | ||
| "Fm", | ||
| "Md", | ||
| "No", | ||
| "Lr", | ||
| "Rf", | ||
| "Db", | ||
| "Sg", | ||
| "Bh", | ||
| "Hs", | ||
| "Mt", | ||
| "Ds", | ||
| "Rg", | ||
| "Cn", | ||
| "Uut", | ||
| "Fl", | ||
| ] | ||
|
|
||
| Site = Union[str, list[int]] | ||
|
|
||
| Prec = Literal["single", "double"] | ||
|
|
||
| Composition = dict[Element | Literal["sites"], Union[int, Site]] | ||
|
|
||
| ShellWeights = dict[Shell, float] | ||
|
|
||
| ShellRadii = list[float] | ||
|
|
||
| LogLevel = Literal["warn", "info", "debug", "error", "trace"] | ||
|
|
||
|
|
||
| class SroParameter: | ||
| @property | ||
| def i(self) -> int: ... | ||
|
|
||
| @property | ||
| def j(self) -> int: ... | ||
|
|
||
| @property | ||
| def shell(self) -> int: ... | ||
|
|
||
| def __float__(self) -> float: ... | ||
|
|
||
| @property | ||
| def value(self) -> float: ... | ||
|
|
||
|
|
||
| class SqsResultInteract(Protocol): | ||
| def shell_index(self, shell: int) -> int: ... | ||
|
|
||
| def species_index(self, species: int) -> int: ... | ||
|
|
||
| def rank(self) -> str: ... | ||
|
|
||
| @overload | ||
| def sro( | ||
| self, | ||
| ) -> ( | ||
| np.ndarray[tuple[int, int, int], np.dtype[np.float32]] | ||
| | np.ndarray[tuple[int, int, int], np.dtype[np.float64]] | ||
| ): ... | ||
|
|
||
| @overload | ||
| def sro(self, shell: int) -> list[SroParameter]: ... | ||
|
|
||
| @overload | ||
| def sro(self, i: int, j: int) -> list[SroParameter]: ... | ||
|
|
||
| @overload | ||
| def sro(self, shell: int, i: int, j: int) -> SroParameter: ... | ||
|
|
||
| @property | ||
| def objective(self) -> float: ... | ||
|
|
||
| def atoms(self) -> Atoms: ... | ||
|
|
||
|
|
||
| class SqsResultSplit(Protocol): | ||
| @property | ||
| def objective(self) -> float: ... | ||
|
|
||
| def atoms(self) -> Atoms: ... | ||
|
|
||
| def sublattices(self) -> list[SqsResultInteract]: ... | ||
|
|
||
|
|
||
| SqsResult = SqsResultSplit | SqsResultInteract |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify ResultType is not defined in _types.py
echo "Searching for ResultType definition in _types.py:"
rg -n "ResultType" src/structuretoolkit/build/sqs/_types.py || echo "ResultType NOT found in _types.py"
echo ""
echo "Checking if ResultType is imported in __init__.py:"
rg -n "ResultType" src/structuretoolkit/build/sqs/__init__.pyRepository: pyiron/structuretoolkit
Length of output: 241
Critical: ResultType is not defined in this file but is imported in __init__.py.
The pipeline fails because __init__.py imports ResultType from this module (line 9) and exports it (line 24), but it's not defined here. Add the ResultType definition to this file—likely as a type alias for SqsResult or another result type.
🧰 Tools
🪛 GitHub Actions: Pipeline
[error] 1-1: ImportError during test collection: cannot import name 'ResultType' from 'structuretoolkit.build.sqs._types' (/home/runner/miniconda3/envs/test/lib/python3.14/site-packages/structuretoolkit/build/sqs/_types.py).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/structuretoolkit/build/sqs/_types.py` around lines 1 - 195, Define the
missing exported type alias ResultType in this module by adding a TypeAlias that
points to the existing union SqsResult (e.g. ResultType: TypeAlias = SqsResult)
so imports from __init__.py succeed; place the declaration near the other
aliases (after SqsResult) and ensure the name ResultType is available at module
level for export.
Comment on lines
+9
to
+125
| Element = [ | ||
| "0", | ||
| "H", | ||
| "He", | ||
| "Li", | ||
| "Be", | ||
| "B", | ||
| "C", | ||
| "N", | ||
| "O", | ||
| "F", | ||
| "Ne", | ||
| "Na", | ||
| "Mg", | ||
| "Al", | ||
| "Si", | ||
| "P", | ||
| "S", | ||
| "Cl", | ||
| "Ar", | ||
| "K", | ||
| "Ca", | ||
| "Sc", | ||
| "Ti", | ||
| "V", | ||
| "Cr", | ||
| "Mn", | ||
| "Fe", | ||
| "Co", | ||
| "Ni", | ||
| "Cu", | ||
| "Zn", | ||
| "Ga", | ||
| "Ge", | ||
| "As", | ||
| "Se", | ||
| "Br", | ||
| "Kr", | ||
| "Rb", | ||
| "Sr", | ||
| "Y", | ||
| "Zr", | ||
| "Nb", | ||
| "Mo", | ||
| "Tc", | ||
| "Ru", | ||
| "Rh", | ||
| "Pd", | ||
| "Ag", | ||
| "Cd", | ||
| "In", | ||
| "Sn", | ||
| "Sb", | ||
| "Te", | ||
| "I", | ||
| "Xe", | ||
| "Cs", | ||
| "Ba", | ||
| "La", | ||
| "Ce", | ||
| "Pr", | ||
| "Nd", | ||
| "Pm", | ||
| "Sm", | ||
| "Eu", | ||
| "Gd", | ||
| "Tb", | ||
| "Dy", | ||
| "Ho", | ||
| "Er", | ||
| "Tm", | ||
| "Yb", | ||
| "Lu", | ||
| "Hf", | ||
| "Ta", | ||
| "W", | ||
| "Re", | ||
| "Os", | ||
| "Ir", | ||
| "Pt", | ||
| "Au", | ||
| "Hg", | ||
| "Tl", | ||
| "Pb", | ||
| "Bi", | ||
| "Po", | ||
| "At", | ||
| "Rn", | ||
| "Fr", | ||
| "Ra", | ||
| "Ac", | ||
| "Th", | ||
| "Pa", | ||
| "U", | ||
| "Np", | ||
| "Pu", | ||
| "Am", | ||
| "Cm", | ||
| "Bk", | ||
| "Cf", | ||
| "Es", | ||
| "Fm", | ||
| "Md", | ||
| "No", | ||
| "Lr", | ||
| "Rf", | ||
| "Db", | ||
| "Sg", | ||
| "Bh", | ||
| "Hs", | ||
| "Mt", | ||
| "Ds", | ||
| "Rg", | ||
| "Cn", | ||
| "Uut", | ||
| "Fl", | ||
| ] |
Contributor
There was a problem hiding this comment.
Element is defined as a list, not a type alias.
Element is assigned a list literal, which makes it a runtime list object rather than a type. This will cause issues when used in type annotations like Composition = dict[Element | Literal["sites"], ...] at line 131 — a list cannot be used as a type key.
If the intent is to define a type for valid element symbols, use Literal:
🔧 Proposed fix using Literal type
-Element = [
- "0",
- "H",
- "He",
- ...
- "Fl",
-]
+Element: TypeAlias = Literal[
+ "0", "H", "He", "Li", "Be", "B", "C", "N", "O", "F", "Ne",
+ "Na", "Mg", "Al", "Si", "P", "S", "Cl", "Ar", "K", "Ca",
+ "Sc", "Ti", "V", "Cr", "Mn", "Fe", "Co", "Ni", "Cu", "Zn",
+ "Ga", "Ge", "As", "Se", "Br", "Kr", "Rb", "Sr", "Y", "Zr",
+ "Nb", "Mo", "Tc", "Ru", "Rh", "Pd", "Ag", "Cd", "In", "Sn",
+ "Sb", "Te", "I", "Xe", "Cs", "Ba", "La", "Ce", "Pr", "Nd",
+ "Pm", "Sm", "Eu", "Gd", "Tb", "Dy", "Ho", "Er", "Tm", "Yb",
+ "Lu", "Hf", "Ta", "W", "Re", "Os", "Ir", "Pt", "Au", "Hg",
+ "Tl", "Pb", "Bi", "Po", "At", "Rn", "Fr", "Ra", "Ac", "Th",
+ "Pa", "U", "Np", "Pu", "Am", "Cm", "Bk", "Cf", "Es", "Fm",
+ "Md", "No", "Lr", "Rf", "Db", "Sg", "Bh", "Hs", "Mt", "Ds",
+ "Rg", "Cn", "Uut", "Fl",
+]Alternatively, if you want to keep the list for runtime validation and have a separate type, define both:
ELEMENT_SYMBOLS = ["0", "H", "He", ...] # runtime list
Element: TypeAlias = str # simple type alias🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/structuretoolkit/build/sqs/_types.py` around lines 9 - 125, Element is
currently a runtime list literal, not a type, so using it in annotations like
Composition causes errors; replace it with a proper type definition and keep the
runtime list separate: create a runtime constant (e.g., ELEMENT_SYMBOLS) holding
the list of symbols and define Element as a type alias or a Literal union (e.g.,
Element: TypeAlias = Literal["0","H","He",...]) or simply Element: TypeAlias =
str if you don't need strict literals, then update usages (e.g., Composition) to
reference the new Element type alias instead of the list object.
…improve-interface
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #476 +/- ##
==========================================
+ Coverage 82.92% 84.34% +1.41%
==========================================
Files 26 28 +2
Lines 1857 1929 +72
==========================================
+ Hits 1540 1627 +87
+ Misses 317 302 -15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/structuretoolkit/build/sqs/__init__.py (1)
18-33: Consider sorting__all__for consistency.Ruff flags that
__all__is not sorted. Sorting the list alphabetically improves consistency and makes future additions easier to review.📋 Proposed alphabetical sort
__all__ = [ "Composition", "Element", "IterationMode", "Prec", "Shell", "ShellRadii", "ShellWeights", "Site", - "SroParameter", "SqsResult", "SqsResultInteract", "SqsResultSplit", + "SroParameter", "SublatticeMode", "sqs_structures", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/structuretoolkit/build/sqs/__init__.py` around lines 18 - 33, The __all__ list in the module is unsorted; sort the entries in the __all__ list alphabetically (by string) while keeping the same items and punctuation so semantics don't change—update the __all__ definition (which currently contains "Composition", "Element", "IterationMode", "Prec", "Shell", "ShellRadii", "ShellWeights", "Site", "SroParameter", "SqsResult", "SqsResultInteract", "SqsResultSplit", "SublatticeMode", "sqs_structures") to an alphabetically ordered sequence to satisfy the linter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/structuretoolkit/build/sqs/__init__.py`:
- Around line 18-33: The __all__ list in the module is unsorted; sort the
entries in the __all__ list alphabetically (by string) while keeping the same
items and punctuation so semantics don't change—update the __all__ definition
(which currently contains "Composition", "Element", "IterationMode", "Prec",
"Shell", "ShellRadii", "ShellWeights", "Site", "SroParameter", "SqsResult",
"SqsResultInteract", "SqsResultSplit", "SublatticeMode", "sqs_structures") to an
alphabetically ordered sequence to satisfy the linter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb11e8f4-48e2-4d88-b418-239b2b8743cb
📒 Files selected for processing (1)
src/structuretoolkit/build/sqs/__init__.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/structuretoolkit/build/sqs/_interface.py`:
- Around line 298-305: The current except TimeoutError after calling
t.join(timeout=5.0) is ineffective because Thread.join never raises
TimeoutError; instead check t.is_alive() after join and raise the RuntimeError
if the thread remains alive. Update the block around t.join(timeout=5.0) (the
thread variable `t`) to call t.join(timeout=5.0) then if t.is_alive(): raise the
same RuntimeError (leave message unchanged) rather than relying on an except
TimeoutError branch so stopping logic in sqs_structures becomes effective.
🪄 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: 23325877-d56f-4034-bb90-09a2e736845b
📒 Files selected for processing (3)
src/structuretoolkit/build/sqs/_interface.pysrc/structuretoolkit/build/sqs/_types.pytests/test_sqs.py
✅ Files skipped from review due to trivial changes (1)
- src/structuretoolkit/build/sqs/_types.py
Comment on lines
+298
to
+305
| try: | ||
| t.join(timeout=5.0) | ||
| except TimeoutError: | ||
| raise RuntimeError( | ||
| "Optimization thread did not finish within the timeout period after requesting it to stop. " | ||
| "The optimization may still be running in the background. Try to decrease chunk_size by passing it as a " | ||
| "keyword argument to sqs_structures to make the optimization more responsive to stop requests." | ||
| ) |
Contributor
There was a problem hiding this comment.
Thread.join() does not raise TimeoutError—the timeout check is ineffective.
Thread.join(timeout=5.0) returns None regardless of whether the thread terminated or the timeout elapsed. To detect a timeout, you must check t.is_alive() afterwards. The current code's except TimeoutError block will never execute.
🐛 Proposed fix
finally:
- try:
- t.join(timeout=5.0)
- except TimeoutError:
+ t.join(timeout=5.0)
+ if t.is_alive():
raise RuntimeError(
"Optimization thread did not finish within the timeout period after requesting it to stop. "
"The optimization may still be running in the background. Try to decrease chunk_size by passing it as a "
"keyword argument to sqs_structures to make the optimization more responsive to stop requests."
)📝 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
| try: | |
| t.join(timeout=5.0) | |
| except TimeoutError: | |
| raise RuntimeError( | |
| "Optimization thread did not finish within the timeout period after requesting it to stop. " | |
| "The optimization may still be running in the background. Try to decrease chunk_size by passing it as a " | |
| "keyword argument to sqs_structures to make the optimization more responsive to stop requests." | |
| ) | |
| finally: | |
| t.join(timeout=5.0) | |
| if t.is_alive(): | |
| raise RuntimeError( | |
| "Optimization thread did not finish within the timeout period after requesting it to stop. " | |
| "The optimization may still be running in the background. Try to decrease chunk_size by passing it as a " | |
| "keyword argument to sqs_structures to make the optimization more responsive to stop requests." | |
| ) |
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 301-305: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/structuretoolkit/build/sqs/_interface.py` around lines 298 - 305, The
current except TimeoutError after calling t.join(timeout=5.0) is ineffective
because Thread.join never raises TimeoutError; instead check t.is_alive() after
join and raise the RuntimeError if the thread remains alive. Update the block
around t.join(timeout=5.0) (the thread variable `t`) to call t.join(timeout=5.0)
then if t.is_alive(): raise the same RuntimeError (leave message unchanged)
rather than relying on an except TimeoutError branch so stopping logic in
sqs_structures becomes effective.
Member
|
@dgehringer Thanks for the update. I just returned from holidays and start the review now. In terms of the suggested changes, I am wondering if those should be included in |
* Add unit tests for SQS functionality Added several test cases to tests/test_sqs.py to increase coverage of the sqs_structures function and its helper classes. Covered parameters include atol, rtol, shell_radii, log_level, and num_threads. Also added tests for error handling (ParseError, AttributeError, RuntimeError) and graceful stopping via KeyboardInterrupt. Coverage for _interface.py increased to 97%. Co-authored-by: jan-janssen <3854739+jan-janssen@users.noreply.github.com> * Add comprehensive unit tests for SQS functionality - Added tests for atol, rtol, and shell_radii in both interact and split modes. - Added tests for different log levels (info, debug, error, trace). - Added tests for kwargs passing (e.g., chunk_size). - Added error handling tests for invalid compositions and non-split result sublattices access. - Added simulation of KeyboardInterrupt to test graceful stopping. - Added test for num_threads parameter. - Added test for optimization failure handling. - Coverage for _interface.py increased to 97%. Co-authored-by: jan-janssen <3854739+jan-janssen@users.noreply.github.com> --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> Co-authored-by: jan-janssen <3854739+jan-janssen@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_sqs.py (1)
35-41: Avoid bare attribute access in theAttributeErrorassertion (Ruff B018).The current pattern can trigger lint noise; using
getattr(...)keeps the same behavior while avoiding the warning.Proposed lint-safe change
with self.assertRaises(AttributeError): # ensure proxy works as expected - stk.build.sqs_structures( - structure=bulk("Au", cubic=True), - composition=dict(Cu=16, Au=16), - supercell=(2, 2, 2), - ).best().not_defined + getattr( + stk.build.sqs_structures( + structure=bulk("Au", cubic=True), + composition=dict(Cu=16, Au=16), + supercell=(2, 2, 2), + ).best(), + "not_defined", + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_sqs.py` around lines 35 - 41, Replace the bare attribute access that triggers Ruff B018 by calling getattr on the returned object from stk.build.sqs_structures(...).best() inside the with self.assertRaises(AttributeError) block; specifically, call getattr(stk.build.sqs_structures(structure=bulk("Au", cubic=True), composition=dict(Cu=16, Au=16), supercell=(2,2,2)).best(), "not_defined") so the test still asserts AttributeError but avoids direct attribute access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_sqs.py`:
- Around line 35-41: Replace the bare attribute access that triggers Ruff B018
by calling getattr on the returned object from
stk.build.sqs_structures(...).best() inside the with
self.assertRaises(AttributeError) block; specifically, call
getattr(stk.build.sqs_structures(structure=bulk("Au", cubic=True),
composition=dict(Cu=16, Au=16), supercell=(2,2,2)).best(), "not_defined") so the
test still asserts AttributeError but avoids direct attribute access.
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.
Add more features to the sqsgenerator interface (>0.4.9)
Summary by CodeRabbit
Refactor
New Features
Tests
Chores