000718_demo: drop dandi_authenticate() (dandiset no longer embargoed)#162
Merged
Conversation
…mbargoed) The notebook calls `client.dandi_authenticate()` in three cells (8, 43, 63), each with the inline comment: 'This line is necessary when the dataset is in embargoed mode and only owners can view the data, once it will be published this line can be removed.' Verified via the DANDI API that 000718 is now `embargo_status: OPEN` — so the authentication call is no longer needed (and would break Colab use because Colab users don't have DANDI creds wired up). `dandi_authenticate()` is also what was triggering the `input()` prompt that put this notebook on `.github/notebook-test-exclusions.txt` with 'raises EOFError under non-interactive nbconvert'. Removing the calls makes the notebook CI-testable, so also dropped it from the test-exclusions list. Same fix that landed in #100 for 001172_demo. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Contributor
|
Preview for this PR has been removed (PR closed). |
Two changes to .github/scripts/run_notebook.py: 1. Insert progress markers between cells. The nbconvert-generated .py has `# In[N]:` separators; instrument the script to print `::cell N::` before each. If execution dies mid-cell (eg OOM kills the process), the last printed marker pinpoints the offender. 2. Stream subprocess output live to the parent's stdout instead of capturing it all in memory and printing only on exit. The previous captured-output design meant a SIGTERM'd runner produced empty logs; with streaming, every output up to the kill point reaches the Actions logs. Still tees a copy to the log file for the uploaded artifact. Motivated by PR #162's 000718_demo failure: two consecutive runs got SIGTERM at ~80s with no diagnostic output, so we couldn't tell what was happening. With this instrumentation in place, the next run will show exactly which cell triggers the kill. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The EEG plot cell did:
ax.plot(timestamps, eeg_signal.data[:])
This loads the full ~1-week EEG recording into memory (~60M+ samples
through remfile HTTP range requests, then matplotlib trying to render
every point). CI consistently SIGTERMs ~80s into execution at this
cell — the runner gets OOM-killed.
Subsample to ~50k points for the overview plot. Users who want the
full-resolution signal can slice `eeg_signal.data[start:stop]` for
a specific time window. This is also a real UX improvement on Colab —
plotting 60M points was unusably slow there too.
Caught after PR-#162's CI run showed the kill happens right after the
::cell 41:: progress marker.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
The notebook calls
client.dandi_authenticate()in three cells (8, 43, 63), each with the inline comment:Verified via the DANDI API that
000718is nowembargo_status: OPEN. The authentication call is no longer needed (and would break Colab use anyway — Colab users don't have DANDI creds wired up).Side benefit: CI-testable now
dandi_authenticate()was triggering theinput()prompt that put this notebook on.github/notebook-test-exclusions.txtwith "raises EOFError under non-interactive nbconvert". With the calls gone, the notebook runs headlessly — so this PR also drops it from the test-exclusions list.The per-PR CI workflow will now exercise this notebook on any future change.
Same fix that landed in PR #100 for 001172_demo.
🤖 Generated with Claude Code