For staging environment, include dynamic text for `dandi download` command #1810

aaronkanzer · 2024-01-11T20:54:00Z

When using gui-staging.dandiarchive.org, a minor inconsistency was observed where the CLI command was incorrect.

This PR makes a simple fix to render the appropriate text depending on if the user is using staging or production.

kabilar

Thank you @aaronkanzer. This looks good to me.

Hi @waxlamp, when you have a chance, this pull request is ready for review. I am not a JavaScript developer so it would be good to have you review. Thank you.

yarikoptic · 2024-01-25T15:21:49Z

web/src/components/FileBrowser/FileUploadInstructions.vue

+  return dandisetIdentifier.value
+    ? `> dandi download ${baseUrl}${dandisetIdentifier.value}/draft`
+    : ''; // Empty string just as a fallback in case store.dandiset? is undefined
+});


this is a good first step since would allow both main and staging one but it is not a generic solution.

The dandi-cli is actually "too smart" to support a wide range of URLs and they do not have to be associated with a specific "short named" instance (if something fails -- it is a bug):

❯ dandi download --help Usage: dandi download [OPTIONS] [URL]... Download files or entire folders from DANDI. Accepted resource identifier patterns: - DANDI:<dandiset id>[/<version>] - https://gui.dandiarchive.org/... - https://identifiers.org/DANDI:<dandiset id>[/<version id>] (<version id> cannot be 'draft') - https://<server>[/api]/[#/]dandiset/<dandiset id>[/<version>][/files[?location=<path>]] - https://*dandiarchive-org.netflify.app/... - https://<server>[/api]/dandisets/<dandiset id>[/versions[/<version>]] - https://<server>[/api]/assets/<asset id>[/download] - https://<server>[/api]/dandisets/<dandiset id>/versions/<version>/assets/<asset id>[/download] - https://<server>[/api]/dandisets/<dandiset id>/versions/<version>/assets/?path=<path> - https://<server>[/api]/dandisets/<dandiset id>/versions/<version>/assets/?glob=<glob> - dandi://<instance name>/<dandiset id>[@<version>][/<path>] - https://<server>/...

so here IMHO it just should use local server URL... chatgpt suggested some code like this

computed: { baseUrl() { const protocol = window.location.protocol; const host = window.location.hostname; const port = window.location.port ? `:${window.location.port}` : ''; return `${protocol}//${host}${port}`; } }

To clarify though -- for example, a user on identifiers.org though does not see the DANDI CLI download command, so I'm curious why not confine it just to the two specific places where it might render.

I do agree that it could be more generic; however, I'd vote to keep it simple and confined in case an unknown edge case exists (e.g. windoww.location.protocol is tied to the browser, not Vue, etc., etc.)

I pushed a few commits to clean up the PR and change the URL generation to something close to Yarik's ChatGPT code. Check it out and let me know what you think.

To clarify though -- for example, a user on identifiers.org though does not see the DANDI CLI download command, so I'm curious why not confine it just to the two specific places where it might render.

sorry, missed the question -- what "two specific places" you have in mind?

here the list of supported URLs organically grown to make it easier for a user to download, e.g. "just cut paste URL from the browser". identifiers.org is indeed niche use case, and would work only for the main instance unless we add some other identifiers to handle in addition to DANDI. Indeed it is not really needed and likely not really used, but if dandi is asked to download it and it knows how to parse it -- why not? ;)

waxlamp · 2024-02-23T01:53:25Z

Just a note to @aaronkanzer and @kabilar: thanks for contributing this; I'm taking a look.

This should not cause problems at runtime, as we only ever instantiate this component inside the DLP, where of necessity a dandiset is set. If that assumption fails, we will want to know about it rather than just handing the user a blank command string.

yarikoptic · 2024-02-23T22:39:52Z

web/src/views/DandisetLandingView/DownloadDialog.vue

@@ -122,7 +122,7 @@ import CopyText from '@/components/CopyText.vue';

 function formatDownloadCommand(identifier: string, version: string): string {
  if (version === 'draft') {
-    return `dandi download https://dandiarchive.org/dandiset/${identifier}/draft`;
+    return `dandi download ${window.location.origin}/dandiset/${identifier}/draft`;


note that below for releases, not draft, we still keep using DANDI: identifier which would not be correct one. I would have made them also generic and to use ${window.location.origin}/dandiset/${identifier} even though loosing that nice DANDI: convenience (well -- could be a matter of a single if to decide to use DANDI: if the main instance)

@aaronkanzer do you think you would have time to add custom handling below for DANDI: as well? it is pretty much "use DANDI: if window.location.origin == 'https://dandiarchive.org'", and otherwise needs a ${window.location.origin}/dandiset/ prefix

Updated here -- let me know if this is what you had in mind -- thanks again

I think it looks good.

Should/could we proceed here or any other aspect remains open?

yarikoptic · 2024-09-09T21:58:38Z

@aaronkanzer ping on above -- could it be considered done??

kabilar · 2024-09-10T14:51:08Z

Thanks @yarikoptic. Aaron is out of the office for the beginning of this week. I will touch base with him when he gets back and we can try to close this pull request out this week.

aaronkanzer · 2024-09-16T17:34:15Z

@yarikoptic works for me -- if you want to approve, I'm happy to merge.

waxlamp

@yarikoptic (cc: @aaronkanzer), I think this is ready to go but I had a couple of questions (see the inline comments below).

You can see this in action for a non-dandiarchive URL at the Netlify deploy preview. We will need to deploy to production to see it in action on the canonical dandiarchve.org instance, but it's low risk to do that since it won't disrupt the actual functioning of the archive and we can deploy quick fixes if anything is wrong.

Let me know what you think.

waxlamp · 2024-09-20T14:19:48Z

web/src/components/FileBrowser/FileUploadInstructions.vue

+}
+
+const downloadCommand = computed(() => {
+  return `dandi download ${window.location.origin}/dandiset/${dandisetIdentifier.value}/draft`


I did not really follow the discussion in the other thread. Could you summarize here whether this logic is correct for the upload instructions? Is there a reason not to use the same logic as in the download dialog below? Specifically, if a user downloads their dandiset via dandi download DANDI:123456/draft, will the subsequent dandi upload command get confused about where to upload?

The DANDI CLI should still be OK -- as @yarikoptic mentioned that any short-hand prefix shouldn't break here, as "The dandi-cli is actually "too smart" to support a wide range of URLs and they do not have to be associated with a specific "short named" instance (if something fails -- it is a bug)"

I can test further if you'd like more confirmation

web/src/views/DandisetLandingView/DownloadDialog.vue

aaronkanzer · 2024-09-20T14:38:30Z

@yarikoptic (cc: @aaronkanzer), I think this is ready to go but I had a couple of questions (see the inline comments below).

You can see this in action for a non-dandiarchive URL at the Netlify deploy preview. We will need to deploy to production to see it in action on the canonical dandiarchve.org instance, but it's low risk to do that since it won't disrupt the actual functioning of the archive and we can deploy quick fixes if anything is wrong.

Let me know what you think.

This sounds good to me, as it won't disturb the API via testing in a prod UI environment -- thanks for the updates here

waxlamp · 2024-09-20T14:55:18Z

@yarikoptic (cc: @aaronkanzer), I think this is ready to go but I had a couple of questions (see the inline comments below).
You can see this in action for a non-dandiarchive URL at the Netlify deploy preview. We will need to deploy to production to see it in action on the canonical dandiarchve.org instance, but it's low risk to do that since it won't disrupt the actual functioning of the archive and we can deploy quick fixes if anything is wrong.
Let me know what you think.

This sounds good to me, as it won't disturb the API via testing in a prod UI environment -- thanks for the updates here

Great, then I'll go ahead and merge this and we can still have a discussion about the questions in my self-review above (and do followup PRs in case anything needs to be fixed). Thanks!

yarikoptic · 2024-09-20T16:00:27Z

You can see this in action for a non-dandiarchive URL at the Netlify deploy preview. We will need to deploy to production to see it in action on the canonical dandiarchve.org instance, but it's low risk to do that since it won't disrupt the actual functioning of the archive and we can deploy quick fixes if anything is wrong.

Let me know what you think.

sounds sensible -- let's just try to keep this in memory so we could check as soon as release comes out. Any plans to release soon?

waxlamp · 2024-09-20T20:42:34Z

Any plans to release soon?

Oops, I meant to merge this as a release. I'll set up an "empty" PR to trigger a release now. Stand by.

dandibot · 2024-09-20T20:44:32Z

🚀 PR was released in v0.3.102 🚀

waxlamp · 2024-09-21T00:18:46Z

Seems to be working nicely 😸

Include dynamic text for staging environment for dandi download

2d703e7

aaronkanzer changed the title ~~For staging environment, Include dynamic text for dandi download command~~ For staging environment, include dynamic text for dandi download command Jan 11, 2024

kabilar requested a review from waxlamp January 13, 2024 02:02

kabilar reviewed Jan 17, 2024

View reviewed changes

yarikoptic reviewed Jan 25, 2024

View reviewed changes

waxlamp added 6 commits February 23, 2024 10:11

Revert .gitignore update

62d8820

Revert spacing changes

b983ab5

Revert spacing change

fc46cff

Use window.location instead of URL selection logic

b19f9d0

Factor the command string out of its presentation

54f1714

yarikoptic reviewed Feb 23, 2024

View reviewed changes

Add more logic per Yarik's comment

f4564cd

waxlamp self-assigned this Sep 10, 2024

Simplify download command string function

8b846b3

waxlamp reviewed Sep 20, 2024

View reviewed changes

waxlamp approved these changes Sep 20, 2024

View reviewed changes

waxlamp merged commit 117b7ef into dandi:master Sep 20, 2024
4 checks passed

aaronkanzer deleted the aaronkanzer-staging-command branch September 20, 2024 16:24

dandibot added the released This issue/pull request has been released. label Sep 20, 2024

kabilar mentioned this pull request Oct 15, 2024

Add neurosift external service for dandisets #2041

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For staging environment, include dynamic text for `dandi download` command #1810

For staging environment, include dynamic text for `dandi download` command #1810

aaronkanzer commented Jan 11, 2024 •

edited by waxlamp

Loading

kabilar left a comment

yarikoptic Jan 25, 2024

aaronkanzer Jan 25, 2024

waxlamp Feb 23, 2024

yarikoptic Feb 23, 2024

waxlamp commented Feb 23, 2024

yarikoptic Feb 23, 2024

yarikoptic Apr 19, 2024

aaronkanzer May 7, 2024

yarikoptic Jul 10, 2024

yarikoptic commented Sep 9, 2024

kabilar commented Sep 10, 2024

aaronkanzer commented Sep 16, 2024

waxlamp left a comment

waxlamp Sep 20, 2024

aaronkanzer Sep 20, 2024

aaronkanzer commented Sep 20, 2024

waxlamp commented Sep 20, 2024

yarikoptic commented Sep 20, 2024

waxlamp commented Sep 20, 2024

dandibot commented Sep 20, 2024

waxlamp commented Sep 21, 2024

For staging environment, include dynamic text for dandi download command #1810

For staging environment, include dynamic text for dandi download command #1810

Conversation

aaronkanzer commented Jan 11, 2024 • edited by waxlamp Loading

kabilar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waxlamp commented Feb 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yarikoptic commented Sep 9, 2024

kabilar commented Sep 10, 2024

aaronkanzer commented Sep 16, 2024

waxlamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronkanzer commented Sep 20, 2024

waxlamp commented Sep 20, 2024

yarikoptic commented Sep 20, 2024

waxlamp commented Sep 20, 2024

dandibot commented Sep 20, 2024

waxlamp commented Sep 21, 2024

For staging environment, include dynamic text for `dandi download` command #1810

For staging environment, include dynamic text for `dandi download` command #1810

aaronkanzer commented Jan 11, 2024 •

edited by waxlamp

Loading