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

feat: migrate to pyo3 #3520

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

feat: migrate to pyo3 #3520

wants to merge 5 commits into from

Conversation

anonrig
Copy link
Contributor

@anonrig anonrig commented May 2, 2024

Written in collaboration with @loewenheim

Goal

The goal of this pull-request is to remove relay_cabi, relay_ffi and relay_ffi_macros and replace it with a more modern approach: pyo3. The proposed changes reduce the Python surface area of the implementation and implements most of the functionality in Rust, making maintenance a lot easier to the current approach.

Limitations

  • pyo3 does not support struct with generics.

Missing functionality:

  • validate_register_response does not return uuid as Uuid type, but returns as a string. This is due to the limitation of pyo3.
  • SecretKey does not have a function called pack. This is due to the limitation of pyo3, since it requires json.dumps to be called on an unknown JSON object. The lack of object signature knowledge results in the requirement of calling json.dumps on Python side.
  • relay_store_normalizer_normalize_event requires an object of Annotated<Event> to be deserialized and serialized. Unfortunately, pyo3 doesn't support structs with generics. Therefore, deserializer/serializer can not be used, and normalize_event can not be implemented in a relatively easy way. This applies to several other functions such as pii_strip_event.

Todo

  • remove relay_cabi
  • remove relay_ffi
  • remove relay_ffi_macros
  • remove py/sentry_relay folder
  • update GitHub workflow
  • update py/Dockerfile
  • update makefile
  • add types for sentry_relay.validate_pii_selector usage

@anonrig anonrig marked this pull request as ready for review May 13, 2024 22:16
@anonrig anonrig requested a review from a team as a code owner May 13, 2024 22:16
@anonrig anonrig changed the title wip pyo3 migration feat: migrate to pyo3 May 14, 2024
@anonrig anonrig force-pushed the relay-py-pyo3 branch 8 times, most recently from 0d6946a to ec727a7 Compare May 14, 2024 23:33
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

@anonrig Thank you for starting this, moving to PyO3 has been on our wishlist for a while! Couple of questions:

  1. Whats the reason for moving the crate to py/rust? I would prefer a top level workspace crate relay-pyo3 or similar (essentially renaming relay-cabi).
  2. Should we split this into multiple steps and as a first step introduce Py03 for the bindings without introducing the dependency in many of our crates? E.g. normalize_global_config could be a #[pyfunction] which still calls serde internally. Ideally the function signatures on the python side (including error handling) do not change. This would reduce the risk of rolling this out.

@anonrig
Copy link
Contributor Author

anonrig commented May 15, 2024

Hey @jjbayer, thank you for the quick Look!

Whats the reason for moving the crate to py/rust? I would prefer a top level workspace crate relay-pyo3 or similar (essentially renaming relay-cabi).

There was no specific reason, merely choice. @loewenheim fixed it and moved it into the correct place in the repo.

Should we split this into multiple steps and as a first step introduce Py03 for the bindings without introducing the dependency in many of our crates? E.g. normalize_global_config could be a #[pyfunction] which still calls serde internally. Ideally the function signatures on the python side (including error handling) do not change. This would reduce the risk of rolling this out.

Ideally yes, for now, we're trying to focus on correctness, and meanwhile I'll prepare a document to discuss our options to merge and release this.

@anonrig anonrig force-pushed the relay-py-pyo3 branch 11 times, most recently from 1b96b9d to 267a20f Compare May 16, 2024 19:29
@loewenheim
Copy link
Contributor

StoreNormalizer.normalize_event continues to be a huge problem. It has two parameters, event and raw_event. event is an event dictionary object, raw_event is the serialization thereof. event is only used of raw_event is not used, so it's immediately unclear why they both need to exist—just use one parameter and serialize it if necessary.

Unfortunately it's currently impossible to convert between PyDict and Event, so the only way to pass event dictionaries across the Python/Rust boundary is string serialization.

@jjbayer
Copy link
Member

jjbayer commented Jun 24, 2024

@loewenheim any next steps planned for this?

@loewenheim
Copy link
Contributor

@jjbayer There's still some work to be done; I'm a bit busy with other things but I do want to get this over the finish line.

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.

3 participants