Introduce custom sympy srepr parser #14024
Merged
Conversation
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
This commit introduces a custom parser to QPY for parameter expression payloads that were generated using sympy. Prior to QPY version 10 this was the only way we supported serializing parameter expressions in QPY. For QPY version 10, 11, and 12 sympy could optionally be used if the payload was generated explicitly to not use symengine (in qiskit 1.0 it defaulted to use symengine). This serialization format relied on sympy to generate a string representation of the expression which we then put in the payload. On deserialization we called sympy's `parse_expr()` function which internally is calling sympy's `sympify()` internally. Sympy documents that `sympify()` relies on Python's `eval()` for string input and should not be used with untrusted input. But `parse_expr()` didn't have such a warning (at the time, I plan to contribute adding one), so using this function provides an avenue for arbitrary code execution during QPY deserialization. This commit fixes this issue by writing a custom parser for the string repesentation in a QPY payload based on python's ast module. This parser walks the abstract syntax tree and builds the sympy expression object as it it goes. It is restricted to the operations that `ParameterExpression` supports and if any part of the string tries to use functionality outside that set it will error.
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 13860814686Details
💛 - Coveralls |
raynelfss
pushed a commit
to raynelfss/qiskit
that referenced
this pull request
Mar 20, 2025
* Introduce custom sympy srepr parser This commit introduces a custom parser to QPY for parameter expression payloads that were generated using sympy. Prior to QPY version 10 this was the only way we supported serializing parameter expressions in QPY. For QPY version 10, 11, and 12 sympy could optionally be used if the payload was generated explicitly to not use symengine (in qiskit 1.0 it defaulted to use symengine). This serialization format relied on sympy to generate a string representation of the expression which we then put in the payload. On deserialization we called sympy's `parse_expr()` function which internally is calling sympy's `sympify()` internally. Sympy documents that `sympify()` relies on Python's `eval()` for string input and should not be used with untrusted input. But `parse_expr()` didn't have such a warning (at the time, I plan to contribute adding one), so using this function provides an avenue for arbitrary code execution during QPY deserialization. This commit fixes this issue by writing a custom parser for the string repesentation in a QPY payload based on python's ast module. This parser walks the abstract syntax tree and builds the sympy expression object as it it goes. It is restricted to the operations that `ParameterExpression` supports and if any part of the string tries to use functionality outside that set it will error. * Simplify visitor logic * Fix lint * Add sanity checks that we only call sympify if coming from symengine * Apply suggestions from code review Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> --------- Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
doichanj
added a commit
to doichanj/qiskit
that referenced
this pull request
Apr 22, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 16, 2025
* Add SymbolicExExpression in Rust This PR is a new implementation of ParameterExpression written in Rust for issue #13267 This PR implements our own symbolic expression library written in Rust, this replaces symengine from ParameterExpression class. `crates/circuit/symbol_expr.rs` is a symbolic expression module. Equations with symbols and values are expressed by using AST structure with nodes of operations. This module contains all the operators defined in Python's ParameterExpression class. To separate Rust's implementation from Python we prepared Python interface class `crates/circuit/symbol_expr_py.rs` that is called from ParameterExpression in Python, instead of symengine. `crates/circuit/parameterexpression.rs` is the ParameterExpression class for Rust. It provides similar functionalities to Python version. * Remove unused parameterexpression.rs file This file is more future facing and doesn't have any current uses. To simplify the PR to just the working component this commit removes the file. We can add it back in the future when there is use case for the module. * Change py suffix to prefix This commit just renames the symbol_expr_py file and module to py_symbol_expr. We typically use a Py prefix when describing objects that are just for python. This change was just done to make this new module match that expectation. * Fix qiskit-circuit cargo dependencies The Cargo.toml for qiskit-circuit was missing some details to use the approx crate correctly with Complex64 types and arrays of those types. This didn't bubble up in combined builds because cargo was reusing the compilation unit for these crates with accelerate which had the correct settings. But when trying to do a standalone build of qiskit-circuit this caused a compilation error. This commit fixes this by updating the local dependencies to ensure the appropriate features to use approx with complex and ndarray are set. * Use unix style newlines for symbol_parser.rs * Remove unused extern crate lines * Run cargo fmt * add some missing fuction to Python interface * fix sqrt * add values function to enumerate all values in expression for pi_check.py * fix for lint * fix for lint error * Make conjugate a unary op In testing the conjugate was not working for parameters. This is because it was not being treated as a unary op. Values were correctly changing the sign of the complex component, but symbols and expressions would not. To fix this the conjugate was added as a unary op which is tracked added to the expression tree. * Add test module for validating all parameter expression ops This commit expands the test coverage for the parameter expression class to ensure that we have coverage of all the mathemetical operations that can be performed on an expression and does so with combinations of operand types to try and provide more thorough coverage of the class. The only method missing from the tet right now is gradient which can be added later. * Add tolerance on new unit tests for multiplication with complex bind This commit adds tolerance to the floating point comparison on the multiplication test with binding a complex number. Running these tests in CI was showing failures on different platforms caused by floating point precision differences. This should fix the failures in CI. * Fix pow logic for complex numbers The pow logic was overeagerly casting values to complex whenever the lhs of the operation was negative. The output is only complex if the lhs is negative and the rhs has a fractional component. This casting to a complex was accumulating a small imaginary component because of fp precision even though the values shouldn't be complex. This commit fixes this issue by adjusting the pow logic to only cast a real or integer value to a complex if it's negative and the rhs has a fractional component. * Fix more pow impl and expand coverage * Remove unused pyfunctions * Add more test coverage * More test coverage * Add tests for .numeric() * Add tolerance to test_pow_complex_binding This test was failing in CI on Windows due to differing fp precision. To fix the failure this adjusts the tests to use assertAlmostEqual instead of assertEqual to account for the platform differences. * modify SymbolExpr enum structure * Fix add/sub optimization * Fix PartialOrd for Binary * remove Symbol struct, add optimization function and remove on-the-fly optimization * add optimization for values, fix equality check * remove debug prints * reformat for lint test * fix cargo error * fix for lint * fix for lint * Fix np.complex128 issue, remove codes for exceptions for np.complex128 * fix for lint * fix for lint * Fix symbol_parser.rs, remove BinaryOpContainer, reordering pow operator * replace rhs to lhs in __r*__ functions, add multispace0 for unary op parser * remove unused imports * move replacements of backslashes for sympify into rust * fix for lint * remove enum for Python multi-types and replace them to PyObject * optimize __eq__ when comparing with value * remove optimmize() from __hash__ * rename PySymbolExpr to ParameterExpression and implemented some function so that we can use both from rust and python * Fix log and div * Expand dedicated ParameterExpression testing * Remove unicode charcter to fix windows charmap failure * recover value.py as #14024 * lint * reflect review comments * cargo fmt * remove unused imports * fix cloning Values * format * add error handling on parse_expression * add exception to is_real when there are unbound parameters * Revert "add exception to is_real when there are unbound parameters" This reverts commit 1c98127. * fix derivative of asin/acos, raise error for sign/conj. Fix sympify with complex numbers * restore test_pauli_feature_map, add comment for epsilon * format * fix for lint * remove debug print * format * Additional tests and gradient note - Added a note that the gradient of parameters is only defined for real parameter expressions - Added a test to call SparsePauliOp.simplify on with coefficients that have complex multipliers - Added numerical checks for derivatives * Update crates/circuit/src/parameter_expression.rs Co-authored-by: Julien Gacon <gaconju@gmail.com> * Update crates/circuit/src/parameter_expression.rs Co-authored-by: Julien Gacon <gaconju@gmail.com> * Update docs / comments and fix conjugate deriv - Update docs: we no longer rely on symengine/sympy for parameters - Reword reno: we still install symengine/sympy at this point - Fix conjugate derivative: since conj() acts as identity on real numbers, the derivative is 1 --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: Julien Gacon <jules.gacon@googlemail.com> Co-authored-by: Julien Gacon <gaconju@gmail.com>
rahaman-quantum
pushed a commit
to rahaman-quantum/qiskit
that referenced
this pull request
Jun 20, 2025
* Add SymbolicExExpression in Rust This PR is a new implementation of ParameterExpression written in Rust for issue Qiskit#13267 This PR implements our own symbolic expression library written in Rust, this replaces symengine from ParameterExpression class. `crates/circuit/symbol_expr.rs` is a symbolic expression module. Equations with symbols and values are expressed by using AST structure with nodes of operations. This module contains all the operators defined in Python's ParameterExpression class. To separate Rust's implementation from Python we prepared Python interface class `crates/circuit/symbol_expr_py.rs` that is called from ParameterExpression in Python, instead of symengine. `crates/circuit/parameterexpression.rs` is the ParameterExpression class for Rust. It provides similar functionalities to Python version. * Remove unused parameterexpression.rs file This file is more future facing and doesn't have any current uses. To simplify the PR to just the working component this commit removes the file. We can add it back in the future when there is use case for the module. * Change py suffix to prefix This commit just renames the symbol_expr_py file and module to py_symbol_expr. We typically use a Py prefix when describing objects that are just for python. This change was just done to make this new module match that expectation. * Fix qiskit-circuit cargo dependencies The Cargo.toml for qiskit-circuit was missing some details to use the approx crate correctly with Complex64 types and arrays of those types. This didn't bubble up in combined builds because cargo was reusing the compilation unit for these crates with accelerate which had the correct settings. But when trying to do a standalone build of qiskit-circuit this caused a compilation error. This commit fixes this by updating the local dependencies to ensure the appropriate features to use approx with complex and ndarray are set. * Use unix style newlines for symbol_parser.rs * Remove unused extern crate lines * Run cargo fmt * add some missing fuction to Python interface * fix sqrt * add values function to enumerate all values in expression for pi_check.py * fix for lint * fix for lint error * Make conjugate a unary op In testing the conjugate was not working for parameters. This is because it was not being treated as a unary op. Values were correctly changing the sign of the complex component, but symbols and expressions would not. To fix this the conjugate was added as a unary op which is tracked added to the expression tree. * Add test module for validating all parameter expression ops This commit expands the test coverage for the parameter expression class to ensure that we have coverage of all the mathemetical operations that can be performed on an expression and does so with combinations of operand types to try and provide more thorough coverage of the class. The only method missing from the tet right now is gradient which can be added later. * Add tolerance on new unit tests for multiplication with complex bind This commit adds tolerance to the floating point comparison on the multiplication test with binding a complex number. Running these tests in CI was showing failures on different platforms caused by floating point precision differences. This should fix the failures in CI. * Fix pow logic for complex numbers The pow logic was overeagerly casting values to complex whenever the lhs of the operation was negative. The output is only complex if the lhs is negative and the rhs has a fractional component. This casting to a complex was accumulating a small imaginary component because of fp precision even though the values shouldn't be complex. This commit fixes this issue by adjusting the pow logic to only cast a real or integer value to a complex if it's negative and the rhs has a fractional component. * Fix more pow impl and expand coverage * Remove unused pyfunctions * Add more test coverage * More test coverage * Add tests for .numeric() * Add tolerance to test_pow_complex_binding This test was failing in CI on Windows due to differing fp precision. To fix the failure this adjusts the tests to use assertAlmostEqual instead of assertEqual to account for the platform differences. * modify SymbolExpr enum structure * Fix add/sub optimization * Fix PartialOrd for Binary * remove Symbol struct, add optimization function and remove on-the-fly optimization * add optimization for values, fix equality check * remove debug prints * reformat for lint test * fix cargo error * fix for lint * fix for lint * Fix np.complex128 issue, remove codes for exceptions for np.complex128 * fix for lint * fix for lint * Fix symbol_parser.rs, remove BinaryOpContainer, reordering pow operator * replace rhs to lhs in __r*__ functions, add multispace0 for unary op parser * remove unused imports * move replacements of backslashes for sympify into rust * fix for lint * remove enum for Python multi-types and replace them to PyObject * optimize __eq__ when comparing with value * remove optimmize() from __hash__ * rename PySymbolExpr to ParameterExpression and implemented some function so that we can use both from rust and python * Fix log and div * Expand dedicated ParameterExpression testing * Remove unicode charcter to fix windows charmap failure * recover value.py as Qiskit#14024 * lint * reflect review comments * cargo fmt * remove unused imports * fix cloning Values * format * add error handling on parse_expression * add exception to is_real when there are unbound parameters * Revert "add exception to is_real when there are unbound parameters" This reverts commit 1c98127. * fix derivative of asin/acos, raise error for sign/conj. Fix sympify with complex numbers * restore test_pauli_feature_map, add comment for epsilon * format * fix for lint * remove debug print * format * Additional tests and gradient note - Added a note that the gradient of parameters is only defined for real parameter expressions - Added a test to call SparsePauliOp.simplify on with coefficients that have complex multipliers - Added numerical checks for derivatives * Update crates/circuit/src/parameter_expression.rs Co-authored-by: Julien Gacon <gaconju@gmail.com> * Update crates/circuit/src/parameter_expression.rs Co-authored-by: Julien Gacon <gaconju@gmail.com> * Update docs / comments and fix conjugate deriv - Update docs: we no longer rely on symengine/sympy for parameters - Reword reno: we still install symengine/sympy at this point - Fix conjugate derivative: since conj() acts as identity on real numbers, the derivative is 1 --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: Julien Gacon <jules.gacon@googlemail.com> Co-authored-by: Julien Gacon <gaconju@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
This commit introduces a custom parser to QPY for parameter expression payloads that were generated using sympy. Prior to QPY version 10 this was the only way we supported serializing parameter expressions in QPY. For QPY version 10, 11, and 12 sympy could optionally be used if the payload was generated explicitly to not use symengine (in qiskit 1.0 it defaulted to use symengine).
This serialization format relied on sympy to generate a string representation of the expression which we then put in the payload. On deserialization we called sympy's parse_expr() function which internally is calling sympy's sympify() internally. Sympy documents that sympify() relies on Python's eval() for string input and should not be used with untrusted input. But parse_expr() didn't have such a warning (at the time, I plan to contribute adding one), so using this function provides an avenue for arbitrary code execution during QPY deserialization.
This commit fixes this issue by writing a custom parser for the string representation in a QPY payload based on python's ast module. This parser walks the abstract syntax tree and builds the sympy expression object as it it goes. It is restricted to the operations that ParameterExpression supports and if any part of the string tries to use functionality outside that set it will error.
Details and comments