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

Improved Path Expansion Interaction #609

Merged
merged 13 commits into from
Feb 20, 2024
Merged

Conversation

garrettmflynn
Copy link
Member

@garrettmflynn garrettmflynn commented Feb 12, 2024

fix #528

There are more improvements we could make here (e.g. #510), but here's a working integration of https://github.com/catalystneuro/neuroconv/pull/680/files.

@CodyCBakerPhD
Copy link
Collaborator

UI looks/feels really good so far

However, run into

index.js:39 [main-process]: [2024-02-18 14:05:11,925] ERROR in app: Exception on /neuroconv/locate/autocomplete [POST]
Traceback (most recent call last):
  File "D:\GitHub\nwb-guide\pyflask\apis\neuroconv.py", line 86, in post
    return autocomplete_format_string(neuroconv_api.payload)
  File "D:\GitHub\nwb-guide\pyflask\manageNeuroconv\manage_neuroconv.py", line 131, in autocomplete_format_string
    raise "Path is not contained in the provided base directory."
TypeError: exceptions must derive from BaseException

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\flask\app.py", line 1484, in full_dispatch_request
    rv = self.dispatch_request()
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\flask\app.py", line 1469, in dispatch_request
    return self.ensure_sync(self.view_functions[rule.endpoint])(**view_args)
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\flask_restx\api.py", line 404, in wrapper
    resp = resource(*args, **kwargs)
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\flask\views.py", line 109, in view
    return current_app.ensure_sync(self.dispatch_request)(**kwargs)
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\flask_restx\resource.py", line 46, in dispatch_request
    resp = meth(*args, **kwargs)
  File "D:\GitHub\nwb-guide\pyflask\apis\neuroconv.py", line 89, in post
    neuroconv_api.abort(500, str(exception))
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\flask_restx\namespace.py", line 153, in abort
    abort(*args, **kwargs)
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\flask_restx\errors.py", line 28, in abort
    flask.abort(code)
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\flask\helpers.py", line 277, in abort
    current_app.aborter(code, *args, **kwargs)
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\werkzeug\exceptions.py", line 861, in __call__
    raise self.mapping[code](*args, **kwargs)
werkzeug.exceptions.InternalServerError: 500 Internal Server Error: The server encountered an internal error and was unable to complete your request. Either the server is overloaded or there is an error in the application.

when I actually hit submit on the popup

SpikeGLX-Phy pipeline on tutorial data

@CodyCBakerPhD
Copy link
Collaborator

(Perhaps symlink related?)

@CodyCBakerPhD
Copy link
Collaborator

Yes, can confirm it's a symlink issue. The SpikeGLX one works fine

@garrettmflynn I can flag this on the NeuroConv side, but for now I guess it might be simplest for everyone if we just copy the artificial Phy data to each subdirectory, similar to the SpikeGLX. Could you make that adjustment? (the Phy data is pretty lightweight)

Comment on lines 130 to 131
if not is_path_contained(filesystem_entry_path, base_directory):
raise "Path is not contained in the provided base directory."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, nevermind, looks like the failure happened before reaching NeuroConv after all

Copy link
Member Author

@garrettmflynn garrettmflynn Feb 19, 2024

Choose a reason for hiding this comment

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

Just to confirm, this is about the Phy symlink stuff?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It was a secondary error triggered by this, which is still the main issue I run into

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it, thanks for the clarification. I'll see if I can replicate on my Mac

@CodyCBakerPhD
Copy link
Collaborator

Steps to reproduce

  • generate tutorial data
  • generate pipelines
  • go to SpikeGLX-Phy pipeline
  • for Phy, specify the base folder for the tutorial dataset
  • go through autocomplete; select the example folder as one of the 4 phy folders and type in the subject/session IDs
  • hit 'create'
Traceback (most recent call last):
  File "D:\GitHub\nwb-guide\pyflask\apis\neuroconv.py", line 86, in post
    return autocomplete_format_string(neuroconv_api.payload)
  File "D:\GitHub\nwb-guide\pyflask\manageNeuroconv\manage_neuroconv.py", line 131, in autocomplete_format_string
    raise ValueError("Path is not contained in the provided base directory.")
ValueError: Path is not contained in the provided base directory.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\flask\app.py", line 1484, in full_dispatch_request
    rv = self.dispatch_request()
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\flask\app.py", line 1469, in dispatch_request
    return self.ensure_sync(self.view_functions[rule.endpoint])(**view_args)
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\flask_restx\api.py", line 404, in wrapper
    resp = resource(*args, **kwargs)
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\flask\views.py", line 109, in view
    return current_app.ensure_sync(self.dispatch_request)(**kwargs)
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\flask_restx\resource.py", line 46, in dispatch_request
    resp = meth(*args, **kwargs)
  File "D:\GitHub\nwb-guide\pyflask\apis\neuroconv.py", line 89, in post
    neuroconv_api.abort(500, str(exception))
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\flask_restx\namespace.py", line 153, in abort
    abort(*args, **kwargs)
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\flask_restx\errors.py", line 28, in abort
    flask.abort(code)
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\flask\helpers.py", line 277, in abort
    current_app.aborter(code, *args, **kwargs)
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\werkzeug\exceptions.py", line 861, in __call__
    raise self.mapping[code](*args, **kwargs)
werkzeug.exceptions.InternalServerError: 500 Internal Server Error: The server encountered an internal error and was unable to complete your request. Either the server is overloaded or there is an error in the application.

Unless I'm missing something

@garrettmflynn
Copy link
Member Author

Ah I see. While Electron doesn't resolve the symlink (if you click on it, rather than into it), the Path(x).resolve() call in the comparison does on the Python side of things.

Fixed that. Once symlinks resolve, I can't think of a reliable way to do this at all.

@CodyCBakerPhD
Copy link
Collaborator

Now the problem seems to be that when I select the symlinked Phy folder, it resolves to the actual test data folder instead of the one to apply autocomplete to

@garrettmflynn
Copy link
Member Author

garrettmflynn commented Feb 19, 2024

Yes, that's unavoidable when going through the filesystem using the native file selector. I'd gestured at this when distinguishing between clicking on the folder and clicking into it. Here are some screenshots to illustrate this point:

Works (click Phy once and press Open)

Screenshot 2024-02-19 at 2 06 58 PM

Fails (after double-clicking Phy, press Open)

Screenshot 2024-02-19 at 2 07 04 PM

This means that symlinks are fundamentally going to present a challenge for this helper, as the way this interaction works is inherent in Electron and controlled by the user.

@CodyCBakerPhD
Copy link
Collaborator

Works (click Phy once and press Open)

Windows doesn't have this behavior; selecting it by the 'once -> open' method works in the selector then becomes the true folder in the gray box, where there's no path to specify

Is there a way to detect if it is a symlink and forbid it with a message along the lines of 'symlinks aren't allowed for this feature'?

If that's all we can do, then should definitely adjust the tutorial data to copy Phy instead of symlinking it

@garrettmflynn
Copy link
Member Author

Yeah, that sounds like all we can do. I've added two errors: One when you provide a symbolic link directly, and another as soon as you provide a path that isn't included in the base_directory.

@CodyCBakerPhD
Copy link
Collaborator

The errors aren't showing up on Windows (still resolving directly to the target) - can you give your Windows a try? (and then update tutorial generator to copy)

@garrettmflynn
Copy link
Member Author

I'm not at home right now, but I'll swap out for my PC as soon as I can.

@garrettmflynn
Copy link
Member Author

The errors aren't showing up on Windows (still resolving directly to the target) - can you give your Windows a try?

These should pop up as red errors underneath the file input. I've just confirmed on a fresh Windows system that the "Must specify path in base directory" error works for the Phy test dataset, whereas the "Does not handle symlinks" one will never show up because Windows handles the resolution of the symlink path internally—so I'm not able to catch it except for the fact that the path is no longer contained.

@garrettmflynn
Copy link
Member Author

No error was shown for you?

@CodyCBakerPhD
Copy link
Collaborator

No error was shown for you?

I do get "Must specify path in base directory", but was expected the symlink error. The current one is rather confusing in explaining the 'why' of the error since I feel like I did what I was supposed to do in that case

whereas the "Does not handle symlinks" one will never show up because Windows handles the resolution of the symlink path internally

since it's not possible for Windows, maybe modify the other error message to include "This is likely due to the target being a symlink, which is unsupported by this feature."?

@garrettmflynn
Copy link
Member Author

garrettmflynn commented Feb 20, 2024

Got it. I'll add that as some subtext

@garrettmflynn
Copy link
Member Author

Updated! This should also identify the issue if users have a symlink further up the path across all platform.

@CodyCBakerPhD CodyCBakerPhD marked this pull request as ready for review February 20, 2024 17:00
@CodyCBakerPhD
Copy link
Collaborator

Looks and feels great!

@CodyCBakerPhD CodyCBakerPhD enabled auto-merge (squash) February 20, 2024 17:01
@CodyCBakerPhD CodyCBakerPhD merged commit d80764a into main Feb 20, 2024
12 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the path-expansion-improvement branch February 20, 2024 17:07
@bendichter
Copy link
Collaborator

bendichter commented Feb 27, 2024

@garrettmflynn I find this to be very busy

image

I much prefer this view:

image

In general, the error messages are really crowding up this interface all over GUIDE. I think usually with forms you only get these messages if you edit a field or if you try to press continue before the end. Would it be possible to do that here? Otherwise, I think the UI is so busy it's hard to know what to do.

@bendichter
Copy link
Collaborator

Also, for this element:

image

I think it we should be able to determine if this should be a directory or a file given the signature of the interface. Could you constrain it that way?

@bendichter
Copy link
Collaborator

Another thing. I tried entering "example" and then finding the relevant file, and the error message continued on the subject field.

image

If you are expecting the user to file the file or folder first, can you inactivate the other fields until one is selected?

@bendichter
Copy link
Collaborator

I think calling the button "Create" is a bit confusing. I understand you mean "create an fstring," but I think "Done" makes more sense here.

@bendichter
Copy link
Collaborator

Another thing. I tried entering "example" and then finding the relevant file, and the error message continued on the subject field.

image If you are expecting the user to file the file or folder first, can you inactivate the other fields until one is selected?

Another thing, if I try to continue after entering the fields out of order I get an error message even though the data is entered correctly:

image

@bendichter
Copy link
Collaborator

Can you also change "Example Filesystem Entry" to "Example file" or "Example folder" depending on the requirements of the Interface?

@bendichter
Copy link
Collaborator

Here, I am in the SpikeGLXConverter AutoComplete modal

image

It's not clear to me what file or folder I should be selecting. Should it be a .bin file? Should it be the metadata file, or the folder that contains all of it? This information should be in the description of the field in the json schema. Can you have that description show up here?

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.

[Feature] Intuitive interface for inferring path expansion
3 participants