implement Decimal to rust_decimal conversions #3016

cardoe · 2023-03-05T22:51:18Z

Implement conversion between rust_decimal::Decimal and decimal.Decimal from Python's stdlib. The C API does not appear to be exposed on the Python side so we need to call into it via Python.

Implements #2774

cardoe · 2023-03-05T22:55:20Z

I created the PR as suggested by @davidhewitt in #2774 to start the conversation. I added an initial benchmark but I'm not sure if it's sufficient to start. The results were:

Benchmarking decimal_via_extract: Collecting 100 samples in estimated 5.0001 s (
decimal_via_extract     time:   [46.788 ns 46.803 ns 46.819 ns]
Found 15 outliers among 100 measurements (15.00%)
  7 (7.00%) low severe
  5 (5.00%) low mild
  2 (2.00%) high mild
  1 (1.00%) high severe

not sure why I had that trailing open paren.

The tests fail because I haven't written any docs yet. I've still got to address @adamreichold's feedback as well but I figured I'd start with the benchmarks to see if it makes sense and if not talk to CPython upstream about exposing an API. If the benchmarks are okay then I'll work in parallel. Add the necessary bits to raise this to completion while getting an API exposed in CPython.

adamreichold · 2023-03-06T08:03:47Z

Personally, I think that independently of the kind of (modernish) system this was run on, that the performance is reasonable and would have this than have it not, understanding that we can always improve things if better CPython API become available.

mejrs · 2023-03-06T10:56:06Z

IMO we should only have these conversions if they're better than what a user could naively write themselves. Python's decimal has no C api, so this isn't the case here.

adamreichold · 2023-03-06T11:09:13Z

IMO we should only have these conversions if they're better than what a user could naively write themselves. Python's decimal has no C api, so this isn't the case here.

But why force the user into reinventing this if we can solve the problem once for everybody? As long as it is the best possible solution. And if a better way comes along, we can improve things in a single place and everyone benefits immediately.

cardoe · 2023-03-07T02:03:01Z

@mejrs My thought process for this PR was that decimal is part of the stdlib as a type. I don't think it makes sense to add every part of stdlib but looking through most of the types (I'm counting the types under the numerical as well) are covered except for big integers which is what this aims to cover.

kngwyu · 2023-03-07T06:14:57Z

I'm not actively catching up on the project, so I'm not sure, but is there any other module where we support conversion between some Rust library and Python object which doesn't have C-API? If so, having this would be OK.

But I'm not very convinced with this string-based conversion... If we can use tuple instead, it would be faster.

davidhewitt · 2023-03-07T08:25:26Z

IMO we should only have these conversions if they're better than what a user could naively write themselves. Python's decimal has no C api, so this isn't the case here.

I keep going back and forth on this line of thinking. I think ultimately the types with a C-API are not significantly different except that we wrap away the unsafe parts.

I do agree with @adamreichold's argument that getting this once right here is better for the ecosystem than forcing fragmentation. Also we essentially use Python APIs for PySequence and PyMapping now that we use the collections base classes, so perhaps we have to accept that to make "nice" bindings between the two languages we have to accept the Python C-API limitations at times?

Although I still agree it would be great to have a native conversion provided upstream, if there is willingness.

But I'm not very convinced with this string-based conversion... If we can use tuple instead, it would be faster.

It's not clear to me which conversion options are available, however it would be good to sense-check what we're offering here is the most efficient.

davidhewitt · 2023-03-07T08:26:21Z

Also of note is that we do have similar precedent with support for converting pathlib.Path into Rust types.

cardoe · 2023-03-07T17:47:25Z

So the reason I went with the string over the tuple is that the Python tuple doesn't map to the Rust interface nicely.

The Python tuple documented at https://docs.python.org/3/library/decimal.html#decimal.Decimal says its

If value is a tuple, it should have three components, a sign (0 for positive or 1 for negative), a tuple of digits, and an integer exponent. For example, Decimal((0, (1, 4, 1, 4), -3)) returns Decimal('1.414')
which would be returned from https://docs.python.org/3/library/decimal.html#decimal.Decimal.as_tuple

We'd have a couple of weird corner cases with Infinity and NaN via as_tuple as well.

>>> nan = Decimal('NaN')
>>> nan.as_tuple()
DecimalTuple(sign=0, digits=(), exponent='n')
>>> inf = Decimal('Infinity')
>>> inf.as_tuple()
DecimalTuple(sign=0, digits=(0,), exponent='F')

Now from the Rust side the closest to the tuple interface is probably new() or from_str_radix(). https://docs.rs/rust_decimal/1.28.1/rust_decimal/struct.Decimal.html#method.new and https://docs.rs/rust_decimal/1.28.1/rust_decimal/struct.Decimal.html#method.from_str_radix

new() takes in the digits as a signed number and the exponent. So that will require some conversion of the digits to an i64
from_str_radix() would take the digits as a &str and would probably be easier to convert than the above but will cause a temp allocation. But this would probably perform better in the Rust side than the Python side.

So ultimately if that's approach you'd like me to take I think I'd steer towards from_str_radix() if that sounds okay?

davidhewitt

I don't have any preference for one implementation over the other. If you're willing to experiment, it would be good to compare from_str_radix and from_str_exact (if those are the candidates we're looking at), to see if there's any meaningful implementation and performance differences.

src/conversions/rust_decimal.rs

kngwyu · 2023-03-08T03:49:13Z

@cardoe
Thank you for your explanation. String-based conversion sounds reasonable for me now.

cardoe · 2023-03-16T05:29:33Z

I believe this addresses all the feedback thus far and could be the first commit considered for a full review / inclusion.

cardoe · 2023-03-16T21:56:36Z

I think this fixed the MSRV issue.

cardoe · 2023-03-16T22:05:36Z

I've just got an M1 based Mac to test on right now so the MSRV won't build on here so fingers crossed really.

davidhewitt · 2023-03-18T21:04:58Z

Hello, I'll do my best to review this tomorrow. A bit behind on OSS this week, had a sick family. We're better now finally :)

birkenfeld · 2023-03-19T07:43:40Z

src/conversions/rust_decimal.rs

+#![cfg_attr(docsrs, cfg_attr(docsrs, doc = concat!("pyo3 = { version = \"", env!("CARGO_PKG_VERSION"),  "\", features = [\"rust_decimal\"] }")))]
+#![cfg_attr(
+    not(docsrs),
+    doc = "pyo3 = { version = \"*\", features = [\"rust_decimal\"] }"


Maybe not encourage version = "*" but use version = ... to indicate something to replace?

I'll add a comment above like:

pyo3/src/conversions/anyhow.rs

Line 31 in ebd417c

doc = "pyo3 = { version = \"*\", features = [\"anyhow\"] }"

pyo3/src/conversions/chrono.rs

Line 21 in ebd417c

doc = "pyo3 = { version = \"*\", features = [\"chrono\"] }"

The others don't have one. I followed num-bigint as an example when writing this.

Yep fine to leave like this for now. When we update MSRV above 1.54 we can simplify this and just have the CARGO_PKG_VERSION embedded everywhere.

I am sorry for re-opening, but I do think @birkenfeld's suggestion of using version = ... instead is better. As it stands now, I get the comment about * on docs.rs even though docs.rs uses CARGO_PKG_VERSION and hence there is no * in sight.

Sure. I was just trying to match the style of the rest of the code base.

The version specification looks good now but it looks like you did not remove the comment about *?

Took the liberty of amending that commit and will merge this now as I think anything has been addressed now. Any other improvements can be follow-ups.

src/conversions/rust_decimal.rs

cardoe · 2023-03-19T15:50:29Z

Hello, I'll do my best to review this tomorrow. A bit behind on OSS this week, had a sick family. We're better now finally :)

Take you time. Family first. OSS is the shoulder monkey that never goes away for better or worse. I'm not in any rush on this. I was playing around with a Python code base and grumping at "type whispering" biting me on something and though I would try to lift part of the code base into Rust and looked for Decimal didn't see it. So I wrote a wrapper type for my use case but figured since there was at least one other person looking for it that I would try to contribute back to make it easier for the next person.

DataTriny · 2023-04-16T09:27:36Z

First time contributor has agreed to the new licensing scheme.

cardoe · 2023-04-21T13:56:15Z

I believe I've addressed all the PR feedback. Please let me know what I can do going forward. The current GitHub actions failures are happening for me locally on my machine with the latest main commit as well.

adamreichold · 2023-04-21T17:02:06Z

@cardoe Could you rebase on main now that #3114 has landed?

adamreichold

Sorry doing another round on this.

benches/bench_decimal.rs

Cargo.toml

adamreichold · 2023-04-22T06:29:30Z

src/conversions/rust_decimal.rs

+#![cfg_attr(docsrs, cfg_attr(docsrs, doc = concat!("pyo3 = { version = \"", env!("CARGO_PKG_VERSION"),  "\", features = [\"rust_decimal\"] }")))]
+#![cfg_attr(
+    not(docsrs),
+    doc = "pyo3 = { version = \"*\", features = [\"rust_decimal\"] }"


I am sorry for re-opening, but I do think @birkenfeld's suggestion of using version = ... instead is better. As it stands now, I get the comment about * on docs.rs even though docs.rs uses CARGO_PKG_VERSION and hence there is no * in sight.

src/conversions/rust_decimal.rs

adamreichold

There still is the left-over comment concerning the * version specification, otherwise I think this is good to land. Thanks for keeping at it!

Implement conversion between rust_decimal::Decimal and decimal.Decimal from Python's stdlib. The C API does not appear to be exposed on the Python side so we need to call into it via Python.

adamreichold · 2023-04-22T17:56:17Z

bors r+

3016: implement Decimal to rust_decimal conversions r=adamreichold a=cardoe Implement conversion between rust_decimal::Decimal and decimal.Decimal from Python's stdlib. The C API does not appear to be exposed on the Python side so we need to call into it via Python. Implements #2774 Co-authored-by: Doug Goldstein <cardoe@cardoe.com>

bors · 2023-04-22T18:27:24Z

Build failed:

conclusion

adamreichold · 2023-04-22T18:27:51Z

bors retry

bors · 2023-04-22T19:05:39Z

Build succeeded:

conclusion

davidhewitt reviewed Mar 7, 2023

View reviewed changes

src/conversions/rust_decimal.rs Show resolved Hide resolved

src/conversions/rust_decimal.rs Show resolved Hide resolved

cardoe mentioned this pull request Mar 8, 2023

Decimal precision mismatches against Python Decimal paupino/rust-decimal#471

Open

cardoe force-pushed the decimal branch 4 times, most recently from 02ddcae to 65f3f8d Compare March 16, 2023 05:24

cardoe changed the title ~~rough swag at rust_decimal support~~ implement Decimal to rust_decimal conversions Mar 16, 2023

cardoe force-pushed the decimal branch from 65f3f8d to 63a7853 Compare March 16, 2023 05:28

cardoe force-pushed the decimal branch 2 times, most recently from 023d669 to 2655a61 Compare March 16, 2023 21:55

cardoe force-pushed the decimal branch 5 times, most recently from ee2466f to c029ed0 Compare March 17, 2023 15:49

cardoe requested a review from davidhewitt March 17, 2023 20:06

birkenfeld reviewed Mar 19, 2023

View reviewed changes

cardoe force-pushed the decimal branch from c029ed0 to 2de35ef Compare March 19, 2023 15:56

cardoe force-pushed the decimal branch from 2de35ef to adff104 Compare April 21, 2023 13:49

cardoe force-pushed the decimal branch 2 times, most recently from 32540a8 to c275a9a Compare April 21, 2023 20:51

cardoe requested review from adamreichold and birkenfeld April 21, 2023 21:56

adamreichold requested changes Apr 22, 2023

View reviewed changes

cardoe force-pushed the decimal branch from c275a9a to e83f8b2 Compare April 22, 2023 15:40

adamreichold approved these changes Apr 22, 2023

View reviewed changes

implement Decimal to rust_decimal conversions

8e0bde1

Implement conversion between rust_decimal::Decimal and decimal.Decimal from Python's stdlib. The C API does not appear to be exposed on the Python side so we need to call into it via Python.

adamreichold force-pushed the decimal branch from e83f8b2 to 8e0bde1 Compare April 22, 2023 17:55

bors bot merged commit 3e21797 into PyO3:main Apr 22, 2023
33 checks passed

cardoe deleted the decimal branch April 22, 2023 21:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement Decimal to rust_decimal conversions #3016

implement Decimal to rust_decimal conversions #3016

cardoe commented Mar 5, 2023 •

edited

cardoe commented Mar 5, 2023

adamreichold commented Mar 6, 2023

mejrs commented Mar 6, 2023

adamreichold commented Mar 6, 2023

cardoe commented Mar 7, 2023

kngwyu commented Mar 7, 2023

davidhewitt commented Mar 7, 2023

davidhewitt commented Mar 7, 2023

cardoe commented Mar 7, 2023

davidhewitt left a comment

kngwyu commented Mar 8, 2023

cardoe commented Mar 16, 2023

cardoe commented Mar 16, 2023

cardoe commented Mar 16, 2023

davidhewitt commented Mar 18, 2023

birkenfeld Mar 19, 2023

cardoe Mar 19, 2023

davidhewitt Mar 23, 2023

adamreichold Apr 22, 2023

cardoe Apr 22, 2023

adamreichold Apr 22, 2023

adamreichold Apr 22, 2023

cardoe commented Mar 19, 2023

DataTriny commented Apr 16, 2023 •

edited

cardoe commented Apr 21, 2023

adamreichold commented Apr 21, 2023

adamreichold left a comment

adamreichold Apr 22, 2023

adamreichold left a comment

adamreichold commented Apr 22, 2023

bors bot commented Apr 22, 2023

adamreichold commented Apr 22, 2023

bors bot commented Apr 22, 2023

implement Decimal to rust_decimal conversions #3016

implement Decimal to rust_decimal conversions #3016

Conversation

cardoe commented Mar 5, 2023 • edited

cardoe commented Mar 5, 2023

adamreichold commented Mar 6, 2023

mejrs commented Mar 6, 2023

adamreichold commented Mar 6, 2023

cardoe commented Mar 7, 2023

kngwyu commented Mar 7, 2023

davidhewitt commented Mar 7, 2023

davidhewitt commented Mar 7, 2023

cardoe commented Mar 7, 2023

davidhewitt left a comment

Choose a reason for hiding this comment

kngwyu commented Mar 8, 2023

cardoe commented Mar 16, 2023

cardoe commented Mar 16, 2023

cardoe commented Mar 16, 2023

davidhewitt commented Mar 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cardoe commented Mar 19, 2023

DataTriny commented Apr 16, 2023 • edited

cardoe commented Apr 21, 2023

adamreichold commented Apr 21, 2023

adamreichold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamreichold left a comment

Choose a reason for hiding this comment

adamreichold commented Apr 22, 2023

bors bot commented Apr 22, 2023

adamreichold commented Apr 22, 2023

bors bot commented Apr 22, 2023

cardoe commented Mar 5, 2023 •

edited

DataTriny commented Apr 16, 2023 •

edited