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

Add PyFrozenSetBuilder #3156

Merged
merged 1 commit into from
Jun 14, 2023
Merged

Add PyFrozenSetBuilder #3156

merged 1 commit into from
Jun 14, 2023

Conversation

adriangb
Copy link
Contributor

These are actually available via the C ABI. Since the distinction between set and frozenset means nothing to Rust safety, we should make these available

@adriangb adriangb force-pushed the frozenset-ops branch 2 times, most recently from 9976903 to 0492768 Compare May 16, 2023 17:34
@adriangb adriangb changed the title Add clear(), add() and discard() for frozenset Add clear(), add(), pop() and discard() for frozenset May 16, 2023
@adamreichold
Copy link
Member

The test failures seem to suggest that the methods do not actually modify a frozen set though? If that is the case, I would prefer not to add them as mutators which do not fail but do not mutate would be confusing at best.

@adriangb
Copy link
Contributor Author

Hmm no they should. The docs make it clear they should work just like with a set. I also implemented add in pydantic and it does work. Let's take a closer look at what's happening in the test.

@davidhewitt
Copy link
Member

From https://docs.python.org/3/c-api/set.html it looks like only PySet_Add works and it's really only intended to use it for filling in frozen sets during creation (similar to tuples).

Personally I think modifying frozensets is dangerous so would prefer to make this hard to use accidentally, whether by unsafe or just not exposing it.

@adriangb
Copy link
Contributor Author

add is indeed the only one I had tried before this PR. If that's the case the docs are very confusing:

The following functions and macros are available for instances of set or frozenset or instances of their subtypes.

Is right before the list of these methods (including clear, etc.).

Personally I think modifying frozensets is dangerous so would prefer to make this hard to use accidentally, whether by unsafe or just not exposing it.

I found it very useful in Pydantic 😅, I don't think it's that dangerous but I'd be okay with going with unsafe for now at least.

@adriangb adriangb changed the title Add clear(), add(), pop() and discard() for frozenset Add PyFrozenSet.add() May 16, 2023
@adriangb
Copy link
Contributor Author

Okay, I left only .add() and marked it as unsafe.

similar to tuples

Interesting! I'd be +1 on doing the same thing for the tuple methods. I think marking them as unsafe is a good balance, it saves users from code duplication and improves discoverability while making sure implementers are cautious when using these methods.

@adamreichold
Copy link
Member

adamreichold commented May 18, 2023

I pondered this for a bit and while I would like us to add add methods for frozen sets and tuples, but I am not happy about marking them as unsafe. unsafe functions are meant to indicate that additional preconditions need to be fulfilled to avoid undefined behaviour which does not seem to be case here. (With the additional learning that unsafe code cannot rely on frozen sets and tuples really being immutable when interacting with other code in the Python ecosystem.)

As an alternative I would like to propose adding PyFrozenSetBuilder and PyTupleBuilder types which exists only to build up PyFrozenSet and PyTuple instances using their (safe) add methods. At the end the builders can be converted into their respective targets without cost. Theoretically, we could also implemented the inverse direction by checking the reference count but it seems really hard to achieve a reference count of one in Python code in my experience.

@adriangb Would that suffice for the use cases you have in mind?

@adriangb
Copy link
Contributor Author

That seems like a really good approach to me.

@davidhewitt
Copy link
Member

davidhewitt commented May 18, 2023

I'm open to adding builders if we feel they're necessary, but first I wonder if we just need to improve the functionality of PyFrozenSet::new and PyTuple::new? I'd hope they'd be pretty flexible with the Iterator inputs.

@adriangb I wonder why they're not meeting your needs?

@davidhewitt
Copy link
Member

(Note that I'm not saying the ::new methods are currently great. Currently PyFrozenSet::new takes impl IntoIterator<Item = &'a T>. PyList::new and PyTuple::new take impl IntoIterator<Item = T>, which I think is better. All of them use the ToPyObject trait, IntoPy<PyObject> might be better, but that's less clear.)

@adriangb
Copy link
Contributor Author

I tried refactoring everything into an Iterator but it was unwieldy because:

  1. It required changing a lot of existing code, although this shouldn't be a consideration for PyO3.
  2. If there's a lot of state the iterator needs to hold it becomes super boilerplatey. If Rust had Python-like generator functions then it'd be less cumbersome. Since it doesn't you have to create a struct w/ generics, etc. just to pass that into a set can easily be dozens of lines of boilerplate if you already had everything you needed in a function.
  3. It's not any faster. If the constructors did something smart with the iterator it'd be one thing, but they're just looping over it and setting individual items. So creating the iterator is going to be necessarily slower (I even had an issue where it was 10%+ slower, still not sure why but it had something to do with having like 6 generic parameters one of which was a struct with dozens of methods).

@adamreichold
Copy link
Member

If Rust had Python-like generator functions then it'd be less cumbersome.

The recent posts by withoutboats came to mind reading this and I agree that writing single use iterators is really cumbersome in today's Rust. If two small builder types let us work around that, I think that would be preferable as a short term solution. We could drop those again when generators have become a thing in Rust.

@davidhewitt
Copy link
Member

That's very fair feedback, sorry if you invested a lot of time into trying the iterator approach @adriangb . I'm on board with us building the builders :)

@adriangb
Copy link
Contributor Author

adriangb commented May 19, 2023

To be clear I’m not complaining, it’s not PyO3’s fault or anything, but thank you 😊

building the builders

laughing at this, good one if it was intentional

@adamreichold
Copy link
Member

Since we agreed to pursue the builder approach, @adriangb do you have the bandwidth to just reboot this PR into adding the two builder types exposing add methods?

@adriangb
Copy link
Contributor Author

adriangb commented May 20, 2023

I don’t have the bandwidth this weekend, but I think I can get to it late next week, and would like to do it you are willing to give me feedback. Just clarifying since you’re trying to make a release soon, I definitely would not plan on that/this being included.

@adriangb
Copy link
Contributor Author

I added a builder for PyFrozenSet. I don't think we need one for non-frozen sets since it's always "safe" to add items, right?

@adriangb adriangb changed the title Add PyFrozenSet.add() Add PyFrozenSetBuilder Jun 13, 2023
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

👍 LGTM!

@adamreichold adamreichold added this pull request to the merge queue Jun 14, 2023
Merged via the queue into PyO3:main with commit ba0c7e1 Jun 14, 2023
@adriangb adriangb deleted the frozenset-ops branch July 3, 2023 16:14
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