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

Using pyo3's signature information to generate python's defualt value #135

Merged
merged 8 commits into from
Jan 16, 2025

Conversation

zao111222333
Copy link
Contributor

@zao111222333 zao111222333 commented Jan 14, 2025

Hi everyone,
I want to improve the function signature when pyo3's signature is specified.
Let me use an example to show what did I do, for the following rust function:

/// `result=(a * b) + c`,
/// `b` and `c` has defualt values
#[gen_stub_pyfunction]
#[pyfunction]
#[pyo3(signature = (a, b = vec![1,2], c = 2 ))]
fn mul_add_vec(a: usize, b: Vec<usize>, c: usize) -> Vec<usize> {
    b.into_iter().map(|b| a * b + c).collect()
}

Original output

def mul_add_vec(a: ..., b: ... = ..., c: ... = ...) -> list[int]:
    ...

Now

def mul_add_vec(a:int, b:typing.Sequence[int]=[1, 2], c:int=2) -> list[int]:
    ...

My approach:

For instance, how to infer python's [1, 2] from rust's vec![1,2]?
To summary, I use pyo3::IntoPyObject::into_pyobject to get the python object converted from vec![1,2], then call to_string to get the python object's value.

More deltails:

  • The convertion is now static, so I use std::sync::LazyLock, see here
  • In pyo3-stub-gen, I merge the signature information into each ArgInfo, see here

Copy link
Collaborator

@konn konn left a comment

Choose a reason for hiding this comment

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

Hi, thank you for your contribution! We are surprised that we are just planning to work on coexistence of signature and type hints in function arguments, and, alas, your PR include this feature! What a great synchronicity. 😄

We are willing to merge your PR, provided that corner cases in to_string()-based approach are fixed; please see the review below.

pyo3-stub-gen-derive/src/gen_stub/member.rs Outdated Show resolved Hide resolved
pyo3-stub-gen-derive/src/gen_stub/pyclass.rs Outdated Show resolved Hide resolved
pyo3-stub-gen/src/generate/member.rs Outdated Show resolved Hide resolved
pyo3-stub-gen-derive/src/gen_stub/signature.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@konn konn left a comment

Choose a reason for hiding this comment

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

It seems that some CI are failing. Please apply rustfmt and clippy accordingly.

Also, if you don't have enough time, I can do the rest of the work, either by creating another PR onto your branch, or directly edit your branch. Please feel free to call for help. 👍

@zao111222333
Copy link
Contributor Author

Hi @konn, thank you so much for your review & reply!
Yes, I will try to use eval or extract to fix the corner cases.
For getter's change, I update some lines to propagate getter's doc, it's not releative to this PR, I will remove them and create a new PR for that feature.

@konn
Copy link
Collaborator

konn commented Jan 15, 2025

Hi @konn, thank you so much for your review & reply! Yes, I will try to use eval or extract to fix the corner cases.

Thanks! As pointed out above, please avoid using eval as it has some security concern.

For getter's change, I update some lines to propagate getter's doc, it's not releative to this PR, I will remove them and create a new PR for that feature.

👍

@zao111222333
Copy link
Contributor Author

Hi @konn , I replace to_string with extract, it's a little messy but verified works in my test cases.
Waitting for your feedback :)

@zao111222333
Copy link
Contributor Author

Sorry for forgetting the CI check for util.rs, I fixed them.
However, I'm not so sure how to fix the errors from cargo semver-checks.

Copy link
Collaborator

@konn konn left a comment

Choose a reason for hiding this comment

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

Thank you for your swift reaction and detailed tests! I think we are almost done, but please check some additional reviews.

pyo3-stub-gen/src/util.rs Outdated Show resolved Hide resolved
pyo3-stub-gen/src/util.rs Outdated Show resolved Hide resolved
pyo3-stub-gen/src/util.rs Outdated Show resolved Hide resolved
pyo3-stub-gen/src/util.rs Outdated Show resolved Hide resolved
pyo3-stub-gen/src/util.rs Outdated Show resolved Hide resolved
@konn
Copy link
Collaborator

konn commented Jan 15, 2025

Sorry for forgetting the CI check for util.rs, I fixed them. However, I'm not so sure how to fix the errors from cargo semver-checks.

Thanks! I've just edited your branch so that semver-checks CI would success. Please pull before working on my change.

@zao111222333
Copy link
Contributor Author

Also, I think formatting dict, list, and tuples manually can contain some corner-cases. Rather, I recommend to check if the values are consisting only of python values, and then use to_string() once the invariance is checked. See below for example.

That's a great idea and we can have a more elegant formatter function. I finished that.

Copy link
Collaborator

@konn konn left a comment

Choose a reason for hiding this comment

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

Thank you for your change! I am not aware that to_string() cannot be used for string literals. It seems calling repr() is more suitable for this kind of conversion and does string escape properly. See suggestions below.

pyo3-stub-gen/src/util.rs Outdated Show resolved Hide resolved
pyo3-stub-gen/src/util.rs Outdated Show resolved Hide resolved
pyo3-stub-gen/src/util.rs Show resolved Hide resolved
@zao111222333
Copy link
Contributor Author

I do a wrong push and I recall that, ignore that :)

@zao111222333
Copy link
Contributor Author

The suggestions are adopted, waitting for your feedback.
And if you are willing, I want to create a PR to do the following things:

  • Add indexmap feature to support IndexMap and IndexSet
  • Propagate doc to getter's class member
  • User specified default value for getter's class member (need stub_gen macro attribute), e.g.,
#[pyclass]
#[stub_gen_pyclass]
struct A {
    #[pyo3(get,set)]
    #[stub_gen(default = 2)]
    x: usize,
}
impl Default for A {
    fn default() -> A {
        Self { x: 2 }
    }
}

Output:

class A:
    x: int = 2

Copy link
Collaborator

@konn konn left a comment

Choose a reason for hiding this comment

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

LGTM 👍, thank you for your contributions!

@konn konn merged commit 5465d26 into Jij-Inc:main Jan 16, 2025
12 checks passed
@konn
Copy link
Collaborator

konn commented Jan 16, 2025

And thank you for your future contributions.

  • Add indexmap feature to support IndexMap and IndexSet

I think you are fine to work on this PR, but there is no particular need for us for time being, it can take some time to be get merged.

  • Propagate doc to getter's class member

It seems duplicate of #96 - if it is different, please let us know.

  • User specified default value for getter's class member (need stub_gen macro attribute), e.g.,

This seems good to have, but we have to carefully discuss the design of new type of attribute macro.

Anyway, you can feel free to open PRs if it is not a duplicate of an existing ones. Thank you again for your contribution!

@zao111222333
Copy link
Contributor Author

zao111222333 commented Jan 16, 2025

Thanks so much for your assistance! I will open PRs once I finish those ideas.

konn added a commit that referenced this pull request Jan 17, 2025
#135 introduces optional argument rendering for primitive types. This
accidentally rejects default argument with non-`PyAny`
`IntoPyObject::Target`. This PR fixes it.
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.

2 participants