build(python): Update to PyO3 to 0.18.0

[jjerphan]

Follow-up of #4794

[jjerphan]

I still have tests failure because NumPy arrays containing strings cannot be converted to pl.Series. I am currently trying to understand why this is the case.

[ritchie46]

I believe it is related to PyO3/pyo3#2615

This blocked previous upgrade.

[ritchie46]

@jjerphan maybe we can add a new constructor specialized for this case?

Or we could ask upstream what is best way to do this in latest release?

[jjerphan]

I guess we can ask upstream and in the mean time have a new constructor which has been specialized for this case with a comment indicating that such a constructor is temporary.

What do you think of this approach?

[ritchie46]

Yes, I think that's a good idea.

[jjerphan]

Perfect.

I am busy now, but I should be able to have a look at it tomorrow.

I also have followed-up discussions with PyO3/pyo3#2615 (comment).

[adamreichold]

Could someone point me to where the entry point for those failing tests is so that I can check which downcast/extraction is failing exactly? Thank you!

[ritchie46]

@adamreichold I can come back to you in an hour. 👍

[adamreichold]

Could someone point me to where the entry point for those failing tests is so that I can check which downcast/extraction is failing exactly? Thank you!

So by digging and observing that this seems to affect only NumPy arrays of strings, I suspect that this comes from PySeries::new_str which extracts Wrap<Utf8Chunked>, right? (rust-numpy does not support the <U* dtypes yet.)

[adamreichold]

Could someone point me to where the entry point for those failing tests is so that I can check which downcast/extraction is failing exactly? Thank you!

So by digging and observing that this seems to affect only NumPy arrays of strings, I suspect that this comes from PySeries::new_str which extracts Wrap<Utf8Chunked>, right? (rust-numpy does not support the <U* dtypes yet.)

I think I found the issue, your various impls like

impl<'a, T> FromPyObject<'a> for Wrap<ChunkedArray<T>>

use a helper function

pub(crate) fn get_pyseq(obj: &PyAny) -> PyResult<(&PySequence, usize)> {
    let seq = <PySequence as PyTryFrom>::try_from(obj)?;
    let len = seq.len()?;
    Ok((seq, len))
}

and this will indeed fail with NumPy arrays as we do not downcast those to PySequence any more after PyO3/pyo3#2477 because they do not really support the sequence API (even though they do pass PySequence_Check).

But I also think that your code does not actually use the sequence interface as both .len() and .iter() work on any PyAny. So dropping get_pyseq together with mechanical changes like

diff --git a/py-polars/src/conversion.rs b/py-polars/src/conversion.rs
index 463b65371..575d69fbd 100644
--- a/py-polars/src/conversion.rs
+++ b/py-polars/src/conversion.rs
@@ -125,10 +125,10 @@ impl<'a> FromPyObject<'a> for Wrap<BooleanChunked> {
 
 impl<'a> FromPyObject<'a> for Wrap<Utf8Chunked> {
     fn extract(obj: &'a PyAny) -> PyResult<Self> {
-        let (seq, len) = get_pyseq(obj)?;
+        let len = obj.len()?;
         let mut builder = Utf8ChunkedBuilder::new("", len, len * 25);
 
-        for res in seq.iter()? {
+        for res in obj.iter()? {
             let item = res?;
             match item.extract::<&str>() {
                 Ok(val) => builder.append_value(val),

should work.

[ritchie46]

Thanks a lot @adamreichold. I believe that was the issue indeed. Will come back with a confirmation. I

[jjerphan]

It looks like default values for all optional parameters must be provided as "required arguments after an Option<_> argument are ambiguous and being phased out".

[ritchie46]

It looks like default values for all optional parameters must be provided as "required arguments after an Option<_> argument are ambiguous and being phased out".

Yes, everything with an Option now needs a attribute defining the arguments:

#[pyo3(signature=(a, b))]
fn do(a: u64, b: Option<u64>) {}  

[jjerphan]

Is there a simple way to infer default values for signatures' arguments?

[ritchie46]

In polars rust side all arguments are always required. So signature should just name all arguments.

[jjerphan]

At the moment, I get this kind of warning:

warning: use of deprecated constant `pyo3::impl_::deprecations::REQUIRED_ARGUMENT_AFTER_OPTION`: required arguments after an `Option<_>` argument are ambiguous and being phased out
         = help: add a `#[pyo3(signature)]` annotation on this function to unambiguously specify the default values for all optional parameters
  --> src/batched_csv.rs:25:9
   |
25 |         chunk_size: usize,
   |         ^^^^^^^^^^
   |
   = note: `#[warn(deprecated)]` on by default

I've tried to only specify the list of arguments' names, but I get other errors.

It seems that default values for required arguments must be provided. For instance, I get the following error:

  error: expected argument from function definition `lambda` but got argument `py`
    --> src/lazy/dsl.rs:1048:25
     |
1048 |     #[pyo3(signature = (py, lambda, window_size, weights, min_periods, center, /))]
     |                         ^^

---

Edit: py: Python arguments must be omitted.

[ritchie46]

Did you add #[pyo3(signature...?

See: https://pyo3.rs/v0.18.0/migration#required-arguments-after-option_-arguments-will-no-longer-be-automatically-inferred

[jjerphan]

Everything should be up to date.

Please, let me know if '#[pyo3(signature = ...)]' macros are properly formatted.

[jjerphan]

Here are some questions regarding some of the proposed changes.

[jjerphan]

Is this change wrong, or is there a naming discrepancy here?

[ritchie46]

Thanks a lot @jjerphan and @adamreichold for the help.

Let me know when it is good to go.

[jjerphan]

Thank you for your guidance, @ritchie46 and @adamreichold.

I think this is mergeable.