Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chore/extendr 0.7.0 support #1187

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ju6ge
Copy link
Contributor

@ju6ge ju6ge commented Aug 17, 2024

This MR builds upon #1154 and updates the test suite and rust code to work as expected.

@ju6ge ju6ge mentioned this pull request Aug 17, 2024
1 task
@ju6ge ju6ge force-pushed the chore/extendr-0.7.0-support branch from ad9358b to 1f2abd2 Compare August 17, 2024 19:54
Copy link
Collaborator

@etiennebacher etiennebacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ju6ge Many thanks for your efforts here and in #1154, they are really appreciated.

My main issue with this PR is that it can degrade some error messages, can you take a look at that?

Also, as you say in #1154 (comment):

Though I am still unsure that the changes are sufficient. There might me be more parts of the code that use the previous behavior and expect to return the error to R. So there might be more cases where users can run into segfaults that are not tested for already!

At the very least I would like a better way to check if the converesion will work before attempting it. Checking against a string is very unergonomic!

so I'm not too confident this should be merged as-is. @CGMossa I know you already answered to some of that in #1154, but it would help if you could just skim through the changes in Rust code.

I'd also like @eitsupi to check that

@@ -20,7 +20,6 @@ use polars::prelude as pl;
use polars::prelude::{ExprEvalExtension, NestedType, SortOptions};
use std::any::Any;
use std::ops::{Add, Div, Mul, Rem, Sub};
use std::result::Result;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change? Isn't it cleaner to keep this rather than putting std::result:: everywhere?

Comment on lines +746 to +748
if rv != "ExternalPtr.set_class([\"RPolarsDataType\"]" {
return Err(RPolarsErr::new().bad_val(rv).mistyped(tn::<RPolarsDataType>().to_string()).into());
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you said in #1154 (comment), checking on this string looks very fragile

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see now.. I think this can be addressed in extendr/extendr#853. I just added a test that shows that we can deal with this situation, with very accurate type-checking.

pl$DataFrame(x = 1, schema = list(x = "foo")),
"expected RPolarsDataType"
)
expect_grepl_error(pl$DataFrame(x = 1, schema = list(x = "foo")))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change (and others below) make the test pass but they degrade the error messages. For example, instead of
expected RPolarsDataType we now have Expected ExternalPtr got Strings which is quite cryptic. Is there a way to not modify those error messages?

expect_grepl_error(pl$n_unique(pl$all()))
expect_grepl_error(pl$n_unique(1))
expect_error(pl$n_unique(pl$all()))
expect_error(pl$n_unique(1))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect_grepl_error() is different from expect_error() because it can catch polars errors that have a weird structure in R.

If we don't check the pattern of the message, then expect_error() can be used indeed, but can you rather keep expect_grepl_error() and add some pattern matching?

@ju6ge
Copy link
Contributor Author

ju6ge commented Aug 18, 2024

Sure thing, I will gladly update this MR to address the concerns about the tests. Though I will have to see if I find time for this next week.

I am coming more from the rust side of things, and the R side of things is still a bit more cryptic to me. Can you give me some pointers on how expect_grepl_error expects an certain error type. Just setting the expected error as a string seems quite strange to me.

Copy link
Collaborator

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ju6ge Thanks for working on this!
It would be great to see the R CMD check error resolved.

I am wondering if you are overlooking #1152.
I mean, we may abandon our entire current codebase in the near future, and since we will be switching from extendr to savvy in the future, I don't see much value in spending time on an extendr-only hack.

If you are willing to work on developing r-polars, it would be great if you could check out the next branch.

@etiennebacher
Copy link
Collaborator

etiennebacher commented Aug 18, 2024

Can you give me some pointers on how expect_grepl_error expects an certain error type. Just setting the expected error as a string seems quite strange to me.

Normally, using expect_error(<code>, <error message pattern>) would work, but the error handling in r-polars is a bit convoluted. It relies on some internal unwrap() that sort of mimics the Rust error handling but doesn't play super well with testthat::expect_error(), see e.g. #987 (comment).

Therefore, we made a custom expect_grepl_error() that should be a drop-in replacement for expect_error() but works fine with our own error handling. You don't have to put the full error message in the second argument, just a piece of it to ensure that the expected error message actually matches what we expect. There are plenty of examples in the tests folder.

@ju6ge
Copy link
Contributor Author

ju6ge commented Aug 19, 2024

@eitsupi I have added my thoughts to #1152

@etiennebacher thanks for the explanation, i will take a look at it and see what i come up with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants