adjust neurosift service endpoint URL, passing additional info

[magland]

This adjusts the external endpoint URL for neurosift such that it:

• always provides the version of the asset URL that includes that asset ID (rather than just doing this for embargoed dandisets)
• provides the dandiset ID and version

This is information I would like to have access to on the neurosift end.

I tried to do this in the most straightforward way I could -- if adjustments are needed in the future I think it wouldn't be a problem since it only affects the one file. But if there is a better way @waxlamp, feel free to suggest.

The other external services should work the same as they did before.

I realize that this is related to #1655 but I wanted to start from the latest commit that includes the other related changes from #1692

[waxlamp]

@magland, would you mind enabling me to push commits to your branch? I have a few minor updates to make this tighter that I'd like to add.

[magland]

@magland, would you mind enabling me to push commits to your branch? I have a few minor updates to make this tighter that I'd like to add.

Sure, I have added you to https://github.com/magland/dandi-archive

[yarikoptic]

I also see

on the right, so @waxlamp should be able to push to your branch @magland without you adding him explicitly AFAIK... even I am allowed to commit to it ;) (I removed that comment since then)

[waxlamp]

I also see on the right, so @waxlamp should be able to push to your branch @magland without you adding him explicitly AFAIK... even I am allowed to commit to it ;) (I removed that comment since then)

Yes, I apologize for being rather unclear in my original request, @magland -- I just wanted you to enable the option to allow maintainers to collaborate on your PR branch.

[waxlamp]

@magland I made a few modifications that I'd like your feedback on. Basically, I made the "API" less abstract by removing the base API information, and supplying more explicit asset URLs (one of which simplifies your Neurosift template changes significantly).

I also removed the asset ID from the transmitted data because your use case no longer needs it and I want to make sure we don't overplay this new "API" without a serious discussion. I'm going to convene such a discussion ASAP so that we have a chance to weigh in on this without delaying merging this PR (when it's ready). I'll include you in that discussion, @magland.

In the meantime, let me know what you think of my changes (and @AlmightyYakob, please weigh in as well).

Thanks, @magland, for sending in this PR, and sorry for the delay in getting to this review.

[magland]

@waxlamp This looks great to me. You are right that neurosift does not need the asset ID if it has the asset_dandi_url. And I do prefer the asset_dandi_url rather than the asset_url even though it requires an extra step on page load because then it is handling things in the same way whether or not it's an embargoed dataset. So what you have there looks perfect.

@yarikoptic I didn't know about that capability - that's helpful. Is there anything you want me to do at this point to enable something?

[yarikoptic]

@yarikoptic I didn't know about that capability - that's helpful. Is there anything you want me to do at this point to enable something?

nope, AFAIK all is good here and in #1666 in that regard.

[yarikoptic]

@waxlamp ping on this, so we could progress forward (there are PRs, #1755, coming which would conflict already)

[magland]

@waxlamp @yarikoptic It would be great if this could be merged/released soon. I ran into an issue with an embargoed dataset where the neurosift link didn't work because it was the s3 bucket link (see below, although you prob don't have access). I thought we had solved that with the previous PR #1692. But in any event, I think this PR here would certainly do the trick because then it would always pass the api URL to neurosift.

https://dandiarchive.org/dandiset/000620/draft/files?location=sub-Elgar&page=1

[waxlamp]

Sorry for the delay on this. Looks good to me!

[dandibot]

🚀 PR was released in v0.3.64 🚀