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 weakref Python types #3835

Merged
merged 48 commits into from
May 31, 2024
Merged

Add weakref Python types #3835

merged 48 commits into from
May 31, 2024

Conversation

SuperJappie08
Copy link
Contributor

#3134 Add PyWeakRef, PyWeakProxy and PyWeakCallableProxy types, corresponding to Python's weakref.ReferenceType, weakref.ProxyType and weakref.CallableProxyType.

@SuperJappie08
Copy link
Contributor Author

There are some issues with the new run_bound.
I will resolve these issues now.

Copy link

codspeed-hq bot commented Feb 14, 2024

CodSpeed Performance Report

Merging #3835 will not alter performance

Comparing SuperJappie08:weakref (7e9aad8) with main (388d176)

Summary

✅ 67 untouched benchmarks

@davidhewitt
Copy link
Member

Thank you and sorry for the slow review, I will try my best to finally look at this tomorrow...

@SuperJappie08
Copy link
Contributor Author

Thank you and sorry for the slow review, I will try my best to finally look at this tomorrow...

No problem, completely understandable.

I also ran into one last issue, PyPy does not include all the required bindings for the weakref module in their C-api.
At least the bindings required in the way it is done now.

The type-objects for the reference types are not exported (not bound to in pyo3-ffi and I can't find them in the PyPy header files). Also, the memory layout is missing, since in PyPy weakref is not actually a binary module.
That last one should at least be solvable, but I cannot think of a solution for the missing type objects

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.

Thank you very much for this!

It looked like a big diff but that's because you've included all docs and tests, thank you 🙏

I've given this a full read; I have a few initial things which need discussing and changing. I think once those are decided upon & done I might need to take another final read through just due to the size of the diff.

.gitignore Outdated Show resolved Hide resolved
src/types/weakref/reference.rs Outdated Show resolved Hide resolved
src/types/weakref/reference.rs Outdated Show resolved Hide resolved
src/types/weakref/reference.rs Outdated Show resolved Hide resolved
src/types/weakref/reference.rs Outdated Show resolved Hide resolved
src/types/weakref/reference.rs Outdated Show resolved Hide resolved
src/types/weakref/reference.rs Outdated Show resolved Hide resolved
src/types/weakref/reference.rs Outdated Show resolved Hide resolved
src/types/weakref/proxy.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member

Regarding PyPy, for now I suggest just not offering these types on PyPy.

@SuperJappie08
Copy link
Contributor Author

SuperJappie08 commented May 26, 2024

I already see a documentation build failure in the CI.

I use a full path to PyNone (crate::types::none::PyNone), which it doesn't like.
Could this be because crate::types::none is private?

(Than the fix would be a small commit, changing crate::types::none::PyNone to crate::types::PyNone)

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 26, 2024
@SuperJappie08
Copy link
Contributor Author

It should be fixed now.

I have looked into the Netlify error, it happens on pyo3-ffi which is not changed by this PR, so that error will indeed be resolved by merging.

@Icxolu Icxolu added this pull request to the merge queue May 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 26, 2024
@SuperJappie08
Copy link
Contributor Author

Some tests are failing, I will change them to work with the new changes

@SuperJappie08
Copy link
Contributor Author

It should now work (I have now also tested on Python 3.13)

@davidhewitt davidhewitt added this pull request to the merge queue May 28, 2024
@davidhewitt
Copy link
Member

Let's have another go 👍

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 28, 2024
@davidhewitt davidhewitt added this pull request to the merge queue May 28, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 29, 2024
@davidhewitt davidhewitt added this pull request to the merge queue May 29, 2024
@davidhewitt
Copy link
Member

I'm going to retry again - it seems to me like it's just cancelling jobs for no clear reason, which is odd.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 29, 2024
@davidhewitt davidhewitt added this pull request to the merge queue May 29, 2024
@davidhewitt
Copy link
Member

Ok, this time it failed with a hypothesis flake which I've not seen before. I've never experienced the CI so unreliable 😢

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 29, 2024
@SuperJappie08
Copy link
Contributor Author

It's always graalpy-macos that fails (I think).

I find it suspicious...

@davidhewitt davidhewitt added this pull request to the merge queue May 31, 2024
@davidhewitt
Copy link
Member

davidhewitt commented May 31, 2024

Let's retry; other stuff is merging ok since. If this still struggles to merge then I think more investigation here of what on this branch is causing the issue.

Merged via the queue into PyO3:main with commit 25c1db4 May 31, 2024
42 of 44 checks passed
@Icxolu Icxolu mentioned this pull request Jun 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants