GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Use real references to `Graphics`, proper garbage collection in ipython and jupyter by kushalkolar · Pull Request #546 · fastplotlib/fastplotlib

Conversation

@kushalkolar

@kushalkolar kushalkolar commented Jul 1, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

closes #472, closes #161

also closes #393

No more weakreferences to Graphics! 🥳

Figured out how to properly garbage collect references proliferated by ipython in jupyter with some help from @lauraporta, thanks! 😄

The pygfx.WorldObject reference in Graphic is still a weakref.proxy, I thought let's keep it like that for now since we don't expect users to directly interact with the rendering engine objects much.

In PlotArea.delete_graphic() this is how proliferated references can be removed:

fastplotlib/fastplotlib/layouts/_plot_area.py

Lines 642 to 651 in 675dfaa

if IS_IPYTHON:
# remove any references that ipython might have made
ip = get_ipython()
# get all references in ipython namespace
references = list(ip.user_ns.keys())
for ref in references:
if ip.user_ns[ref] is graphic:
# we found the graphic, remove from ipython
ip.del_var(ref)
break

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lauraporta reacted with hooray emoji
All reactions
  • 🎉 1 reaction
kushalkolar added 7 commits June 17, 2024 20:48
@kushalkolar
ndarray.ptp -> np.ptp for numpy v2
57702ac
@kushalkolar
Merge branch 'main' of https://github.com/fastplotlib/fastplotlib
eecb5a3
@kushalkolar
Merge branch 'main' of https://github.com/fastplotlib/fastplotlib
ddf13bd
@kushalkolar
Merge branch 'main' of https://github.com/fastplotlib/fastplotlib
d1c9c31
@kushalkolar
Merge branch 'main' of https://github.com/fastplotlib/fastplotlib
ebebc91
@kushalkolar
Merge branch 'main' of https://github.com/fastplotlib/fastplotlib
89bf7cd
@kushalkolar
use real refs to graphics
675dfaa
@kushalkolar
kushalkolar requested a review from almarklein July 1, 2024 20:56
@kushalkolar
kushalkolar requested a review from clewis7 as a code owner July 1, 2024 20:56
almarklein
almarklein reviewed Jul 2, 2024
View reviewed changes

@almarklein almarklein left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

No more weakreferences to Graphics! 🥳

🥳 ❤️

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread fastplotlib/graphics/_base.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread fastplotlib/graphics/_collection_base.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread fastplotlib/layouts/_plot_area.py Outdated
Comment on lines +647 to +651
for ref in references:
if ip.user_ns[ref] is graphic:
# we found the graphic, remove from ipython
ip.del_var(ref)
break

@almarklein almarklein Jul 2, 2024

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

How likely is it that the object is in user_ns? If it's only occasionally, can wrap this in:

if graphic in ip.user_ns.values():
    ...

I guess it's possible that the graphic is present more than once, under a different name?

Alt solution that might be faster because it avoids copying the keys and looking up the values:

            keys_to_delete = []
            for key, ob in ip.user_ns.items():
                if ob is graphic:
                    keys_to_delete.append(key)
            for key in keys_to_delete:
                    ip.del_var(key)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@kushalkolar kushalkolar Jul 8, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

How likely is it that the object is in user_ns? If it's only occasionally, can wrap this in:

if graphic in ip.user_ns.values():
    ...

in operators don't work if any item of the iterable is a numpy array, for example:

a = np.random.rand(10)
l = [a, "bah", 123]

# will raise a ValueError
123 in l

I've resorted to this, couldn't think of a more performance solution. Time taken scales with the number of items in the namespace.

if IS_IPYTHON:
    # remove any references that ipython might have made
    ip = get_ipython()

    # check both namespaces
    for namespace in [ip.user_ns, ip.user_ns_hidden]:
        # find the reference
        for ref, obj in namespace.items():
            if graphic is obj:
                # we found the reference, remove from ipython
                ip.del_var(ref)
                break

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@almarklein almarklein Jul 8, 2024

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

No problems with deleting items from an object being iterated over? I guess it depends on IPython internals whether that'd be a problem.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
clewis7
clewis7 previously approved these changes Jul 2, 2024
View reviewed changes

@clewis7 clewis7 left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM aside from the minor suggestions that Almar had

I think this will be super useful for users to get the actual object instead of a weakref back (less confusing when interacting with objects etc.). I agree that for now we just leave weakref to the world object...we can always change later.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@kushalkolar
code review, cleaner
8ee03ec
@kushalkolar
kushalkolar dismissed clewis7’s stale review via 8ee03ec July 8, 2024 01:29
@kushalkolar

kushalkolar commented Jul 8, 2024

Copy link
Copy Markdown
Member Author

ready for another look, sorry for the delay been sick 🤧

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

almarklein
almarklein reviewed Jul 8, 2024
View reviewed changes
Comment thread fastplotlib/graphics/_base.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@almarklein

almarklein commented Jul 8, 2024

Copy link
Copy Markdown
Collaborator

Hope you get better soon!

clewis7 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

clewis7
clewis7 reviewed Jul 8, 2024
View reviewed changes
Comment thread fastplotlib/graphics/_collection_base.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@kushalkolar
small fixes
05a5d0c
@kushalkolar

kushalkolar commented Jul 10, 2024

Copy link
Copy Markdown
Member Author

@clewis7 ready to go!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

clewis7
clewis7 approved these changes Jul 10, 2024
View reviewed changes
@clewis7
clewis7 merged commit 9ff92a7 into main Jul 10, 2024
@kushalkolar
kushalkolar deleted the ipython-gc branch July 12, 2024 03:17
@kushalkolar kushalkolar mentioned this pull request Sep 29, 2024
update contributing guide #580
Closed
5 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@almarklein almarklein almarklein left review comments
@clewis7 clewis7 clewis7 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

use of weakref.proxy GraphicCollection.__del__ method can cause confusing tracebacks Create a Proxy subclass that we use for our Graphic classes

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@kushalkolar @almarklein @clewis7
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.