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

Runtime type information for objects crossing the Rust–Python boundary #2490

Merged
merged 6 commits into from Sep 6, 2022

Conversation

CLOVIS-AI
Copy link
Contributor

@CLOVIS-AI CLOVIS-AI commented Jul 1, 2022

Thank you for contributing to pyo3!

Please consider adding the following to your pull request:

  • an entry in CHANGELOG.md
  • docs to all new functions and / or detail in the guide Documentation will be added when the whole inspect module will be done
  • tests for all new or changed functions

First step towards #2454.
The goal of this PR is to provide runtime information about Python types for any object that is transferred to/from Python.

Example usage:

println!("a: {}", <(bool, usize, Vec<HashMap<String, u32>>)>::type_input());
a: Tuple[bool, int, Sequence[Mapping[str, int]]]

This PR introduces two new methods:

  • FromPyObject::type_input: the Python type when the object appears as a parameter to a Rust function,
  • IntoPy::type_output: the Python type when the object appears as a return value of a Rust function.

Both methods are implemented for the most important Rust types (primitives like usize, collections like HashMap).
The default implementation is to return Any (which is the recommended type when the exact type is missing, and avoids this PR being a breaking change).

In the future, this will allow the inspection module to generate code calling type_input and type_output to delay the Python type generation until runtime, which is required because macros are executed before the Rust compiler resolves the types.

Future work:

  • Implementing these methods for all other objects (PyDict, hashbrown tables,...); this is fairly easy and may be done on a case-by-case basis when more precise information is needed
  • Automatically generating the precise type information as part of #[pyclass]

@mejrs
Copy link
Member

mejrs commented Jul 2, 2022

You'll need to rebase on main to make CI work (because of #2489 ) :)

@CLOVIS-AI
Copy link
Contributor Author

@mejrs it's rebased :)

@CLOVIS-AI
Copy link
Contributor Author

Fixed cargo fmt and cargo clippy

@davidhewitt
Copy link
Member

👍 looks like you were thinking along the same lines for a first step as me with this PR! Nice work!

So I guess the only question here is whether type_input should be used for downcast errors as suggested in #2454? I think I'd like to confirm that use case works before we introduce new public APIs.

Similarly I wonder if users can already use these to include function signatures in docstrings? Might need some macro support. One to think about, can probably implement this in a separate PR if we want it.

Just for completeness, do other use cases for these beyond the introspection API come to mind? I'm sure users will always surprise us 😊

@davidhewitt
Copy link
Member

davidhewitt commented Jul 7, 2022

Also probably obvious from my other comments, though just wanted to add I'd prefer we merge this after 0.17 is released so that we allow ourselves time to iterate on this further if needed after merge.

@CLOVIS-AI
Copy link
Contributor Author

Just for completeness, do other use cases for these beyond the introspection API come to mind? I'm sure users will always surprise us blush

Well, this PR itself is just the building block of "what is the actual Python type of this type as seen from Python". I expect that this can be used for hundreds of different things 😓. Indeed, debugging/error information seems like a good use case.

So I guess the only question here is whether type_input should be used for downcast errors as suggested in #2454? I think I'd like to confirm that use case works before we introduce new public APIs.

'should' is your decision, but it definitely sounds like it 'could'. I don't think it should be a part of this PR though.

I'd prefer we merge this after 0.17 is released so that we allow ourselves time to iterate on this further if needed after merge.

Do you have an estimate on when that will be? I'm a bit tight on schedule and there's a lot of things to do until .pyi generation is a thing :)

@davidhewitt
Copy link
Member

Tracking preparation for 0.17 at #2493 - TBH at the moment I'm working on getting those pieces complete whenever I can. I'd like it to be at most a week or two, though it depends how much time I can manage to free up to make progress.

@CLOVIS-AI
Copy link
Contributor Author

CLOVIS-AI commented Aug 1, 2022

Fixed rustdoc & conflicts, rebased on current main, better commit messages.

@CLOVIS-AI
Copy link
Contributor Author

I'm not sure I understand why test fails. What should I do to fix it?

@messense
Copy link
Member

messense commented Aug 1, 2022

I'm not sure I understand why test fails. What should I do to fix it?

Run TRYBUILD=overwrite cargo test to update ui tests output, see https://github.com/dtolnay/trybuild#workflow

@davidhewitt
Copy link
Member

@CLOVIS-AI sorry for the long delay with this caused by the 0.17 release. I'm now able to widen bandwidth again, I propose let's merge this and then we can continue iterating on the stub generation? I think this PR is good as-is, just needs a rebase :)

@CLOVIS-AI
Copy link
Contributor Author

Updated the expected test output & rebased.

Copy link
Member

@mejrs mejrs left a comment

Choose a reason for hiding this comment

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

This looks great, I have just a couple of nits :)

Any,
/// The type `typing.None`.
None,
/// The type `typing.NoReturn`, which represents functions that never end (similar to `never` in Rust).
Copy link
Member

Choose a reason for hiding this comment

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

Minor nit - such functions can still panic (in rust) or raise an exception (in python)

Suggested change
/// The type `typing.NoReturn`, which represents functions that never end (similar to `never` in Rust).
/// The type `typing.NoReturn`, which represents functions that never return (similar to `never` in Rust).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, added clarification in parenthesis

/// The module this class comes from.
module: ModuleName,
/// The name of this class, as it appears in a type hint.
name: &'static str,
Copy link
Member

Choose a reason for hiding this comment

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

Could someone want to create names at runtime? In that case Cow<'static, str> is more flexible 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't know it was possible to create a Cow in a const context. That does sound more flexible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really see a use case for creating names at runtime, but I don't really see a reason to forbid it either.

/// The type is in the current module: it doesn't need to be imported in this module, but needs to be imported in others.
CurrentModule,
/// The type is in the specified module.
Module(&'static str),
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@CLOVIS-AI CLOVIS-AI force-pushed the type-info branch 3 times, most recently from 0d34f72 to 28cc154 Compare September 6, 2022 17:16
@davidhewitt
Copy link
Member

Thanks! With apologies will need one final rebase as #2587 has conflicted on the CHANGELOG. (Really need to find some time to work on #2337 soon.)

Also the coverage job is reporting that a lot of new implementations are not covered by tests, it would be much appreciated if we could add a few more. I find it's much better to add coverage at the same time as new APIs wherever possible rather than hope it gets covered later 😄

@CLOVIS-AI
Copy link
Contributor Author

The rebase is done. I do not have the enough time on my hands at the moment to increase the code coverage.

I'm not familiar with GitHub's UI for code coverage so I have some trouble reading it, but it seems to me like it is complaining about all the one-liner functions that simply return a TypeInfo instance? It's not possible for a bug to hide there (they are just Rust structure instanciations).

@davidhewitt
Copy link
Member

Understood. Given that I'm the source of the delay that caused you to lose your availability, seems reasonable to me to proceed to merge in that case.

@davidhewitt davidhewitt merged commit ec58ad2 into PyO3:main Sep 6, 2022
@CLOVIS-AI CLOVIS-AI deleted the type-info branch February 1, 2024 11:00
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.

None yet

4 participants