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

Docker voila #226

Merged
merged 21 commits into from
Oct 27, 2022
Merged

Docker voila #226

merged 21 commits into from
Oct 27, 2022

Conversation

luiztauffer
Copy link
Collaborator

@luiztauffer luiztauffer commented Oct 17, 2022

  • Container image that serves a Voila process running nwbwidgets with Panel()
  • Panel update
  • documentation update

Depends on #225

@bendichter
Copy link
Collaborator

bendichter commented Oct 19, 2022

lgtm, we just need to wait for the next pynwb release

@luiztauffer luiztauffer marked this pull request as ready for review October 20, 2022 17:08
@codecov
Copy link

codecov bot commented Oct 20, 2022

Codecov Report

Merging #226 (4770f32) into master (0df2857) will increase coverage by 0.43%.
The diff coverage is 82.00%.

@@            Coverage Diff             @@
##           master     #226      +/-   ##
==========================================
+ Coverage   68.60%   69.03%   +0.43%     
==========================================
  Files          29       30       +1     
  Lines        2086     2157      +71     
==========================================
+ Hits         1431     1489      +58     
- Misses        655      668      +13     
Flag Coverage Δ
unittests 69.03% <82.00%> (+0.43%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
nwbwidgets/panel.py 80.00% <78.57%> (-4.10%) ⬇️
nwbwidgets/utils/dandi.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bendichter
Copy link
Collaborator

So how would someone use this?

@bendichter
Copy link
Collaborator

Oh I see it launches the panel. Could you also allow it to take as input a file path or s3 path in which case it just renders that file?

@luiztauffer
Copy link
Collaborator Author

Oh I see it launches the panel. Could you also allow it to take as input a file path or s3 path in which case it just renders that file?

sure! but then this would be a modification to the Panel, I can make it in a next PR, together with other improvements (e.g. auto-filling the list of existing DANDIsets, etc...)

@bendichter
Copy link
Collaborator

Would that work? Wouldn't you need to pass a variable to modify the contents of the notebook?

@luiztauffer
Copy link
Collaborator Author

The Panel itself should have one extra functionality to be able to read from any S3 path, which is not difficult to implement, it is already doing so indirectly by getting the S3 path from a DANDI set. We wouldn't have to change anything when running the container
Of course, if running from a container this would be mostly used for streaming remote data

@bendichter
Copy link
Collaborator

would it be possible to make this docker function able to take an s3 path as input? That would allow us to, e.g. create a button on a web interface and launch a viewer for a specific NWB file.

@luiztauffer
Copy link
Collaborator Author

@bendichter I changed it now so it works in the following way (see also README):

  1. If at container run time we pass an S3 url as an argument, the Notebook will run nwb2widget streaming this specific file

  2. If no s3 url is passed, it runs with the Panel

@bendichter
Copy link
Collaborator

using an environmental variable! smart! I'm just going to test this out on my own computer, then it's good to go

@bendichter
Copy link
Collaborator

bendichter commented Oct 24, 2022

image

Can you fix the layout of the source box?

@bendichter
Copy link
Collaborator

When I try to load a local file I'm getting "Invalid local dir path"

@luiztauffer
Copy link
Collaborator Author

@bendichter yes I can improve this layout
There are also other necessary improvements on Panel that are easy to make and I'll solve in the coming days:

  • Auto-list the existing DANDI sets if DANDI is chosen (only once per run)
  • allow for boolean control, on Panel() instantiation, for source options. In a running docker container, supposedly running in a remote machine, the local dir/file options wouldn't make sense, so we could disable these options

if you're running this container, you're getting the local file error because there's no such file inside your container

@luiztauffer
Copy link
Collaborator Author

luiztauffer commented Oct 24, 2022

In a running docker container, supposedly running in a remote machine, the local dir/file options wouldn't make sense, so we could disable these options

that wouldn't be true in the case of running a container which has mounted a virtual file system, which might be the case of DANDI servers! But then it would just be a matter of running Panel with the local_source=True in that case

@luiztauffer
Copy link
Collaborator Author

@bendichter

  • added S3 option
  • Panel can be instantiated now with boolean controls enable_dandi_source, enable_s3_source and enable_local_source, to control for source data options
  • DANDI option auto-lists all DANDI sets in a dropdown, and sows the description of the selected set
  • I improved the layout in other small details as well

What I'd also like to do related to this is adding to the documentation:

  • Instructions for these new Panel options
  • A session for running the container with Voila

but I can also add these in a separate PR later if you'd like to merge this as it is now

Peek 2022-10-24 21-34

docker/README.md Outdated Show resolved Hide resolved
docker/README.md Outdated Show resolved Hide resolved
@bendichter bendichter merged commit f555f4d into master Oct 27, 2022
@luiztauffer luiztauffer deleted the docker-voila branch October 27, 2022 17:20
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.

None yet

2 participants