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

Collapsing identical reprs broken by caching #5

Closed
aazuspan opened this issue Nov 6, 2022 · 2 comments
Closed

Collapsing identical reprs broken by caching #5

aazuspan opened this issue Nov 6, 2022 · 2 comments
Labels
bug Something isn't working

Comments

@aazuspan
Copy link
Owner

aazuspan commented Nov 6, 2022

Collapsing behavior is based on pseudo-random UUIDs generated for each <ul> in an objects repr. Because the repr is cached, printing the same object twice will return exactly the same repr with the same UUIDs. When you try to collapse the second repr, the first repr will (probably) get collapsed instead.

For some reason, collapsing works correctly in Jupyter classic, Colab, and VS Code, but is broken in Jupyter Lab (including Binder) or when a notebook is rendered statically (nbviewer). I'm guessing this is an implementation detail since the UUID issue isn't platform-dependent.

The quick solution is to cache the Earth Engine data by wrapping getInfo instead of caching the repr, but regenerating the repr may be a non-negligible performance hit for huge collections. If performance is too slow I'll need to think about another solution, e.g. caching the repr but re-generating each pair of UUIDs.

@aazuspan aazuspan added the bug Something isn't working label Nov 6, 2022
@aazuspan
Copy link
Owner Author

aazuspan commented Nov 6, 2022

I ran some tests and generating the HTML is a significant chunk of the total time for large collections and can actually take longer than getInfo, so disabling caching on the HTML generation is a no go.

Another alternative I'm looking into is bypassing UUIDs altogether. Currently, UUIDs are used to tie the header <label> to the hidden <input> checkbox, but that can also be done by just making the <input> a child of the <label> instead of a sibling (see). This would remove any possible interaction between duplicated reprs, save some processing in generating UUIDs, and make the reprs a little lighter.

The challenge is that making the <input> a child of the <label> breaks the CSS selector I'm currently using, and I'm not sure there's any well-supported workaround for that. Basically I need to be able to select eerepr-list elements that are siblings of eerepr-header elements that contain eerepr-collapser:checked inputs. The has:() selector works, but isn't supported by Firefox yet. Instead of relying on pure CSS, I could fix that by injecting some JS into the repr, but I'd like to avoid that if possible to keep things simple.

aazuspan added a commit that referenced this issue Nov 9, 2022
Caching of reprs broke collapsing behavior that depended on unique UUIDs to tie label
elements to hidden checkbox inputs. To fix that, labels and inputs are now connected
implicitly by nesting elements. This change to the HTML structure prevents a pure CSS
solution to collapsing, so a JS snippet was added to restore collapsing behavior.

Skipping UUID generation substantially reduced HTML generation time and slightly
decreased output HTML size. Another performance boost was achieved by removing
singledispatch which has a substantial overhead and replacing with simple isinstance
checks.
@aazuspan
Copy link
Owner Author

aazuspan commented Nov 9, 2022

This is fixed by scrapping UUIDs and connecting label and input elements implicitly. I don't think a pure CSS solution is possible without using has:() which would break Firefox support, so until that changes I'm using JS to handle collapsing behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant