feat: Make register_plugin a standalone function and include shared lib discovery

[MarcoGorelli]

Starting by giving plugin authors a public version of _get_shared_lib_location

I'd also really like to have a public version of parse_as_expression + wrap_expr, but will leave that for a separate discussion a public parse_as_expression would no longer be needed with these changes

[stinodego]

If we're going to make this part of the public API, we have to make sure the typing is done correctly and it has a proper docstring (e.g. document the parameters / point to the user guide with extensive plugin docs). Probably should also properly use pathlib.

We can also take this opportunity to reconsider the function/parameter names.

We should also document it as unstable.

[codecov]

Codecov Report

Attention: Patch coverage is 93.24324% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 81.06%. Comparing base (fd9eba2) to head (263225c).
Report is 15 commits behind head on main.

❗ Current head 263225c differs from pull request most recent head 0f5183e. Consider uploading reports for the commit 0f5183e to get more accurate results

Files	Patch %	Lines
py-polars/polars/expr/expr.py	0.00%	4 Missing ⚠️
py-polars/src/functions/misc.rs	97.14%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14804      +/-   ##
==========================================
+ Coverage   81.02%   81.06%   +0.04%     
==========================================
  Files        1338     1342       +4     
  Lines      173706   173865     +159     
  Branches     2461     2459       -2     
==========================================
+ Hits       140742   140945     +203     
+ Misses      32494    32454      -40     
+ Partials      470      466       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[stinodego]

Could you add a simple test for this function? Should be easy enough to set up a sensible test by using tmp_path fixture.

[stinodego]

(I) have added a step in the Python3.11 Ubuntu job to test a little sample plugin in CI. It adds ~1m 6s to that job's CI time but I think it's well worth it, especially if clients are using plugins in production

This is probably a good idea, but I don't really want a sample_plugin folder in the repo root. Could you split the testing/CI change off to a separate PR? Then we can merge this API improvement now, and think a bit about the best way to test plugins separately.

[MarcoGorelli]

Sounds good, thanks!

[stinodego]

I did some cleanup and made some minor changes.

The function is now called register_plugin_function, as it registers a single function (not all functions in the plugin).

We have to decide whether we want to throw a deprecation warning for the previous Expr.register_plugin (and ultimately remove it?), due to backward compatibility. It was marked unstable and thanks to Marco's experience we have found that the existing API is lacking.

@ritchie46 what do you think?

[ritchie46]

Yeap, sounds good to me. 👍

[stinodego]

Okay, I have done a few more things:

• The old plugin functions are marked as deprecated through a warning in the docstring, but do not throw a deprecation warning, since this warning is intended for plugin creators but would be shown to plugin users too.
• Un-mark the new function as unstable. Again, an unstable warning would end up with end users, which we do not want. We also do not plan to mess around with this API, so it doesn't need to be unstable.
• Redirect the deprecated functionality to the new functionality, to avoid code duplication.
• Some small updates to the API names.
• Include another test.

@MarcoGorelli could you give this another look and let me know if you're on board with everything as it currently stands? Then we can work out any last potential issues and get it merged.

I'd say this is a really nice ergonomics improvement for plugin authors!

[stinodego]

I will rename cast_to_supertypes internally to cast_to_supertype. The inputs all get cast to their (singular) supertype, so the previous name didn't make sense. I already corrected it in the new plugin function public API, so we don't have to change that again later.

[MarcoGorelli]

Thanks!

do not throw a deprecation warning, since this warning is intended for plugin creators but would be shown to plugin users too

Are you sure about this?:

• Polars _get_shared_lib_location emits DeprecationWarning
• plugin uses _get_shared_lib_location
• user users plugin

So the user wouldn't see the DeprecationWarning https://docs.python.org/3/library/exceptions.html#DeprecationWarning "Ignored by the default warning filters, except in the __main__ module". There's a nice example of this here: #15081

[stinodego]

Aha, in that case I'm adding back the warning 👍

[MarcoGorelli]

Looks good to me, thanks

I just checked this out with polars-xdt, and:

• current tests pass, but with deprecation warning
• running polars_xdt directly as a user doesn't show any deprecation warning
• updating to use the new public function results and tests all pass

[stinodego]

Thanks for checking! All right, let's press the green button then 👍