Repository navigation
Ember dandi integration with OSB - #17
Conversation
Left for dev environment to be tested fully:
|
| } as const | ||
|
|
||
| /** A draft's validation, as EMBER reports it after every change. */ | ||
| export interface DraftStatus { |
There was a problem hiding this comment.
why we have an interface called DraftStatus with a status parameter that, I assume, can change into something different from draft? Could we please revisit this model and make it clearer? Thanks
| updateDraftMetadata(token: string, dandisetId: string, metadata: DandisetMetadata): Promise<void> | ||
|
|
||
| /** `GET /dandisets/{id}/versions/draft/info/`: whether the draft can be published. */ | ||
| getDraftStatus(token: string, dandisetId: string): Promise<DraftStatus> |
There was a problem hiding this comment.
again, why get Draft status and not getDandiStatus for instance, if this can be something different from draft?
There was a problem hiding this comment.
thanks @ddelpiano! renamed to getPublishReadiness - what we do here is check the status for draft upload by - dandisets/{id}/versions/draft/info
Readiness is carried in three fields. From EMBER's own spec:
status is one of Pending, Validating, Valid, Invalid, Publishing or Published. Once its valid - it returns so - and we can go ahead with the upload.
There was a problem hiding this comment.
checked the api docs again - and fixed! it's now DandisetVersion. DandisetVersion (version, status, errors, filesBeingChecked), read with getVersionInfo(token, dandisetId, version)
There was a problem hiding this comment.
Sorry missed this one, now fixed!
ddelpiano
left a comment
There was a problem hiding this comment.
The structure is sound and well tested. The main problems are in the resume promise: the EMBER sign-in path wipes the checkpoint it means to keep, and the OSB re-login path reloads the page, so the in-memory checkpoint is lost there too. Two smaller gaps: the MAABCD copy can fail with no sign to the user, and only the first page of workspaces is used to match dandisets to workspaces.
Notes
- In
bucketmode,PageLayout.checkLoginsstill requires an EMBER login before the upload dialog opens. The README says this is intended ("Either way"), but it means the bucket fallback can't be used without an EMBER account. If the fallback is meant for when EMBER is unavailable, gate the EMBER check onUPLOAD_BACKEND. /ember-proxy/in nginx forwards any path and theAuthorizationheader to EMBER for any caller. It only reaches EMBER, so the risk is low. Still, consider restricting it to the paths the app actually uses (/oauth/token/,/api/).
Drafted with Claude and reviewed by Dario Del Piano in MetaCell Copilot.
| // The EMBER-DANDI session ended: back to the form, where the sign-in button is. | ||
| // Back to the first step, which points to the Login dialog; the dandiset and workspace stay. | ||
| setForm((prev) => ({ | ||
| ...prev, step: 'select', uploadMessage: EMBER_SIGN_IN, |
There was a problem hiding this comment.
should: The comment above says "the dandiset and workspace stay", but they don't. Moving the form back to step: 'select' makes the next opening treat it as unstarted. The open effect (line 138: current.step === 'select' || …) then calls freshForm(), which throws away checkpoint, dandisetId, spawnedWorkspaceId and the file.
The path is: the EMBER token expires mid-run, the user clicks this branch's Login button (which closes the dialog), signs in, then clicks Data upload again. They get a blank form and a fresh run, possibly a second dandiset, and a workspace that was created but never recorded is orphaned.
Even if the form weren't reset, the dandiset-list effect (line 168) re-preselects list[0] whenever step === 'select', which would overwrite the dandiset this run created.
Suggested fix: make unstarted also require !current.checkpoint, as the 'upload' case already does, or keep a separate flag for "returned to select for EMBER sign-in" and skip the preselect in that state.
There was a problem hiding this comment.
Fair point on throws away - checked. unstarted now also requires !current.checkpoint, and the dandiset preselect skips a form whose checkpoint is for this protocol, so the dandiset and workspace do stay now.
In practice, it's rare anyway: the dialog would starts a run with a valid 24 h EMBER token period.
| if (state.phase === 'failed' && state.error?.toLowerCase().includes(SIGN_IN_AGAIN)) { | ||
| setForm((prev) => ({ | ||
| ...prev, step: 'upload', uploadMessage: '', spawnedWorkspaceId: state.workspaceId ?? prev.spawnedWorkspaceId, | ||
| checkpoint: state.checkpoint ?? prev.checkpoint, |
There was a problem hiding this comment.
should: This keeps the checkpoint so Retry can carry on after the OSB session ends. But the fix offered next is OSB's popup login, and when it completes App.tsx calls window.location.reload(). That wipes the module-level session store, including the checkpoint and the File.
The EMBER popup-blocked fallback (emberAuth.connect) is a full-page redirect and loses the state the same way. In practice, "Retry carries on where a run stopped" doesn't hold for the most common reason a run stops part-way: an expired login.
Suggested fix: persist the serialisable parts of the checkpoint (dandiset id, workspace id, runFolderAt, import flags, stored asset URL/id) to sessionStorage, and ask the user to re-pick the file. Alternatively, have the OSB popup completion refresh the Keycloak state in place instead of reloading. If neither is in scope, say in the PR that resume does not survive re-login.
There was a problem hiding this comment.
Fixed! After the re-login's reload, the dialog reopens with the run stopped and Retry carries on. The file only has to be picked again if the upload itself hadn't finished. An ended OSB session mid-run now stops the run in place instead of resetting the form
… each of the upload backend
|
Also added |
ddelpiano
left a comment
There was a problem hiding this comment.
This PR changes where uploads go: from the public bucket to EMBER-DANDI. Files go into the researcher's own dandiset and, optionally, into the protocol's MAABCD dandiset through OSB. The PR also adds a two-login dialog, a run checkpoint that lets Retry carry on and survives a page reload, and optional publishing for a DOI. It is a large change and mostly hangs together. Before merge I need one answer about who controls the signing key for the MAABCD copy. Two things should be fixed: the nginx proxy only looks up EMBER's address once, at startup, and "Start a new upload" quietly creates another dandiset and workspace.
Notes
- Every environment uses production EMBER (
EMBER_API_ORIGINis hardcoded, andVITE_EMBER_ORIGINwas dropped). So every dev or test run creates public dandisets on the production archive and writes into the real MAABCD dandiset 000607. Each "Publish dandiset" click mints a permanent DOI. If that's intended, say so in the README; otherwise dev runs need a guard or a sandbox. - Most of the new state logic sits in
DataUploadDialog, and none of it has tests. That includes: when the form resets on open, the dandiset preselect,withRetriedRows, the footer vs row Retry, and EMBER_SIGN_IN sending the dialog back to the first step. Several review rounds here have found bugs in exactly this code. Moving it into a pure reducer or helper with unit tests (as was done foruploadSession/runStorage) would help. - I couldn't review files whose patches weren't included, so please check these yourselves:
emberOAuthClient.ts: where the 24hread writeEMBER token is stored (localStorage means any XSS can read it), and how the popup relay checksstateand origin.emberDandiDirectClient.ts: whether a 401 from EMBER maps toEMBER_SIGN_IN. Otherwise an expired or revoked token shows a generic upload error instead of sending the user to sign in.keycloakAuthClient.ts,vite.config.tsandcodefresh-dev.yaml: whetherVITE_EMBER_CLIENT_IDandVITE_UPLOAD_BACKENDare actually passed as build args.- The
LoginDialogdocblock still says EMBER-DANDI is "a full-page OAuth redirect". It now opens in a popup and only redirects when the popup is blocked.
Drafted with Claude and reviewed by Dario Del Piano in MetaCell Copilot.
| }, [step]) | ||
|
|
||
| /** Leaves this run (it stays on OSB and EMBER) for a fresh form on the same protocol; nothing is kept for it. */ | ||
| const startNewUpload = () => setForm({ ...INITIAL_FORM, protocol }) |
There was a problem hiding this comment.
should: This resets dandisetId to '', but the effect that lists and preselects dandisets only re-runs when open, usesEmber, emberSignedIn or protocolId changes. None of those change here, so after a finished run, clicking "Start a new upload":
- leaves the stale list on screen, without the dandiset that run just created. If the list was empty, the dialog still says "No dandiset for this protocol yet…".
- sets the picker to "Create a new dandiset and workspace" instead of
list[0].
The researcher's second upload therefore creates a second dandiset and a second workspace unless they notice. A fix: re-fetch the list (or bump a key the effect depends on) in startNewUpload, or preselect the dandiset just used.




Task description:
Researchers upload their data to EMBER-DANDI instead of the public bucket, and the
analysis runs on it in OSB as before. Each run ends with a DOI for their data.
EMBER login opens in a popup. One Logout signs out of both. Users are assumed to
have an approved EMBER account already (EMBER has its own approval process).
protocol), and optionally a copy into the protocol's MAABCD dandiset ("Share a copy of
this data with the MAABCD consortium", on by default) through OSB's new
/emberendpoints. OSB imports the researcher's copy into the workspace and runs the notebooks.
resources"), so choosing a protocol offers the researcher's dandisets with their workspaces.
version; the dialog shows its DOI with Copy. The MAABCD dandiset is never published.
and closing the dialog doesn't lose a run in progress.
logged in only while it has a token, visible disabled buttons, comment tidy-up.
Screenshots of some of the changes:
The two upload looks like (continuing to keep one logout - which will logout from both)
Login with Ember looks like this for the first time. (We assume user has a ember account - there's no signup process here for Ember like OSB - since Ember needs an approval process - which is something user is expected to do in ember official website)
Notice the Related resources section in Ember Dandiset - it takes it from the metadata coming from the upload.
One can continue from where it failed - so if one of the steps got failed - retry continues from where it failed.
Selecting the same Ember dandiset will show up something like below - since you have published previosuly and new upload does a new publishing to give you new DOI from the same Dandiset.