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

Add neurosift external service for dandisets #2041

Merged
merged 14 commits into from
Oct 31, 2024

Conversation

magland
Copy link
Contributor

@magland magland commented Oct 14, 2024

Addresses #2039

This doesn't yet include any of the general framework proposed by @yarikoptic in #2039. I do agree with what you are describing there, so we can view this as step 1 toward that. Whether it gets merged as is first or whether we wait until it is generalized that of course is up to you guys. I can work on it futher, but it will take me time to become more familiar with how data are passed around in the application.

@yarikoptic
Copy link
Member

I don't mind to start with an ad-hoc solution first, but we might need to figure out how to make styling better since it doesn't fit nicely ATM:

image

I think it might make sense it aligned with how it looks in Files browser where we have "Open with"

image

Also, while trying on that CI-driven against staging, I tried to go to
https://neurosift.app/?p=/dandiset&dandisetId=215297&dandisetVersion=draft
and it is simply stuck on "Loading dataset..." instead of stating something more specific -- (that there is no such dataset ID on the main instance of the DANDI archive).

@yarikoptic yarikoptic added UX Affects usability of the system patch Increment the patch version when merged labels Oct 14, 2024
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
@magland
Copy link
Contributor Author

magland commented Oct 14, 2024

Also, while trying on that CI-driven against staging, I tried to go to https://neurosift.app/?p=/dandiset&dandisetId=215297&dandisetVersion=draft and it is simply stuck on "Loading dataset..." instead of stating something more specific -- (that there is no such dataset ID on the main instance of the DANDI archive).

I updated Neurosift so it gives a more specific error message now.

And to solve this, query parameter staging=1 needs to be included in the URL when this is the staging site.

I accepted your commits. Do you think the styling is okay now?

@magland
Copy link
Contributor Author

magland commented Oct 14, 2024

@yarikoptic I have add handling for staging site, although haven't tested that.

alert('Unexpected: No url field found in the metadata of the dandiset');
return;
}
const staging = dandisetUrl.startsWith('https://gui-staging.dandiarchive.org/');
Copy link
Member

Choose a reason for hiding this comment

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

Is there a better way to decide on the "instance name" used to tell apart at the Vue client level not only from staging but also from LINC or temporary testing one? attn @kabilar @aaronkanzer @jjnesbitt . If so we better use that one and overall enable this only for stock DANDI main and staging but not for anything else.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that we have yet abstracted the instance name. At present, it is hard-coded in the LINC fork.

Copy link
Member

Choose a reason for hiding this comment

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

#1810 does abstract the DANDI main and staging deployments.

Copy link
Member

@kabilar kabilar left a comment

Choose a reason for hiding this comment

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

Thank you, @magland. This is great. I left a few minor comments above on the phrasing of the text. I will leave the review of the javascript to the Kitware team.

@kabilar kabilar requested a review from waxlamp October 15, 2024 04:06
…og.vue

Co-authored-by: Kabilar Gunalan <kabilar.gunalan@gmail.com>
@yarikoptic
Copy link
Member

I updated Neurosift so it gives a more specific error message now.

thanks, confirming it does ;)

And to solve this, query parameter staging=1 needs to be included in the URL when this is the staging site.

works well. FWIW, attempt to find similar doesn't work for staging

Similar dandisets Problem loading embeddings: Embedding not found for this dandiset

I accepted your commits. Do you think the styling is okay now?

yes, looks nice as to my non-artistic eye ;-)

@yarikoptic I have add handling for staging site, although haven't tested that.

FWIW, if you missed, in the status comment from CIs there is
image
so it provides URL to the preview at https://deploy-preview-2041--gui-staging-dandiarchive-org.netlify.app/ . It does look consistent/nice as to me, e.g

image

@magland
Copy link
Contributor Author

magland commented Oct 24, 2024

Wondering, is anything blocking this? Thx

@yarikoptic
Copy link
Member

@waxlamp please review

@magland please remove changes to change log - they are automatically populated from the PR title when release comes

@magland
Copy link
Contributor Author

magland commented Oct 24, 2024

changelog entry removed.

@kabilar
Copy link
Member

kabilar commented Oct 25, 2024

Hi @magland, perhaps the Neurosift link should open in a new tab, which would be similar to the behavior in the file browser.

@kabilar
Copy link
Member

kabilar commented Oct 25, 2024

It looks like the option to open with Neurosift is also available for Dandisets that don't contain NWB files (e.g. 211293). On the Neurosift page for this Dandiset it then says that zero assets are loaded and links back to the DLP. Perhaps we can catch this case and disable the Neurosift link when there are no assets to be visualized with Neurosift?

@magland
Copy link
Contributor Author

magland commented Oct 25, 2024

@kabilar

Hi @magland, perhaps the Neurosift link should open in a new tab, which would be similar to the behavior in the file browser.

That sounds good to me... although I don't know how to add the target=_blank attribute to that link. Here's the relevant snippet

<v-list-item
    :href="openInNeurosift()"
>

Does anyone know how to add that?

Perhaps we can catch this case and disable the Neurosift link when there are no assets to be visualized with Neurosift?

That also sounds good, but I'm not sure how to detect that in this file. My suspicion is that it would be a challenge.

@aaronkanzer
Copy link
Member

@kabilar

Hi @magland, perhaps the Neurosift link should open in a new tab, which would be similar to the behavior in the file browser.

That sounds good to me... although I don't know how to add the target=_blank attribute to that link. Here's the relevant snippet

<v-list-item
    :href="openInNeurosift()"
>

Does anyone know how to add that?

Perhaps we can catch this case and disable the Neurosift link when there are no assets to be visualized with Neurosift?

That also sounds good, but I'm not sure how to detect that in this file. My suspicion is that it would be a challenge.

Hi @magland -- you should be able to bind a target HTML attribute to the v-list-item:

<v-list-item
    :href="openInNeurosift()"
    target="_blank"
>

@magland
Copy link
Contributor Author

magland commented Oct 25, 2024

Thanks @aaronkanzer that seems to have worked, and I made the change.

@yarikoptic
Copy link
Member

FWIW, I would have (and probably in the past did) argue to avoid breeding new tabs -- it is IMHO up to the user to open in a new tab/window or just to continue in the current one. But we do have it to open in new tab for per-file browsing, so for consistency of UX it is indeed good to have it here in a new tab too.

@mvandenburgh mvandenburgh self-requested a review October 30, 2024 13:49
@waxlamp
Copy link
Member

waxlamp commented Oct 30, 2024

This looks pretty good so far. I am working on a few changes to the Javascript part to bring it into line with our style in other frontend components.

Copy link
Member

@mvandenburgh mvandenburgh left a comment

Choose a reason for hiding this comment

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

Looks good to me, just one minor code improvement

Co-authored-by: Mike VanDenburgh <37340715+mvandenburgh@users.noreply.github.com>
@waxlamp waxlamp added release Create a release when this pr is merged and removed UX Affects usability of the system labels Oct 31, 2024
@waxlamp waxlamp merged commit 7bb1654 into dandi:master Oct 31, 2024
4 checks passed
@dandibot
Copy link
Member

🚀 PR was released in v0.3.109 🚀

@dandibot dandibot added the released This issue/pull request has been released. label Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged release Create a release when this pr is merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants