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

async-2.5: Vispy Changes #1607

Merged
merged 36 commits into from
Sep 9, 2020
Merged

Conversation

pwinston
Copy link
Contributor

@pwinston pwinston commented Sep 2, 2020

Description

@sofroniewn sofroniewn added this to the 0.3.8 milestone Sep 3, 2020
@sofroniewn sofroniewn added async Async rendering feature New feature or request labels Sep 3, 2020
@sofroniewn
Copy link
Contributor

Looks like tests are failing on a simple missing np.all call around the key comparison, but otherwise PR looks very clean and easy to understand.

Copy link
Member

@jni jni left a comment

Choose a reason for hiding this comment

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

Just tried this out and it works great! @pwinston looking at the diff here vs the diff in #1590, I can't for the life of me figure out why we couldn't get things to work with #1590 + ripping out the cache. But, to me these changes make sense, it's the opposite that didn't make sense, so, 🤷. Just curious whether you have additional insight after ripping this out.

I'll also note that I'd like for us to open an issue after this merges to try to improve the "stale" marking. Actually in many contexts (NAPARI_ASYNC=1 napari https://s3.embassy.ebi.ac.uk/idr/zarr/v0.1/6001240.zarr), I find working with the fade to black less pleasant than just having a delay in the display changing, as things are on master now. Anyway, it's obvious that we need to improve that and it's always been on our radar, but I think the many options discussed are scattered throughout the repo and chats, and it would be good to have them in a central place.

@pwinston
Copy link
Contributor Author

pwinston commented Sep 3, 2020

@jni the difference in this PR vs. async-3 is these lines:
https://github.com/pwinston/napari/blob/feature/async-2.5/napari/layers/image/_image_slice.py#L91-L94

Basically for some reason (I did not track it down) we redundantly as for the same slice more than once, in a row. With the cache that doesn't matter. Without the cache we need to explicitly check for duplicates.

Removing the duplicate requests might be good. But this fix is worth doing anyway, for robustness if that ever breaks again.

@pwinston
Copy link
Contributor Author

pwinston commented Sep 3, 2020

I find working with the fade to black less pleasant than just having a delay in the display changing

Yes! This is why we are trying to get all the parts from #1510 merged, so that we can address all the visual issues, which is the very next change we want to make post-#1510. As I think has been communicated.

The nice thing about getting code merged and filing issues, rather than going through it line by line and fixing every last issue during the review, is that we can prioritize, re-order, and parallelize the subsequent fixes instead of doing in linearly with a 12 hour delay between every exchange.

For example once it's merged we might conclude that fixing the visuals is higher priority than improving the caching, because until the visuals are fixed no one is going to use async and thus no cache will ever fill up.

One of my favorite AWS Lambda case studies was a company that wanted re-encode twenty minute video clips faster than real-time. Re-encoding is often slower than real-time, so to re-encode twenty minutes of video it takes more than twenty minutes.

Their solution was break the video into pieces and then fire off 1,200 Lambdas in parallel. Each job took only a few seconds, then they paste the video back together. Imagine how much faster that is than doing the 1,200 in a row, with a discussion between each one?

You want to leverage the capacity the developer to cycle through many issues at the same time, all of varying priority levels, chipping away at all of them. This is especially useful if your one person becomes 10 people, and if you are really successful your one person will become hundreds of people. Pretend your one person is actually 10 people already, and maybe they are with subcontracting these days.

This is all only possible with an opt-in or otherwise guarded/protected feature. How many feature flags do you think Facebook has right now? In this article Uber used a tool to eliminate 2,000 stale flags, no mention of how many non-stale flags there were. There's no way to have thousands of people working on code unless most of that code is flagged and unused by the mainstream. But a lot of those techniques work for teams of 10 people just as well. What scales up really well sometimes scales down really well.

@pwinston
Copy link
Contributor Author

pwinston commented Sep 4, 2020

A good example of above is drawing. To draw a face you don't take out your sharpie and do a detailed drawing the left eye. Then try to guess where the right eye should go. You draw the whole face in pencil, you sketch, you refine, you iterate, you tweak, you redraw. Then when very happy you pull out your pen.

An opt-in feature is pencil, a released feature is in pen. Using a pen to start is a huge mistake, and if you mess up a very costly one. It's also not fun for anyone. Another analogy is you don't put up the wallboard and paint it before you've decided where the walls should go. Huge waste of time and money, and again horribly unfun to tear down a wall you just painted.

More about feature flags:

Copy link
Contributor

@sofroniewn sofroniewn left a comment

Choose a reason for hiding this comment

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

This looks great to me and is now ready to merge. I will merge without further modification, we're just trying to chase down a minimal requirements bug first that's caused problems with our 0.3.7 release and debating on making an emergency patch release. I'm confident we'll have this in by the end of tonight though as is. Thanks for your patience.

@sofroniewn
Copy link
Contributor

Ok, this is ready to go, will merge now

@sofroniewn sofroniewn merged commit b1d75c2 into napari:master Sep 9, 2020
@pwinston pwinston changed the title Async rendering part 2.5 async-2.5: Vispy Changes Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async Async rendering feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants