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

Indicate that the list of interfaces is loading #315

Merged
merged 3 commits into from
Aug 23, 2023

Conversation

garrettmflynn
Copy link
Member

This PR accounts for the delay in the list population given that the server endpoint now has a startup time.

I'm waiting for the server to return the list of valid interfaces before I render any previously selected ones. This ensures an appropriate message is provided to the user, indicating this operation is in progress—though if desired, I can just render the unvalidated list.

The Add Interface button is also hidden from the user until the search list is populated.

@garrettmflynn garrettmflynn self-assigned this Aug 21, 2023
@CodyCBakerPhD
Copy link
Collaborator

There's an alternative to this wait time I think we should try first - can you try to trigger import neuroconv as an isolated operation after initial startup of the app?

The thing about Python imports (as long as the Flask is constantly running anyway) is that there is only a delay to import something heavy the first time it is ever imported, even by local scope

@garrettmflynn
Copy link
Member Author

You're right. Adding neuroconv to the top of app.py results in a near-instantaneous response.

It may still be useful to have these changes in case the response takes longer than expected (e.g. if we end up hosting the backend remotely and make this request over a slow internet connection)—though we don't necessarily have to.

@CodyCBakerPhD
Copy link
Collaborator

Yeah I'll still check it out

Adding neuroconv to the top of app.py results in a near-instantaneous response.

Right but that runs the risk of causing the same issues we've been fixing - I'm saying still use a local scope - but inside a new endpoint that would get triggered in a safe/isolated way on server startup, if that's possible. In case the import fails due to bad environment, only want the endpoint to fail (and even then maybe silently since user didn't explicitly say to do it) not the server itself

@garrettmflynn
Copy link
Member Author

garrettmflynn commented Aug 21, 2023

Was originally intending to test using the changes on #300, but realized they weren't available on main yet. This would be the ideal way to do it, right?

If so, I can simply change the base for this PR.

@CodyCBakerPhD
Copy link
Collaborator

Was originally intending to test using the changes on #300, but realized they weren't available on main yet. This would be the ideal way to do it, right?

Usually yes but also feel free to just port over the import endpoint defined from that PR; the experimental aspect was the companion endpoint for installation which is not needed here, no telling when (or if) we want to merge #300)

@CodyCBakerPhD
Copy link
Collaborator

I'm thinking I should review this and #324 before #326, based on the number of conflicts across all 3 (most in #326)?

Let me know when resolved

@garrettmflynn
Copy link
Member Author

Alright this is up-to-date. Closed #324 and reimplemented in #326 (also now current) to align with updated request results.

@CodyCBakerPhD CodyCBakerPhD merged commit 08c0226 into main Aug 23, 2023
@CodyCBakerPhD CodyCBakerPhD deleted the improve-list-rendering-for-structure branch August 23, 2023 22:08
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.

2 participants