Add HSpice Simulator Backend#5
Conversation
Implement HSpiceSimulator , HSpiceServer , and HSpiceRawFile to support HSPICE simulations. Vendor the HSPICE raw binary result parser ( hspice_read.c / hspice_read.h ) from PyOPUS under PySpice/Spice/HSpice/hspicefile/ Add docs of hspice binary file structure
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughAdds an HSpice backend with simulator registration, a server wrapper for running HSpice and reading outputs, raw-file conversion into PySpice analyses, compiled binary parsing support, packaging changes, and verification/test scripts. ChangesHSpice Backend and Verification
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SimulatorBase
participant HSpiceSimulator
participant HSpiceServer
participant HSpiceRawFile
SimulatorBase->>HSpiceSimulator: factory(simulator="hspice")
HSpiceSimulator->>HSpiceServer: __call__(str(simulation))
HSpiceServer-->>HSpiceSimulator: raw_file
HSpiceSimulator->>HSpiceRawFile: raw_file.simulation = simulation
HSpiceSimulator->>HSpiceRawFile: to_analysis()
HSpiceRawFile-->>HSpiceSimulator: analysis
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
Looks very nice at first glance. I'll come back for a detailed review after coderabbit is done. One immediate concern is licensing of the PyOPUS components. According to their website, PyOPUS is AGPL-3.0, but the PyPI package says GPL-3.0. Do you know which of these is correct? EDIT: Also please note that I have coderabbit set up to be extra-picky. Some of its comments may be totally irrelevant. Feel free to tell it so. Most importantly: thanks for your interest in improving this stuff! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@PySpice/Spice/HSpice/hspicefile/build.py`:
- Line 18: The `define_macros` parameter unconditionally includes the LINUX
macro, which forces Linux-specific preprocessor behavior on all platforms and
breaks cross-platform builds. Modify the `define_macros` list to conditionally
include the LINUX macro only when the build platform is actually Linux. Use
platform detection such as sys.platform or platform.system() to check if the
current platform is Linux before adding the macro to the list, ensuring the
macro is only defined for actual Linux builds.
In `@PySpice/Spice/HSpice/hspicefile/hspice_read.c`:
- Around line 356-368: The endian swap handling in the readBlockData function
call is inconsistent for modern format files that contain mixed 4-byte and
8-byte fields. The code currently passes a swap value calculated for 4-byte
chunks, but modern payloads include 8-byte doubles that require field-level
width-aware swapping. The swapGlobal variable referenced in the later decoding
logic (around lines 529-546) is never initialized, causing endian conversion to
be skipped for 8-byte fields. Initialize and properly assign swapGlobal based on
the swap value returned from readBlockHeader, then modify the data reading and
decoding path to perform width-aware endian swapping that handles both 4-byte
and 8-byte fields appropriately for modern format files.
- Around line 949-965: In the failed error handling section, the variables
varTypes and varSizes are being freed twice, which causes a double-free error.
Remove the duplicate PyMem_Free calls for varTypes and varSizes that appear at
the end of the failed cleanup block, keeping only the first occurrence of these
two frees near the beginning of the error handling path.
- Around line 635-638: In the error handling block following the failed
PyArg_ParseTuple call at line 635, replace the current return statement that
increments and returns Py_None with a direct return of NULL. The Python C-API
contract requires returning NULL to signal an error condition, not Py_None, as
returning a non-NULL value masks the exception that PyArg_ParseTuple has already
set. Remove the Py_INCREF(Py_None) line and change the return statement to
simply return NULL.
In `@PySpice/Spice/HSpice/RawFile.py`:
- Around line 38-43: The code directly accesses data_list[0] to assign res_dict
without verifying that data_list contains exactly one table, causing outer-sweep
tables to be silently discarded when multiple tables are present. Add a guard
condition that checks len(data_list) immediately after unpacking sweeps, and
raise an exception if len(data_list) != 1 to fail fast with a clear error
message. Only proceed with res_dict = data_list[0] if the guard passes, ensuring
that partial or truncated analysis results are never silently returned.
In `@PySpice/Spice/HSpice/Server.py`:
- Around line 53-69: The code does not validate the concurrency_limit parameter,
which can cause an infinite blocking loop if the value is less than or equal to
0. When limit is zero or negative, the for loop creating lock files at line 53
produces no lock files, causing the while True loop at lines 58-69 to sleep
indefinitely without ever acquiring a lock. Add validation in the __init__
method to ensure concurrency_limit is at least 1 before it is used, and raise an
appropriate exception if the constraint is violated.
- Line 102: The subprocess.run() call that executes the HSPICE command is
missing a timeout parameter, which can cause the process to hang indefinitely
and block service capacity. Add a timeout parameter to the subprocess.run()
function call where cmd is executed with cwd=tmp_dir, capture_output=True, and
text=True. Choose an appropriate timeout value that allows legitimate HSPICE
operations to complete while preventing hung processes from blocking resources
indefinitely. Ensure that the timeout exception handling is implemented to
gracefully handle cases where the process exceeds the timeout limit.
- Around line 25-28: The issue is that the check for the 'meg' suffix is nested
inside the condition that checks for 'm' suffix, making it unreachable since any
string ending with 'meg' also ends with 'm'. Reorder the suffix checks by
evaluating the more specific 'meg' suffix first before checking for the general
'm' suffix. Move the `if val_str.endswith('meg')` condition to be checked before
the `elif val_str.endswith('m')` condition so that values like '1meg' are
correctly parsed as mega (multiplied by 1e6) instead of being misparsed as milli
(multiplied by 1e-3).
In `@PySpice/Spice/Simulator.py`:
- Around line 23-27: The HSpiceSimulator import from
PySpice/Spice/HSpice/Simulator.py is eagerly loaded at module initialization,
which causes the entire Simulator module to fail if HSpice dependencies are
unavailable, even for users who only need NgSpice or Xyce backends. Wrap the
HSpiceSimulator import statement in a try-except block to make it optional and
gracefully handle cases where HSpice components are not available, ensuring the
module remains usable for other backends when HSpice is unavailable.
In `@PySpice/Spice/SimulatorBase.py`:
- Around line 45-53: The hard-coded SIMULATORS tuple is being used as a
whitelist validation in the factory() method (around lines 84-89), which
prevents dynamically registered simulators from being used and creates a drift
between the static list and the actual registration map maintained by
register_simulator_class(). Remove the SIMULATORS tuple validation from the
factory() method and instead check directly against the registration map to
determine if a simulator is valid. This way the registration map becomes the
single source of truth for available simulators, allowing both pre-defined and
dynamically registered simulators to work correctly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 718ca7d9-2cbc-47ae-8ae8-a176e984e582
⛔ Files ignored due to path filters (3)
PySpice/Spice/HSpice/hspicefile/docs/block_structure.svgis excluded by!**/*.svgPySpice/Spice/HSpice/hspicefile/docs/file_structure.svgis excluded by!**/*.svgPySpice/Spice/HSpice/hspicefile/docs/record_layout.svgis excluded by!**/*.svg
📒 Files selected for processing (14)
PySpice/Spice/HSpice/RawFile.pyPySpice/Spice/HSpice/Server.pyPySpice/Spice/HSpice/Simulator.pyPySpice/Spice/HSpice/__init__.pyPySpice/Spice/HSpice/hspicefile/__init__.pyPySpice/Spice/HSpice/hspicefile/build.pyPySpice/Spice/HSpice/hspicefile/docs/hspice_file_structure.mdPySpice/Spice/HSpice/hspicefile/hspice_read.cPySpice/Spice/HSpice/hspicefile/hspice_read.hPySpice/Spice/NgSpice/Simulator.pyPySpice/Spice/Simulator.pyPySpice/Spice/SimulatorBase.pyPySpice/Spice/Xyce/Simulator.pysetup_data.py
|
One more comment re. c2q setup/hold window simulations: those are very sensitive to slew conditions. I'm currently collaborating with a colleague on an alternate method for parameter selection based on Bayesian Optimization, but that probably won't be ready anytime soon. My c2q_by_bisection branch is probably a dead end unless I find good reason to resurrect that work. |
…tor specific arguments to the relevant class
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
PySpice/Spice/HSpice/hspicefile/hspice_read.c (3)
839-844: 🗄️ Data Integrity & Integration | 🟠 MajorStrip the trailing
)fromv(...)node names.v(out)becomesout)here, so waveform lookups likeanalysis.nodes["out"]/analysis.outmiss.🤖 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 `@PySpice/Spice/HSpice/hspicefile/hspice_read.c` around lines 839 - 844, The node-name normalization in hspice_read.c’s waveform parsing is leaving a trailing ) on v(...) names, so lookups by the plain node name fail. Update the handling in the name[i] processing block to fully strip both the leading v( and the closing ) before storing the node name, and make sure the logic around name[i], j, and the copy loop in hspice_read.c produces out rather than out).
675-727: 🩺 Stability & Availability | 🟠 MajorGuard the header before fixed-offset access.
readHeaderBlock()only gives a buffer andoffset; this block still reads and writesbufatpostStartPosition*,dateEndPosition,titleStartPosition,numOfSweepsEndPosition,numOfSweepsPosition,numOfProbesPosition,numOfVariablesPosition, andvectorDescriptionStartPositionwithout checking for a short or malformed header. Add a length check before those accesses.🤖 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 `@PySpice/Spice/HSpice/hspicefile/hspice_read.c` around lines 675 - 727, In readHeaderBlock(), the fixed-offset parsing still assumes the header is long enough before touching postStartPosition*, dateEndPosition, titleStartPosition, numOfSweepsEndPosition, numOfSweepsPosition, numOfProbesPosition, and numOfVariablesPosition. Add a single bounds/length validation right after the buffer is read and before any strncmp/memcmp or buf[...] writes, and fail early for short or malformed headers. Keep the existing parsing flow in hspice_read.c but gate the modern-format detection and date/title/sweep extraction behind that guard.
951-971: 🩺 Stability & Availability | 🟠 MajorReturn
NULLwhen cleanup follows a Python exception.goto failedis reached afterPyUnicode_InternFromString,PyTuple_Pack,PyList_New, andPyList_Appendfailures, so returningPy_Nonefrom the shared cleanup block can hide the real error and surface asSystemError. KeepNoneonly when no exception is set.Proposed fix
- Py_INCREF(Py_None); - return Py_None; + if (PyErr_Occurred()) { + return NULL; + } + Py_RETURN_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 `@PySpice/Spice/HSpice/hspicefile/hspice_read.c` around lines 951 - 971, The shared cleanup path in hspice_read.c returns Py_None even when goto failed is reached after Python API errors in hspice_read, which can mask the real exception and trigger SystemError. Update the failed label cleanup so it checks whether an exception is already set after failures from PyUnicode_InternFromString, PyTuple_Pack, PyList_New, or PyList_Append, and return NULL in that case; only return Py_None when no Python exception is pending.
🤖 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 `@PySpice/Spice/HSpice/hspicefile/build.py`:
- Around line 2-10: The module-level NumPy import in LazyBuildExt should be
moved into finalize_options() so importing this build helper during setup does
not պահանջ NumPy before build-time dependencies are available. Update the
LazyBuildExt.finalize_options method to import NumPy locally, then use
np.get_include() when appending to each extension’s include_dirs, and remove the
top-level import from the build module.
---
Outside diff comments:
In `@PySpice/Spice/HSpice/hspicefile/hspice_read.c`:
- Around line 839-844: The node-name normalization in hspice_read.c’s waveform
parsing is leaving a trailing ) on v(...) names, so lookups by the plain node
name fail. Update the handling in the name[i] processing block to fully strip
both the leading v( and the closing ) before storing the node name, and make
sure the logic around name[i], j, and the copy loop in hspice_read.c produces
out rather than out).
- Around line 675-727: In readHeaderBlock(), the fixed-offset parsing still
assumes the header is long enough before touching postStartPosition*,
dateEndPosition, titleStartPosition, numOfSweepsEndPosition,
numOfSweepsPosition, numOfProbesPosition, and numOfVariablesPosition. Add a
single bounds/length validation right after the buffer is read and before any
strncmp/memcmp or buf[...] writes, and fail early for short or malformed
headers. Keep the existing parsing flow in hspice_read.c but gate the
modern-format detection and date/title/sweep extraction behind that guard.
- Around line 951-971: The shared cleanup path in hspice_read.c returns Py_None
even when goto failed is reached after Python API errors in hspice_read, which
can mask the real exception and trigger SystemError. Update the failed label
cleanup so it checks whether an exception is already set after failures from
PyUnicode_InternFromString, PyTuple_Pack, PyList_New, or PyList_Append, and
return NULL in that case; only return Py_None when no Python exception is
pending.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 58c13a43-0b17-414f-9488-ccc02dbe0ae5
📒 Files selected for processing (8)
PySpice/Spice/HSpice/RawFile.pyPySpice/Spice/HSpice/Server.pyPySpice/Spice/HSpice/hspicefile/__init__.pyPySpice/Spice/HSpice/hspicefile/build.pyPySpice/Spice/HSpice/hspicefile/hspice_read.cPySpice/Spice/Simulator.pyPySpice/Spice/SimulatorBase.pyPySpice/Spice/Xyce/Simulator.py
I can see a lot of potential in a project like this. Hopefully the changes I've made will address any initial concerns. I have tried to limit the changes I've made to only the hspice simulator but we'll see where that goes. PyOPUS seems to be AGPL3.0 according to the license file included in their repository. This matches up with the PySpice license, so hopefully that is not so much of an issue. I do need to add the proper copyright notices to the file headers for hspice_read. I also thought it would be useful to see how ngspice compares to hspice on characterisation results so I will add that here too |
NGSpice vs HSPICE Backend Comparison ReportThis report details the comparison between the OverviewA full test suite was run for both simulators. Values inside the generated Statistical Results
Error DistributionsThe plots below show the distribution of the percentage error across all measured data points for each category. 
Raw Value DistributionsThe plots below show the overlapping distributions of the raw physical values calculated by each simulator. 
Detailed Analysis of Outliers1. Leakage Power
|
…imulator performance later on when running larger libraries etc
AidenDawn
force-pushed
the
feature/hspice-backend
branch
from
00279dd to
9a2336f
Compare
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 `@PySpice/Spice/HSpice/hspicefile/hspice_read.c`:
- Line 138: Validate every file-derived block size before using it in
allocations or fread paths: in hspice_read.c, the value coming from
blockHeader[3] must be checked for zero, negative, misaligned, and overflow
conditions before it is used in the blockHeader handling and the later
read/allocate flows. Add guarding logic around the blockHeader parsing and the
code paths that consume it (including the reads around the marked sections in
the same parser) so invalid lengths are rejected early instead of being
converted into undersized buffers or oversized fread counts.
- Around line 492-505: Legacy row sizing in the HSpice parser is using only
numOfVectors in hspice_read, which overestimates num for complex datasets and
can read past rawData. Update the rowSize calculation in the legacy branch to
account for the expanded complex column count using the existing vector metadata
(for example, varSizes / complex-dependent-variable expansion), and keep the num
computation consistent wherever the same legacy sizing logic is reused,
including the later block referenced by the review.
- Around line 730-735: The header-length guard in HSpiceRead only checks
dateEndPosition, but later parsing also uses sweepSizePosition and
vectorDescriptionStartPosition, so a shorter header can still be read out of
bounds. Update the validation in hspice_read.c around the existing offset check
to verify the buffer is long enough for every fixed header position used later
in the same parsing path before any indexing occurs, and keep the failure path
and debugFile message in HSpiceRead consistent.
- Around line 1015-1017: The cleanup in hspice_read.c is double-releasing the
sweep-related references on the failure path because sweep, sweepValues, and
dataList remain non-NULL after Py_XDECREF. Update the cleanup block around the
sweep teardown so that after releasing each of these objects, the corresponding
pointer is immediately set to NULL, and make sure the failed label path in the
same function only decrefs live references. Use the hspice_read cleanup sequence
and the sweep/sweepValues/dataList variables to locate the fix.
- Around line 785-800: Validate the parsed counts in hspice_read before they are
used as allocation sizes or loop bounds. In the HSpiceRead parsing block,
replace the raw atoi-based handling for numOfVectors/numOfVariables with checks
that the values are positive and that the total cannot overflow when adding the
probe/vector counts. If any count is invalid, fail early in the same error path
used for unsupported sweep counts so PyMem_Malloc and later loops never see a
bad size.
- Around line 412-413: The table-read loop in hspice_read.c still uses the
legacy 4-byte float cutoff to detect the end of a block, so modern HSpice sweep
tables can run past their intended boundary. Update the read/merge logic in the
hspice table parsing path (the loop around the rawData offset check) to stop on
the modern 8-byte double sentinel instead of only checking for a huge float
value, and keep the EOF-based merge behavior for modern tables.
In `@PySpice/Spice/HSpice/RawFile.py`:
- Around line 140-142: The multi-analysis branch in RawFile.read currently
returns a plain list, which bypasses the aggregate accessor. Update the return
path in the RawFile parsing logic so that when there is more than one analysis
it returns an AnalysisList instance (with the existing analyses and combined
measurements), while preserving the single-analysis return from the same method.
Use the RawFile.read and AnalysisList symbols to locate and adjust the
multi-table result handling.
In `@PySpice/Spice/HSpice/Server.py`:
- Around line 245-247: The measurement parsing in Server.py currently uses
zip(names, vals), which can silently drop missing or extra columns in malformed
rows. Update the row-processing logic in the measurement loop to validate that
each vals row has the expected width before iterating, and handle mismatches
explicitly in the same area where names and vals are used so bad measurement
lines are not partially accepted.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 308b84f7-2fe2-421d-b9f4-199e9d718001
📒 Files selected for processing (4)
PySpice/Spice/HSpice/RawFile.pyPySpice/Spice/HSpice/Server.pyPySpice/Spice/HSpice/hspicefile/build.pyPySpice/Spice/HSpice/hspicefile/hspice_read.c
infinitymdm
left a comment
infinitymdm
left a comment
There was a problem hiding this comment.
Aside from the license note, this PR just needs improved consistency on the indentation. Please modify all .py files to use 4-space indentation rather than 2. Currently some use 2 spaces and some use 4.
…right notices to hspice_read files
|
Overall this is great work, and I'm looking forward to getting it merged. There are a handful of open Coderabbit comments I'd like you to resolve (or explain why they're not relevant), but once those are done this will be an excellent contribution. |
|
Sorry for the delay in getting this ready, I have had a couple of other priorities over the last couple of weeks. I moved this back to draft when I realised that I would need to go through and check that a reasonably complete range of simulation outputs was being parsed correctly by the C module. I have now worked through this, but the C itself changed quite significantly and there were some updates that were then necessary to the python. Hopefully the latest set of commits doesnt confuse this too much |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
PySpice/Spice/HSpice/hspicefile/hspice_read.c (1)
747-751: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winStill returns
Py_Noneinstead ofNULLonPyArg_ParseTuplefailure.This exact issue was flagged previously and marked "Addressed", but the current code still returns
Py_Nonehere.PyArg_ParseTuplesets an exception and returns false on failure; the function must returnNULLto propagate it. ReturningPy_Nonemasks the exception, which can cause silent corruption or crashes for callers.Proposed fix
if (!PyArg_ParseTuple(args, "si", &fileName, &debugMode)) { - Py_INCREF(Py_None); - return Py_None; + return NULL; }🤖 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 `@PySpice/Spice/HSpice/hspicefile/hspice_read.c` around lines 747 - 751, In hspice_read(), the PyArg_ParseTuple failure path still returns Py_None instead of propagating the exception. Update the error-handling branch after PyArg_ParseTuple(args, "si", &fileName, &debugMode) fails to return NULL directly, so the exception set by the parser is preserved; use the hspice_read() argument parsing block as the place to fix it.
🤖 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 `@PySpice/Spice/HSpice/hspicefile/docs/tests/run_simulations.py`:
- Around line 6-8: The hspice binary lookup in `run_simulations.py` can return
`None`, but the script continues into the template/version loop and only fails
later inside the per-case exception handling. Add an early guard right after
`HSPICE_BIN = shutil.which("hspice")` to stop execution immediately with a clear
message if the executable is missing, and make sure the simulation entry points
that use `HSPICE_BIN` do not proceed when it is unset.
In `@PySpice/Spice/HSpice/hspicefile/docs/tests/test_hspice_read.py`:
- Around line 9-13: The test setup in test_hspice_read.py is adding too shallow
a path, so the PySpice package root is not reliably discoverable in a clean
checkout. Update the sys.path adjustment used before importing hspice_read so it
walks one level higher, and keep the import from PySpice.Spice.HSpice.hspicefile
unchanged; this ensures the test can resolve the package correctly from the sims
test location.
In `@PySpice/Spice/HSpice/hspicefile/docs/tests/verify_all_types.py`:
- Around line 285-313: The ASCII parser in parse_ascii_to_flat_floats is
hardcoded to 13-character chunks, which does not match the default 9601 layout
used by the golden file. Update this helper to use the real field width based on
the post format/version, or change the ASCII test setup to explicitly request
post_version=2001 so the width matches. Make sure the logic in
verify_all_types.py stays aligned with the generated file format rather than
assuming 13 characters.
In `@PySpice/Spice/HSpice/hspicefile/hspice_read.c`:
- Around line 853-926: The header parsing in HSpiceRead still uses unvalidated
atoi conversions on file-derived substrings, so malformed data can silently
become 0 instead of raising a parse error. Update the parsing path in
hspice_read.c (including the sweep/header extraction logic and getSweepInfo) to
use validated strtol-style conversion with explicit end-pointer/errno checks,
and fail with the existing PyErr_Format/HSpiceParseError handling when tokens
are not valid integers. Keep the current bounds checks, but ensure each atoi
call site is replaced or wrapped so bad input is detected immediately.
In `@PySpice/Spice/HSpice/hspicefile/hspice_read.h`:
- Around line 18-50: Add an include guard to hspice_read.h to prevent duplicate
definition issues if it is included multiple times. Wrap the existing
declarations for FastArray, HSpiceFormat, ParserContext, HSpiceRead, and
HSpiceParseError with a standard guard using a unique macro name for this
header, or use `#pragma` once if that matches the project style.
In `@PySpice/Spice/HSpice/RawFile.py`:
- Line 140: The AnalysisList and each analysis are currently sharing the same
measurements dictionary reference via analysis._measurements and
self.measurements, which can cause accidental cross-analysis mutation. Update
the RawFile loading path that builds the AnalysisList so each analysis gets its
own shallow copy of self.measurements instead of the shared dict, and keep the
AnalysisList-level measurements assignment consistent with that behavior.
- Around line 10-39: The AnalysisList attribute lookup is too aggressive:
__getattribute__ currently checks _measurements before normal attribute
resolution, so measurement names can shadow real list methods/properties like
count, index, append, sort, or measurements. Move the measurement fallback into
__getattr__ so built-in attributes on AnalysisList and list always win, and keep
the lowercase lookup there as well. Also align AnalysisList.__getitem__ with
normal conventions by raising KeyError for missing string measurement names and
IndexError only for unsupported positional indexing.
In `@PySpice/Spice/HSpice/Server.py`:
- Around line 44-73: `acquire_hspice_lock` currently waits forever in its `while
True` polling loop, so callers can still block indefinitely even though
`subprocess.run` has a timeout. Update `acquire_hspice_lock` in `Server.py` to
accept an overall timeout, track elapsed time while trying `fcntl.flock`, and
raise `TimeoutError` when no slot is acquired in time. Then thread that timeout
through the call sites in `Server` so the lock acquisition behavior matches the
subprocess timeout path.
---
Duplicate comments:
In `@PySpice/Spice/HSpice/hspicefile/hspice_read.c`:
- Around line 747-751: In hspice_read(), the PyArg_ParseTuple failure path still
returns Py_None instead of propagating the exception. Update the error-handling
branch after PyArg_ParseTuple(args, "si", &fileName, &debugMode) fails to return
NULL directly, so the exception set by the parser is preserved; use the
hspice_read() argument parsing block as the place to fix it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e0072cd4-65a1-4e70-a4a9-6fd44ccc1763
⛔ Files ignored due to path filters (1)
PySpice/Spice/HSpice/hspicefile/docs/file_structure.svgis excluded by!**/*.svg
📒 Files selected for processing (20)
.gitignorePySpice/Spice/HSpice/RawFile.pyPySpice/Spice/HSpice/Server.pyPySpice/Spice/HSpice/Simulator.pyPySpice/Spice/HSpice/hspicefile/__init__.pyPySpice/Spice/HSpice/hspicefile/build.pyPySpice/Spice/HSpice/hspicefile/docs/hspice_file_structure.mdPySpice/Spice/HSpice/hspicefile/docs/tests/.clang-formatPySpice/Spice/HSpice/hspicefile/docs/tests/.gitignorePySpice/Spice/HSpice/hspicefile/docs/tests/MakefilePySpice/Spice/HSpice/hspicefile/docs/tests/run_simulations.pyPySpice/Spice/HSpice/hspicefile/docs/tests/test_hspice_read.pyPySpice/Spice/HSpice/hspicefile/docs/tests/verify_all_types.pyPySpice/Spice/HSpice/hspicefile/docs/tests/verify_spec.pyPySpice/Spice/HSpice/hspicefile/hspice_read.cPySpice/Spice/HSpice/hspicefile/hspice_read.hPySpice/Spice/HSpice/hspicefile/release_notePySpice/Spice/Simulator.pyPySpice/Spice/SimulatorBase.pysetup_data.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
PySpice/Spice/HSpice/Server.py (1)
62-76: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove the timeout check out of the per-slot
tryblock.Line 73 is unreachable for normal contention:
BlockingIOErrorhitscontinue, while success returns at Line 70. The lock wait can still block forever when all slots are busy.🐛 Proposed fix
start = time.monotonic() while True: for fd, idx in lock_files: try: fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) for other_fd, other_idx in lock_files: if other_idx != idx: os.close(other_fd) return fd, idx except BlockingIOError: continue - if timeout is not None and time.monotonic() - start > timeout: - for fd, _ in lock_files: - os.close(fd) - raise TimeoutError("Timed out waiting for an HSPICE slot.") + if timeout is not None and time.monotonic() - start > timeout: + for lock_fd, _ in lock_files: + os.close(lock_fd) + raise TimeoutError("Timed out waiting for an HSPICE slot.") time.sleep(0.05)🤖 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 `@PySpice/Spice/HSpice/Server.py` around lines 62 - 76, The timeout check in Server._acquire_lock is unreachable because the BlockingIOError path continues before it can run, so contention can wait forever. Move the timeout handling out of the per-slot try/except inside the lock-acquisition loop, and check elapsed time in the outer while True loop after attempting all lock_files; use the existing timeout logic and TimeoutError path in _acquire_lock to enforce the limit.
🤖 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 `@PySpice/Spice/HSpice/hspicefile/hspice_read.c`:
- Around line 381-385: The sweep-size validation in the hspice parser currently
only checks that parse_int succeeds, so zero or negative values can slip through
and produce an empty sweep. Update the sweep-size handling in hspice_read.c
after parse_int for sweepSize to explicitly reject any value less than or equal
to zero, emit a PyErr_Format(HSpiceParseError, ...) message for non-positive
sizes, and return -1 from the same parsing path.
---
Outside diff comments:
In `@PySpice/Spice/HSpice/Server.py`:
- Around line 62-76: The timeout check in Server._acquire_lock is unreachable
because the BlockingIOError path continues before it can run, so contention can
wait forever. Move the timeout handling out of the per-slot try/except inside
the lock-acquisition loop, and check elapsed time in the outer while True loop
after attempting all lock_files; use the existing timeout logic and TimeoutError
path in _acquire_lock to enforce the limit.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6250fa44-1156-4f27-ad48-38f7c7e6d23a
📒 Files selected for processing (7)
PySpice/Spice/HSpice/RawFile.pyPySpice/Spice/HSpice/Server.pyPySpice/Spice/HSpice/Simulator.pyPySpice/Spice/HSpice/hspicefile/build.pyPySpice/Spice/HSpice/hspicefile/docs/tests/test_hspice_read.pyPySpice/Spice/HSpice/hspicefile/hspice_read.cPySpice/Spice/HSpice/hspicefile/hspice_read.h
infinitymdm
left a comment
infinitymdm
left a comment
There was a problem hiding this comment.
LGTM. I'm testing with CharLib (alongside stineje/CharLib#107). Assuming that works, this should be good to merge.
|
With your PRs Charlib is working with Interestingly, this happens at the time of the With the above in mind, I'm in favor of moving forward with merging this. It enables I'm also curious about whether you're running into this. Did you run into these errors and already find a workaround? |
…517 error. Becasue setup_data refers to build.py it was trying to import the whole module during packaging and key dependencies are not avialable during that process
|
I actually hadnt seen this yet, but did find a bug with how the C module is built during packaging. Thats the latest commit I've just pushed and that seems to resolve it. I will try and have a look into this because I'm just getting started on building up my flow. I want to get it working with the internal PDKs first before I try any other libraries. |
I have figured this one out and have a fix so I'll include that here before we merge, feel free to delay if you want to review the changes |
|
Note Unit test generation is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
1 similar comment
|
Note Unit test generation is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
❌ Failed to create PR with unit tests: AGENT_CHAT: Failed to open pull request |
|
Request timed out after 900000ms (requestId=0c33c5e0-e008-4dc3-914e-21c8b2970f98) |
…ser was struggling with whitespace so this has been resolved in a different way
AidenDawn
force-pushed
the
feature/hspice-backend
branch
from
7d248c6 to
f175f47
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
PySpice/Spice/HSpice/Server.py (1)
44-76: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPass the configured timeout into lock acquisition.
acquire_hspice_lock()now supportstimeout, but Line 119 still calls it withoutself._timeout, so exhausted license slots can still block indefinitely before the subprocess timeout applies.🩺 Proposed fix
- lock_fd, slot_idx = acquire_hspice_lock(self._concurrency_limit) + lock_fd, slot_idx = acquire_hspice_lock( + self._concurrency_limit, + timeout=self._timeout, + )Also applies to: 118-120
🤖 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 `@PySpice/Spice/HSpice/Server.py` around lines 44 - 76, Pass the configured timeout through the HSPICE lock path so slot acquisition cannot block indefinitely; update the caller in the server flow that uses acquire_hspice_lock() to provide self._timeout (the same timeout used for the subprocess), and verify the Hspice Server lock-acquisition sequence still releases/cleans up lock file descriptors correctly when timeout is hit.
🤖 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 1-3: The build configuration in the [build-system] section
currently allows building against a newer NumPy than the runtime floor supported
by install_requires, which can lead to incompatible extensions. Update
pyproject.toml so the build dependency for NumPy is constrained to the oldest
supported runtime version, either by replacing the plain numpy entry with
oldest-supported-numpy or by pinning the build-time NumPy version to match the
minimum supported runtime.
In `@PySpice/Spice/HSpice/Server.py`:
- Around line 331-346: The measurement parsing logic in Server.py should
validate that the flattened data width matches the header width before entering
the strided loop over names and data_tokens. Add a guard in the parsing block
that checks len(data_tokens) % header_len and, if it is non-zero, fails or skips
the malformed file before indexing into data_tokens in the loop that uses
name_lower and j stepping by header_len. Keep the fix localized to the existing
names/data_tokens/header_len handling so the column slicing never silently
shifts.
---
Outside diff comments:
In `@PySpice/Spice/HSpice/Server.py`:
- Around line 44-76: Pass the configured timeout through the HSPICE lock path so
slot acquisition cannot block indefinitely; update the caller in the server flow
that uses acquire_hspice_lock() to provide self._timeout (the same timeout used
for the subprocess), and verify the Hspice Server lock-acquisition sequence
still releases/cleans up lock file descriptors correctly when timeout is hit.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0229ec8b-f12d-4161-b89b-8b88872ad732
📒 Files selected for processing (9)
PySpice/Spice/HSpice/RawFile.pyPySpice/Spice/HSpice/Server.pyPySpice/Spice/HSpice/Simulator.pyPySpice/Spice/HSpice/hspicefile/docs/tests/test_hspice_read.pyPySpice/Spice/HSpice/hspicefile/hspice_read.cPySpice/Spice/HSpice/hspicefile/hspice_read.hpyproject.tomlsetup.pysetup_data.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@PySpice/Spice/HSpice/Server.py`:
- Around line 119-122: The unpacking in the HSpice server lock acquisition path
includes an unused slot variable, which triggers Ruff RUF059. Update the
destructuring in the Server method that calls acquire_hspice_lock to use a dummy
name for the discarded second value instead of binding slot_idx, keeping lock_fd
for the value that is actually used.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e9f15f6e-61ea-4b15-8309-652d1da2d917
📒 Files selected for processing (2)
PySpice/Spice/HSpice/Server.pypyproject.toml
|
Works great in my testing, including |
2 participants
This pull request adds PySpice support for the HSpice commerical simulator.
It is broken into 2 commits.
Commit 1: Update the API for simulators to depend on a base class. This makes adding more simulators more straightforward and makes no mention of HSpice.
Commit 2: Add support for HSpice.
Things get complicated here because rather than replying on parsing ASCII outputs, HSpice uses a binary output format. A new binary parser is included in hspicefile taken from the PyOPUS and updated to support newer binary output formats (post-2013). Details of the format are included in hspicefile/docs.
With commercial simulators, available licenses needs to be taken into account. The first option which I have used here is to limit the number of processes that can be executed at once. This is the concurrency_limit option in the PySpice Factory and can also be driven by the PYSPICE_HSPICE_CONCURRENCY_LIMIT environment variable. Another good option for the cell characterisation workflow could also be the HSPICE Client/Server Mode, but setting this up to run automatically would require a much larger number of changes to the project than this first version warrants.
I was running into some issues with the simulations when running sequential c2q characterisation, but I can see that you have been doing some work on that here. Hopefully that would resolve the issue with HSpice as well.
Summary by CodeRabbit