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

Create Dandisets from the GUIDE #522

Merged
merged 83 commits into from
Dec 13, 2023
Merged

Create Dandisets from the GUIDE #522

merged 83 commits into from
Dec 13, 2023

Conversation

garrettmflynn
Copy link
Member

fix #221 using the Electron-specific fix linked on dandi/dandi-archive#1640

@garrettmflynn garrettmflynn changed the base branch from main to loose-select-dropdown November 21, 2023 20:58
@CodyCBakerPhD
Copy link
Collaborator

  1. First, the button wasn't easy to spot up in the corner. I kind of knew to go looking for it, but I'd like to recommend a routing page that could also pave the way for adding future features like the llmsearch.dandiarchive.org integration, an 'edit dandiset metadata' page that goes well beyond the UX of the existing website, an 'upload files to existing dandiset' page, and of course this PR, the 'create new dandiset' page (maybe create can be create OR edit existing due to overlap in metadata between the two...)

@garrettmflynn If you agree, could you start a mockup of a Figma for such a landing page page?

  1. Aside from that, a rather odd window nesting - the 'keywords' of the dandiset creation page (3 windows deep) gets pushed to the side

image

  1. And finally, this bug when I hit the 'Create' button
TypeError: dandi.API is not a constructor
    at Button.onClick [as __onClick]

@garrettmflynn
Copy link
Member Author

garrettmflynn commented Nov 21, 2023

For (3), you'll have to reinstall your package.json dependencies using either npm install or npm ci as one was bumped.

@garrettmflynn
Copy link
Member Author

Just fixed (2). Good catch!

@garrettmflynn
Copy link
Member Author

@CodyCBakerPhD I'll need a bit more information as to what we'll be looking for to complete (1).

To make Upload Files to Existing Dandiset and this new Create Dandiset option more interchangeable and obvious, I could simply move the Create Dandiset button directly next to the input—though this seems like this might not encapsulate the requirements you have for new features (e.g. LLM search) and editing existing Dandiset metadata.

So here are some questions:

  1. What operations do we want to do on existing Dandiset metadata?
    • Maybe I missed something from earlier discussions, but this seems out of scope from what I see on the milestones and have heard from prior discussions.
  2. Would this "button by input" approach work for the LLM search too?
    • As I see it, this modal-based presentation of additional layers is fairly extensible. The main reason we'd want to create different views (e.g. on tabs) would be to separate a Basic vs. Advanced mode that both have standalone—rather than additive—functionality at different levels of control

I'm definitely open to re-designing this page in full—though I want to make sure we fully capture all our current and future needs.

This sounds like it might be a great conversation for our meeting tomorrow or the next NWB GUIDE meeting :)

@CodyCBakerPhD
Copy link
Collaborator

Selector is working again: it seemed to work fine but console did throw this error after displaying the results window

Uncaught (in promise) TypeError: Cannot set properties of null (setting 'innerHTML')
    at DandiResults.updated (DandiResults.js:54:33)

@CodyCBakerPhD
Copy link
Collaborator

The main result of which seems to be this appearance on the page after upload

image

@CodyCBakerPhD
Copy link
Collaborator

Ah... And this is persistent if I close and restart the app

@CodyCBakerPhD
Copy link
Collaborator

Since this PR requires this work to debug, I'll also throw in another request

Would it be hard to order entries in the selector with draft versions starting from the top instead of the bottom? Those are the more likely targets for uploads, though uploading to an already published one is not impossible or unusual

@garrettmflynn
Copy link
Member Author

Is there an error when the loading is happening? The first error shouldn't change anything related to fetching all of your Dandisets—which is what is happening on the Uploads page on refresh.

@garrettmflynn
Copy link
Member Author

Yeah reordering is quite simple, I'll get on that

@CodyCBakerPhD
Copy link
Collaborator

Did a hard restart of the console / gave more time after last shut down and that seemed to reset the page

Only thing I notice otherwise is the thing on the conda console (not the app dev console)

[13224:1213/104532.219:ERROR:cert_issuer_source_aia.cc(36)] Error parsing cert retrieved from AIA (as DER):
ERROR: Couldn't read tbsCertificate as SEQUENCE
ERROR: Failed parsing Certificate

@CodyCBakerPhD
Copy link
Collaborator

But oddly, the form contents didn't get stuck this time, still see previous error on dev console though

@CodyCBakerPhD
Copy link
Collaborator

CodyCBakerPhD commented Dec 13, 2023

The issue is sporadic for me and seems to affect other things on the app

I just let it close down and waited for about a minute (it also tends to get stuck on the 'killing all active processes bit' then got

image

on next launch

The connection to the backend also otherwise stalls out even if I don't get this message

@garrettmflynn
Copy link
Member Author

Hmmm that's weird. I don't have it on my system—and since it's a certificate issue, I think it'll be mostly an issue on your system. I'll try on my Windows PC to see if it pops up there.

Otherwise the ordering and HTML-related error have been fixed.

@garrettmflynn
Copy link
Member Author

Can you see if it's fixed by running main?

@CodyCBakerPhD
Copy link
Collaborator

OK, pretty weird issue, present on main as well

After a successful upload to dandi using the app, I close down the app and then reboot it

After reboot, it never connects to the backend, and the uploads page says it's waiting to connect to the Flask server (different display message on main, but I assume same cause)

No errors on any console

Attempting to exit the app at this point closes the main window but gets stuck on the 'Killing all previous processes' in the main console

@CodyCBakerPhD
Copy link
Collaborator

This is on a completely fresh environment following a hard reboot to clear some process that was preventing removal of the SLL .dll files in the conda environment

@CodyCBakerPhD
Copy link
Collaborator

After like 5 minutes of waiting, this did just randomly pop up in the dev console

index.ts:63 Uncaught (in promise) ReferenceError: statusBar is not defined
    at pythonServerClosed (index.ts:63:3)
    at EventEmitter.pythonServerOpened (index.ts:55:15)

@CodyCBakerPhD
Copy link
Collaborator

And at the same time as that, the backend printed the usual

WARNING: This is a development server. Do not use it in a production deployment. Use a production WSGI server instead.
 * Running on http://127.0.0.1:4242

but still claims to not be connected to the backend - but the app can now successfully close down as usual

@garrettmflynn garrettmflynn mentioned this pull request Dec 13, 2023
@garrettmflynn
Copy link
Member Author

We caught the statusBar when I initially started working in the Dandiset creation PR. I'll push a quick fix to main for that

@garrettmflynn
Copy link
Member Author

Ah sure. The form waits for two conditions:

  1. CPU information is resolved
  2. All dandisets are resolved

So if the server is down, it wouldn't load.

@CodyCBakerPhD
Copy link
Collaborator

OK - considering the issues found here as separate in #540

The actual features of this PR feel great at this point

@CodyCBakerPhD
Copy link
Collaborator

One question on that last chromatic page - is it possible to get a story for the new layout? @garrettmflynn

@garrettmflynn
Copy link
Member Author

Updated! Let's see if it works

@CodyCBakerPhD CodyCBakerPhD merged commit 57ee19b into main Dec 13, 2023
11 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the create-dandiset branch December 13, 2023 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle Embargoed Dandiset Creation Create Dandisets using the REST API
2 participants