throw nice error if directory doesn't exist #203

harsham4026 · 2018-09-12T05:04:42Z

@MSeal this addresses the issue of Nicer error when output directory doesn't exist #201
Please review the PR and let me know for any changes. At present I added the exception throwing for local file system. Please let me know if I need to do the same for others as well

codecov · 2018-09-12T06:24:08Z

Codecov Report

Merging #203 into master will increase coverage by 0.12%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #203      +/-   ##
=========================================
+ Coverage   77.38%   77.5%   +0.12%     
=========================================
  Files          11      11              
  Lines        1110    1116       +6     
=========================================
+ Hits          859     865       +6     
  Misses        251     251

harsham4026 · 2018-09-12T06:27:00Z

@MSeal please review

Could you please consider merging the changes?

betatim · 2018-09-12T07:38:31Z

papermill/iorw.py

@@ -88,6 +88,12 @@ def listdir(cls, path):

    @classmethod
    def write(cls, buf, path):
+        dir = (path.split("/"))[:-1]


I am not quite sure what the next few lines do but I think they try and find the directory name in the path. I'd recommend using https://docs.python.org/3/library/os.path.html#os.path.dirname or other tools from that module to do this.

By using an existing tool from the standard library you make it easier for others to read your code and we are less likely to have a bug in the code that manipulates the path.

@betatim Sure. Will make the change as you suggested

betatim · 2018-09-12T07:39:26Z

papermill/exceptions.py

@@ -13,6 +13,10 @@ class FileExistsError(AwsError):
    """Raised when a File already exists on S3."""


+class FileNotFoundError(AwsError):


I think we shouldn't inherit from AwsError here as the exception is only used for local files.

@betatim ok. Can I throw the normal Exception with the message that directory doesn't exists?

@harsham4026 There's already a built-in FileNotFoundError in Python. Let's use that here :)

betatim · 2018-09-12T07:45:23Z

Thanks for working on this! I left some feedback.

It would be super nice to add a test to make sure that the correct exception is raised when the directory does not exist and not raised when the directory is missing.

harsham4026 · 2018-09-12T08:47:17Z

@betatim Made the changes you suggested using the standard OS library for getting the directory path. And I removed the exception added in AwsError and raising the exception if dir doesn't exist. Can you please request to merge these changes to Master?

betatim · 2018-09-12T12:38:52Z

I should have been clearer with my comment about the exception: I would keep the custom exception but I would not have it inherit from AwsException and instead inherit from Exception.

betatim · 2018-09-12T13:44:35Z

papermill/iorw.py

+        if not dir.endswith("/"):
+            dir += "/"
+        if not os.path.exists(dir):
+            raise Exception('output folder {} doesn\'t exist!'.format(dir))


I'd rewrite this block as:

dir_name = os.path.dirname(path) if not os.path.exists(dir_name): raise NoSuchDirectory("The output folder {} does not exist.".format(dir_name))

and define a NoSuchDirectory exception

@betatim have you added this NoSuchDirectory exception or shall I add and commit the code?

@betatim added the code block and committed.

harsham4026 · 2018-09-12T14:19:29Z

@betatim yes I added a custom exception and inherited from Exception instead of AwsException.

MSeal

Thanks for making the contribution! A few minor notes here. As tim mentioned, a quick test would be nice to keep out coverage going up instead of down :)

MSeal · 2018-09-12T17:24:59Z

papermill/iorw.py

+        if not dir.endswith("/"):
+            dir += "/"
+        if not os.path.exists(dir):
+            raise NoSuchDirectory('output folder {} doesn\'t exist.'.format(dir))


Let's still use FileNotFoundError instead of making a new error class, as it's used for missing directories as well.

From Python docs:
| exception FileNotFoundError
| Raised when a file or directory is requested but doesn’t exist. Corresponds to errno ENOENT.

@MSeal used FileNotFoundError for throwing the error if a folder or file doesn't exist but the build is failing because FileNotFoundError is not directly supported in Python 2.7. Could you please let me know if we can proceed this way.

MSeal · 2018-09-12T17:25:21Z

papermill/iorw.py

@@ -88,6 +88,11 @@ def listdir(cls, path):

    @classmethod
    def write(cls, buf, path):
+        dir = os.path.dirname(path)
+        if not dir.endswith("/"):


I don't think you need this

@MSeal removed this piece of code

MSeal · 2018-09-12T17:26:06Z

papermill/iorw.py

@@ -88,6 +88,11 @@ def listdir(cls, path):

    @classmethod
    def write(cls, buf, path):
+        dir = os.path.dirname(path)


Since dir is a reserved keyword in python let's use a different name here.

@MSeal removed the dir and used os.path.dir(path)

harsham4026 · 2018-09-12T19:02:04Z

@MSeal made the changes you suggested but the build is failing as FileNotFoundError directly not available to use in Python 2.7

harsham4026 · 2018-09-12T19:32:42Z

@MSeal added the test case to check if it throws error when we give a folder which doesn't exist and it's passed. Please have a look into the following build
https://travis-ci.org/nteract/papermill/builds/427840680?utm_source=github_status&utm_medium=notification
Could you please consider having a review and merging it?

MSeal · 2018-09-12T19:47:57Z

@harsham4026 You can add

try:
    FileNotFoundError
except NameError:
    FileNotFoundError = IOError

in both the iorw.py and the test file to fix the test error for python 2 (this is the recommended pattern as FileNotFoundError is a child of IOError and IOError is what python 2 built-in libraries use for this issue. Sorry I forgot python 2 didn't have this error type when I mentioned it before.

MSeal · 2018-09-12T20:23:57Z

Once that's in place and the tests pass I'll happily hit merge :)

Apologies there that there were a lot of small asks on the PR here.

harsham4026 · 2018-09-13T02:23:23Z

@MSeal made the changes as you suggested and now the build is successful :)

Harsha Mandai added 4 commits September 12, 2018 10:29

throw nice error if directory doesn't exist

7590cee

fixing the CI of the new lines of code added

fe10e39

fixing the white space formatting issues

53c1b36

fixing the dir issues

92f2e9b

harsham4026 mentioned this pull request Sep 12, 2018

Nicer error when output directory doesn't exist #201

Closed

betatim approved these changes Sep 12, 2018

View reviewed changes

betatim reviewed Sep 12, 2018

View reviewed changes

changes for getting the dir name using standard library

dd63ddd

betatim reviewed Sep 12, 2018

View reviewed changes

Harsha Mandai added 2 commits September 12, 2018 19:37

added custom exception for NoSuchDirectory

60ce29d

adding new line at the end of the file

4853fd6

MSeal requested changes Sep 12, 2018

View reviewed changes

changes as requested

cf15d89

Harsha Mandai added 4 commits September 13, 2018 00:43

test case added for checking if directory doesn't exist

424cd8f

reformatting

4eb085b

test case fix

14e8d21

test case fixes

dae8ebd

fixes for importing the FileNotFoundError

3fb238b

MSeal approved these changes Sep 13, 2018

View reviewed changes

MSeal merged commit 8d6eeaf into nteract:master Sep 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

throw nice error if directory doesn't exist #203

throw nice error if directory doesn't exist #203

harsham4026 commented Sep 12, 2018

codecov bot commented Sep 12, 2018 •

edited

harsham4026 commented Sep 12, 2018 •

edited

betatim Sep 12, 2018

harsham4026 Sep 12, 2018

betatim Sep 12, 2018

harsham4026 Sep 12, 2018

MSeal Sep 12, 2018

betatim commented Sep 12, 2018

harsham4026 commented Sep 12, 2018 •

edited

betatim commented Sep 12, 2018

betatim Sep 12, 2018

harsham4026 Sep 12, 2018

harsham4026 Sep 12, 2018

harsham4026 commented Sep 12, 2018

MSeal left a comment

MSeal Sep 12, 2018

harsham4026 Sep 12, 2018

MSeal Sep 12, 2018

harsham4026 Sep 12, 2018

MSeal Sep 12, 2018 •

edited

harsham4026 Sep 12, 2018

harsham4026 commented Sep 12, 2018

harsham4026 commented Sep 12, 2018 •

edited

MSeal commented Sep 12, 2018

MSeal commented Sep 12, 2018

harsham4026 commented Sep 13, 2018

		@@ -13,6 +13,10 @@ class FileExistsError(AwsError):
		"""Raised when a File already exists on S3."""


		class FileNotFoundError(AwsError):

throw nice error if directory doesn't exist #203

throw nice error if directory doesn't exist #203

Conversation

harsham4026 commented Sep 12, 2018

codecov bot commented Sep 12, 2018 • edited

Codecov Report

harsham4026 commented Sep 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betatim commented Sep 12, 2018

harsham4026 commented Sep 12, 2018 • edited

betatim commented Sep 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harsham4026 commented Sep 12, 2018

MSeal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSeal Sep 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harsham4026 commented Sep 12, 2018

harsham4026 commented Sep 12, 2018 • edited

MSeal commented Sep 12, 2018

MSeal commented Sep 12, 2018

harsham4026 commented Sep 13, 2018

codecov bot commented Sep 12, 2018 •

edited

harsham4026 commented Sep 12, 2018 •

edited

harsham4026 commented Sep 12, 2018 •

edited

MSeal Sep 12, 2018 •

edited

harsham4026 commented Sep 12, 2018 •

edited