Update PyO3 #244
Update PyO3 #244
Conversation
There was a problem hiding this comment.
There's a difference between signature and args. args's default (what's after =>) is not the default value used in the python constructor, but just a default value for a specific type to be used in __getnewargs__, which isn't the same thing. We still need both, for example if any argument is not optional, we don't want to add a default in signature but still need it in args. So we need signature to setup py_new args between positional, optional kwarg and mandatory kwarg, see the comments on the PR. We could change the macro syntax to get all the info in one "field", but I'm not sure we'll gain much.
Another place where we can make use of signature and autogenerated text_signature is all the public methods in Dataflow struct (like input, map ecc.). Since all the arguments are positional, we can use signature instead of text_signature everywhere there.
One more thing, given this PR: PyO3/pyo3#2939 we can also add default-features = false to our own dependency on chrono, so we can get rid of the time dependency too, I tried it locally and everything seems to work.
Also, I noticed the signatures for the tracing configs are not quite right (sampling_ratio should default to 1.0, and it should be an f64 rather than an Option<f64>), but that's not related to this PR, so maybe I can make a separate PR with the fix after this is merged.
src/run.rs
Outdated
| #[pyfunction(flow, "*", epoch_interval = "None", recovery_config = "None")] | ||
| #[pyfunction] | ||
| #[pyo3(text_signature = "(flow, *, epoch_interval, recovery_config)")] | ||
| pub(crate) fn run_main( |
There was a problem hiding this comment.
The text_signature here implies that epoch_interval and recovery_config are mandatory kwargs, while they are optional kwargs. Also, since text_signature doesn't generate signature, while the reverse is true, if we want to show the default as "None" rather than "...", we need to use both ( https://pyo3.rs/v0.18.3/function/signature#overriding-the-generated-signature ):
#[pyo3(
signature = (flow, *, epoch_interval = None, recovery_config = None),
text_signature = "(flow, *, epoch_interval = None, recovery_config = None)"
)]
src/run.rs
Outdated
| #[pyo3( | ||
| signature = (flow, addresses, proc_id, *, epoch_interval = None, recovery_config = None, worker_count_per_proc = 1), | ||
| text_signature = "(flow, addresses, proc_id, *, epoch_interval, recovery_config, worker_count_per_proc)" | ||
| )] |
There was a problem hiding this comment.
Same here, text_signature should include the default value:
#[pyo3(
signature = (flow, addresses, proc_id, *, epoch_interval = None, recovery_config = None, worker_count_per_proc = 1),
text_signature = "(flow, addresses, proc_id, *, epoch_interval = None, recovery_config = None, worker_count_per_proc = 1)"
)]
src/run.rs
Outdated
| #[pyfunction] | ||
| #[pyo3( | ||
| signature = ( | ||
| flow, | ||
| *, | ||
| processes = 1, | ||
| workers_per_process = 1, | ||
| process_id = None, | ||
| addresses = None, | ||
| epoch_interval = None, | ||
| recovery_config = None | ||
| ) | ||
| )] |
There was a problem hiding this comment.
Same here, we should include the text_signature:
#[pyo3(
signature=(flow, *, processes=1, workers_per_process=1, process_id=None, addresses=None, epoch_interval=None, recovery_config=None),
text_signature="(flow, *, processes=1, workers_per_process=1, process_id=None, addresses=None, epoch_interval=None, recovery_config=None)"
)]
src/window/sliding_window.rs
Outdated
| @@ -52,7 +52,7 @@ pub(crate) struct SlidingWindow { | |||
| add_pymethods!( | |||
| SlidingWindow, | |||
| parent: WindowConfig, | |||
| py_args: (length, offset, align_to), | |||
| signature: (length=Duration::zero(), offset=Duration::zero(), align_to=DateTime::<Utc>::MIN_UTC), | |||
There was a problem hiding this comment.
This is not right, signature should only have default values if the argument is optional, and this is not the case here, so signature should still be (length, offset, align_to)
src/window/session_window.rs
Outdated
| @@ -35,7 +35,7 @@ impl WindowBuilder for SessionWindow { | |||
| add_pymethods!( | |||
| SessionWindow, | |||
| parent: WindowConfig, | |||
| py_args: (gap), | |||
| signature: (gap=Duration::zero()), | |||
There was a problem hiding this comment.
Since gap is mandatory, we should not add a default value in the signature here:
signature: (gap)
src/window/clock/event_time_clock.rs
Outdated
| @@ -68,7 +68,7 @@ impl ClockBuilder<TdPyAny> for EventClockConfig { | |||
| add_pymethods!( | |||
| EventClockConfig, | |||
| parent: ClockConfig, | |||
| py_args: (dt_getter, wait_for_system_duration), | |||
| signature: (dt_getter, wait_for_system_duration=Duration::zero()), | |||
There was a problem hiding this comment.
wait_for_system_duration is not optional in our current api, so we shouldn't add a default value here:
signature: (dt_getter, wait_for_system_duration)
- Fixes issues with using `signature` incorrectly. - Use `default-features = false` with chrono to avoid time dependency. - Fixes signatures for the tracing configs. - Moves `python-source` from Cargo.lock to pyproject.toml for Maturin compatibility
- Don't install dill
Updates PyO3 to 0.18.3
There may be a better way to unpack
argswithinadd_pymethods!that removes the need to write out bothargsandsignature, but my macro skills are sadly not up to the task.