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

Remove Redundant Dir_Exists Invocation When Creating New Files with ContentsManager #720

Merged

Conversation

jhamet93
Copy link
Contributor

Removes the redundant ContentsManager.dir_exists calls when creating a new file. Maintains current behavior in other known usages of ContentsManager.new_untitled.

Issue: #719

@welcome
Copy link

welcome bot commented Mar 12, 2022

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! 🎉

@blink1073
Copy link
Contributor

This one needs a rebase now after merging #721

@kevin-bates
Copy link
Member

Hi @jhamet93 - I totally agree with the premise and removal of extraneous dir_exists calls make sense - nice catch!

I'm wondering if we should instead remove the dir_exists call in the handlers and retain those in the target methods (e.g., new_untitled()). My concern is that server extensions (that don't go through the handlers) will lose the existence checks. In addition, the handler _copy() method will call copy() on the manager and still encounter redundant directory existence checks. This same issue may be present "save" and "upload", although I'm not finding an internal dir_exists in the "save" flow (so one would be needed). That said, it feels like keeping the directory existence checks in the manager classes (and perhaps adding checks to those methods that currently rely on the handler's dir_exist) would be less likely to break those "clients" (like server extensions) that don't go through handlers.

@jhamet93
Copy link
Contributor Author

Hi @jhamet93 - I totally agree with the premise and removal of extraneous dir_exists calls make sense - nice catch!

I'm wondering if we should instead remove the dir_exists call in the handlers and retain those in the target methods (e.g., new_untitled()). My concern is that server extensions (that don't go through the handlers) will lose the existence checks. In addition, the handler _copy() method will call copy() on the manager and still encounter redundant directory existence checks. This same issue may be present "save" and "upload", although I'm not finding an internal dir_exists in the "save" flow (so one would be needed). That said, it feels like keeping the directory existence checks in the manager classes (and perhaps adding checks to those methods that currently rely on the handler's dir_exist) would be less likely to break those "clients" (like server extensions) that don't go through handlers.

I agree! That was my other proposed solution in the issue after I implemented this that makes more sense like you mention. Will take a look at making the mods there.

kevin-bates
kevin-bates previously approved these changes Mar 14, 2022
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

These changes look good to me Josh - thank you. Could you please merge in the main branch to resolve the conflicts? Also, I think filemanager.py should probably have a newline in its last character (which probably triggers the conflict 😄 ).

@blink1073
Copy link
Contributor

Kicking CI

@blink1073 blink1073 closed this Mar 15, 2022
@blink1073 blink1073 reopened this Mar 15, 2022
@blink1073
Copy link
Contributor

The pre-commit failure is unrelated and addressed in #736

@blink1073
Copy link
Contributor

Ah, it looks like there are some relevant test failures:

 tests/services/contents/test_manager.py::test_copy[jp_contents_manager2]
tests/services/contents/test_manager.py::test_copy[jp_contents_manager2]
tests/services/contents/test_manager.py::test_copy[jp_contents_manager3]
tests/services/contents/test_manager.py::test_copy[jp_contents_manager3]

@kevin-bates kevin-bates self-requested a review March 15, 2022 21:18
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

Thanks for pointing out the real test failures @blink1073.

Per my comment, this presents an interesting issue (hassle) in order to preserve parity with the REST API. I haven't looked at the various utility methods, but I think we'll need to distinguish a directory segment of a path from a filename segment of a path to do that.

if to_path is not None:
to_path = to_path.strip("/")
dir_exists = self.dir_exists(to_path)
if not dir_exists:
Copy link
Member

Choose a reason for hiding this comment

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

We should probably leverage the self.dir_exists(to_path) at L483 below to avoid a redundant dir-exists check.

It's interesting, but the REST API appears to only support "copy" into a directory (typically the same directory as the source), whereas the ContentsManager also supports copy to a file, which is why the tests are failing (due to the first dir_exists() call since to_path represents a file).

It seems like the case that's missing here (even before this PR) is when to_path (being either a directory or file) references a non-existent directory. The way things stand now, this will result in a 500 error similar to the following (reproduced by changing a character in the parent directory name from 'b' to 'x' in the test_copy test):

tornado.web.HTTPError: HTTP 500: Internal Server Error (Unexpected error while saving file: å x/copy 2.ipynb [Errno 2] No such file or directory: '/private/var/folders/4w/qxgsbhyx1y93qlk9mnzwjryc0000gn/T/pytest-of-kbates/pytest-113/test_copy_jp_contents_manager30/å x/copy 2.ipynb')

jupyter_server/services/contents/filemanager.py:806: HTTPError

In order to preserve parity with the handler (where to_path can only be a directory and a 404 is returned if it doesn't exist), I think some additional logic is required to check if to_path is only a directory name or a combination of directory/file and ensure the respective directory portion exists (throwing 404 if it does not).

(Same would be needed in the async CM as well.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good eye. Made the modifications and tried to preserve the parity you mentioned with a few test cases to verify.

@codecov-commenter
Copy link

codecov-commenter commented Mar 26, 2022

Codecov Report

Merging #720 (1e3b4b7) into main (e0c126f) will decrease coverage by 0.54%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##             main     #720      +/-   ##
==========================================
- Coverage   70.80%   70.25%   -0.55%     
==========================================
  Files          62       62              
  Lines        7627     7497     -130     
  Branches     1220     1226       +6     
==========================================
- Hits         5400     5267     -133     
- Misses       1850     1856       +6     
+ Partials      377      374       -3     
Impacted Files Coverage Δ
jupyter_server/services/contents/handlers.py 85.46% <ø> (+0.54%) ⬆️
jupyter_server/services/contents/manager.py 82.72% <88.23%> (-0.94%) ⬇️
jupyter_server/services/kernels/kernelmanager.py 79.66% <0.00%> (-2.88%) ⬇️
jupyter_server/gateway/handlers.py 31.87% <0.00%> (-2.07%) ⬇️
jupyter_server/services/shutdown.py 76.92% <0.00%> (-1.65%) ⬇️
jupyter_server/auth/decorator.py 81.25% <0.00%> (-1.61%) ⬇️
jupyter_server/nbconvert/handlers.py 27.27% <0.00%> (-1.30%) ⬇️
jupyter_server/extension/application.py 72.39% <0.00%> (-1.09%) ⬇️
jupyter_server/gateway/gateway_client.py 75.27% <0.00%> (-0.92%) ⬇️
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0c126f...1e3b4b7. Read the comment docs.

@Zsailer
Copy link
Member

Zsailer commented Mar 29, 2022

Going to close and reopen to trigger the pre-commit bot.

@Zsailer Zsailer closed this Mar 29, 2022
@Zsailer Zsailer reopened this Mar 29, 2022
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

LGTM - thank you @jhamet93. I apologize for the delay in getting back to this.

@Zsailer
Copy link
Member

Zsailer commented Mar 29, 2022

Thanks, @jhamet93. Merging!

@Zsailer Zsailer merged commit 57c0676 into jupyter-server:main Mar 29, 2022
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.

5 participants