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

Enable users to copy both files and directories #1190

Merged
merged 13 commits into from
Feb 24, 2023

Conversation

kenyaachon
Copy link
Contributor

Objective

Enable users to copy both files and directories
Currently, if users try copying a directory, an error will thrown backing saying "Can't Copy directories".

jupyterlab

  • The copy option in the right click menu, was enabled for directories.

jupter_server

  • The copy directory functionality has been added to both the FileContentsManager and AsyncFileContentsManager
  • Users can copy folders up to 100 MB,
    • The actual limit for copying directories before a timeout occurs is 500MB, but without a loading indicator, if a user tries to copy a folder that is near 500MB, jupyterlab will seem like its frozen, since its not clear whether the copy operation is complete or in progress.
    • if we can add a loading indicator in the future to jupyterlab, then we would be able to increase the limit of copying folders to 500 MB

How to test

Create a virtual environment with venv (you can use what ever viritual environment tool you like)

python3 -m venv jupyter-connected
source jupyter-connected/bin/activate
pip install --upgrade pip
# install the jupyter_server package into the environment
git clone https://github.com/kenyaachon/jupyter_server.git
cd jupyter_server
git checkout copy-folder-functionality
pip install -e ".[test]"
# verify the dev version (i.e 2.1.0.dev0) of the package is in the environment
pip list | grep jupyter
# install the jupyterlab in development mode with latest changes
cd ..
git clone https://github.com/kenyaachon/jupyterlab.git
cd jupyterlab
git checkout develop
pip install -e .
jlpm install
jlpm run build 
jupyter lab --dev-mode

Limitations

  • We were not able to find a suitable solution yet for being able to copy folders above 500MB in size without having a timeout issue

  • Users, cannot easily know if the folder they are copying is above 100MB before they try and copy

    • We did attempt in adding the folder size to the preview info that users get when hovering over a file or folder. But this ended up making the filebrowser-extension very slow when loading the correct directory. Since it would have to recalculate the size of the folders in the given directory each time a new path was loaded.

@welcome
Copy link

welcome bot commented Jan 30, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@codecov
Copy link

codecov bot commented Jan 30, 2023

Codecov Report

Base: 80.45% // Head: 80.28% // Decreases project coverage by -0.17% ⚠️

Coverage data is based on head (395842b) compared to base (903942c).
Patch coverage: 82.90% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1190      +/-   ##
==========================================
- Coverage   80.45%   80.28%   -0.17%     
==========================================
  Files          68       68              
  Lines        8135     8249     +114     
  Branches     1586     1602      +16     
==========================================
+ Hits         6545     6623      +78     
- Misses       1176     1202      +26     
- Partials      414      424      +10     
Impacted Files Coverage Δ
jupyter_server/services/contents/filemanager.py 76.93% <82.75%> (+1.18%) ⬆️
jupyter_server/services/contents/manager.py 82.99% <100.00%> (-2.27%) ⬇️
jupyter_server/extension/handler.py 67.69% <0.00%> (-1.54%) ⬇️
...ter_server/services/kernels/connection/channels.py 61.06% <0.00%> (-0.67%) ⬇️
jupyter_server/gateway/managers.py 83.37% <0.00%> (-0.24%) ⬇️
jupyter_server/serverapp.py 79.96% <0.00%> (-0.09%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@blink1073
Copy link
Contributor

Hi @kenyaachon, thank you for working on this! Where are you seeing a timeout, on the server or the browser?

@kenyaachon
Copy link
Contributor Author

kenyaachon commented Jan 30, 2023

Hi @kenyaachon, thank you for working on this! Where are you seeing a timeout, on the server or the browser?

Hi @blink1073 I was seeing the timeout on the server. Let me reproduce a screenshot of the error.

@kenyaachon
Copy link
Contributor Author

kenyaachon commented Feb 3, 2023

@blink1073 Here is the timeout error that occurs,
Copy-Folder Timeout

It seems with the latest version of the jupyter-server, this error no longer occurs, in the other project that I developed this code from, we are using version 1.18.0 which had that limitation. The timeout error would occur when copying folders around 500MB, but it seems now I can copy folders around >1GB. It seems it would be fine to remove the code around limiting how big of a folder can be copied, but I am not sure if that might cause problems on less powerful machines.

@blink1073
Copy link
Contributor

We can expose this as a setting, a new trait on ContentsManager like (max_copy_folder_size_mb), and then whoever is deploying the server can make that determination. 500MB seems like a reasonable default.

@kenyaachon kenyaachon force-pushed the copy-folder-functionality branch from 7afa176 to aa8b252 Compare February 10, 2023 00:45
@kenyaachon kenyaachon force-pushed the copy-folder-functionality branch from d5c0471 to d7520ed Compare February 10, 2023 03:24
@blink1073
Copy link
Contributor

@kenyaachon, can you please add that new trait as part of this PR?

@kenyaachon
Copy link
Contributor Author

@kenyaachon, can you please add that new trait as part of this PR?

I will make sure to add the trait as part of the review, once I am done addressing the failing tests.

@blink1073
Copy link
Contributor

Great, thanks! The failing tests are unrelated to this PR.

@@ -121,6 +133,8 @@ def _notary_default(self):
""",
)

max_copy_folder_size_mb = Int(500, config=True, help="The max folder size that can be copied")
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, perhaps this trait should be on FileManager, since it isn't implemented in the more generic ContentsManager. Anyone have any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Yes - I completely agree - good catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will make sure to make that change @blink1073.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@blink1073, when you say FileManager do you mean the FileContentsManager or the LargeFileManager?

Copy link
Member

Choose a reason for hiding this comment

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

Hi @kenyaachon - yes, FileContentsManager is the appropriate location for the trait since it's the "most base" class for the file-based contents managers. Sorry for the confusion.

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@blink1073 blink1073 merged commit bc828c5 into jupyter-server:main Feb 24, 2023
@welcome
Copy link

welcome bot commented Feb 24, 2023

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@vidartf
Copy link
Member

vidartf commented Mar 3, 2023

Most windows installs do not have du command, so the _get_dir_size method fails. Any chance that this can be fixed? Or at least that we disable the test on Windows? (or maybe even a check whether du is available as a callable, and skips the test if not?)

@blink1073
Copy link
Contributor

Yeah, I think checking for which("du") makes sense, PR welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants