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

Switch examples to Binder #98

Merged
merged 24 commits into from
Dec 3, 2021
Merged

Switch examples to Binder #98

merged 24 commits into from
Dec 3, 2021

Conversation

gjoseph92
Copy link
Owner

@gjoseph92 gjoseph92 commented Dec 3, 2021

Coiled notebooks are deprecated, so we'll now host runnable examples on Binder. This is nice because you don't need a Coiled account to open them anymore (though you do still need one to launch clusters).
In the process, all notebooks have also been rerun and updated (and a couple errors fixed).

This also adds a __version__ to stackstac (finally!). We'll see if this actually works correctly or not at the next release.

Closes #77, closes #97. Also adds jupyter-server-proxy as a dependency, which should address #96 (I think there are multiple separate issues in there).

TBD if this works
this will hopefully keep us more in sync with the coiled environment
Amazingly this just works!
Show still doesn't work though, because binder doesn't allow processes to open any ports. This is a big issue. Working around it would probably require droping `aiohttp` and figuring out how to add an extension to jupyter directly, to keep everything on 8888. Makes switching to binder pretty unappealing.

NOTE: verify that gif.ipynb isn't messed up in this commit
These are verified ready for docs release
still need to remove the coiled ones
ItemCollection has been moved to pystac
it doesn't work with coiled over websockets
It seems to be something broken with BatchedSend (what a surprise!).
_maybe_ something to do with visualizing something that's been persisted
(Futures that depend on other Futures)? Locally at least, commenting out
the `persist()` seems to make things work smoothly.

But the main problem we see is messages getting stuck in the BatchedSend
and not getting sent. If you look at `client.scheduler_comm` when things
should be computing but aren't, you'll usually see that there are
messages in the buffer.

In these cases, `scheduler_comm.waker` is not set. This seems wrong;
calling send should set the event. Unless the deadline is set.

But also, `waker._waiters` is empty, as though nothing is waiting on the
event either. I suppose this would be the case while `background_send`
is awaiting the `comm.write`. Or there's some broken thread-safety
(dask/distributed#5552).

Another thing I've noticed is that `scheduler_comm.next_deadline -
scheduler_comm.loop.time()` goes very, very negative (up to hundreds of
seconds) while there are things in the buffer. As in it's hundreds of
seconds behind when it should have sent.
One question is what happens the deadline for `waker.wait` has already
passed. The "avoid sprious wakeups" logic is assuming that so long as
any deadline is set, the coroutine is going to wake up soon anyway. This
would only be the case if that timeout actually works. It's possible
that a `comm.write` takes longer than the interval, and by the time we
come back around to `waker.wait`, the deadline has already passed. If
the wait returns immediately in that case, then we're fine. But if
tornado schedules the callback into the ether, then we'll never wake up.
Though this still doesn't line up with the empty `_waiters` on the
event.

I guess the last explanation is just that `await comm.write` is really,
really, really slow. If it takes hundreds of seconds to do the write
(including serialization), then of course the next batch wouldn't be
sent yet. Websocket comms do have some ridiculous copying. And maybe
because the binder machines and network are so wimpy, it's just extra
slow there? Feel like this doesn't quite add up though.
@gjoseph92 gjoseph92 merged commit 9d24228 into main Dec 3, 2021
@gjoseph92 gjoseph92 deleted the use-binder branch December 3, 2021 18:03
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.

Error when attempting to bind on address ::1 Switch notebooks from Coiled to Binder
1 participant