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

Creates tempfile when output_filename is not writable (notebook) #5942

Merged
merged 5 commits into from Mar 3, 2017

Conversation

@jsignell
Copy link
Contributor

jsignell commented Mar 1, 2017

  • issues: fixes #5677
  • tests added / passed
  • release document entry (if new feature or API change)
@bryevdv
Copy link
Member

bryevdv commented Mar 2, 2017

Several examples in python 2 seem to be reliably failing on Travis examples tests with e.g.

tests/examples/test_examples.py::test_file_examples[/home/travis/build/bokeh/bokeh/examples/plotting/file/bar_chart.py-example58] 
[INFO] Example run in 1.245s
[INFO] Example rendered in 0.352s
[JS] Error opening /home/travis/build/bokeh/bokeh/examples/plotting/file/bar_chart.html: No such file or directory
FAILED

I note that, at least for that bar chart example, the script does not call output_file:

https://github.com/bokeh/bokeh/blob/master/examples/plotting/file/bar_chart.py

I think that is a mistake (I think all examples should set a filename explicitly), but it also seems to show some issue with this change and python 2?

@mattpap
Copy link
Contributor

mattpap commented Mar 2, 2017

it also seems to show some issue with this change and python 2

phantomjs part of examples testing is done only on 2.7, so it doesn't work at all (presumably, I didn't look at the code).

@mattpap mattpap added the status: WIP label Mar 2, 2017
bokeh/io.py Outdated
@@ -413,6 +414,8 @@ def _get_save_args(state, filename, resources, title):
if filename is None:
warn = False
filename = _detect_filename("html")
if not os.access(filename, os.W_OK):

This comment has been minimized.

@mattpap

mattpap Mar 2, 2017 Contributor

_detect_filename() can return None, indicating that detection failed, so os.access() check should be moved there.

This comment has been minimized.

@mattpap

mattpap Mar 2, 2017 Contributor

os.access("non_existent_filename", os.W_OK) returns False, so you have to proceed with the solution proposed in the issue, i.e. check W_OK | X_OK for the base directory.

This comment has been minimized.

@jsignell

jsignell Mar 2, 2017 Author Contributor

Shouldn't we just be checking W_OK be on the base directory? Why should X_OK allow a pass?

This comment has been minimized.

@bryevdv

bryevdv Mar 2, 2017 Member

agree with the suggestion to move to _detect_filename

This comment has been minimized.

@mattpap

mattpap Mar 2, 2017 Contributor

You need X_OK (executable bit) on a directory to make it "useful". Try chmod -x on_some_directory and then touch on_some_directory/whatever and see what happens.

This comment has been minimized.

@bryevdv

bryevdv Mar 2, 2017 Member

@jsignell the syntax is a bit confusing, W_OK | X_OK is bitwise-or'ing two flags together. It's actually stating the directory has to be both writeable and "executable" (i.e. listable, for a dir) .

bokeh/io.py Outdated
return tempfile.NamedTemporaryFile().name

elif not os.access(dirname(filename) or curdir, os.W_OK | os.X_OK):
return tempfile.NamedTemporaryFile().name

This comment has been minimized.

@bryevdv

bryevdv Mar 3, 2017 Member

I think this usage of or might be surprising to many python programmers. But in any case, why the fallback check to curdir? The branch below only ever uses dirname(filename) so the accessibility of curdir does not seem relevant? Maybe you mean to fall back to curdir in the branch below. I'm ambivalent on that and it will complicate the code a fair bit.

As a personal preference, I don't use elif if all the branches return (just use if)

This comment has been minimized.

@jsignell

jsignell Mar 3, 2017 Author Contributor

If you are in the folder with the file and run the file from there then dirname(filename) evaluates to "" which then fails the writeability check. But really what you want is to check the writeability of the current directory.

bokeh/io.py Outdated
elif not os.access(dirname(filename) or curdir, os.W_OK | os.X_OK):
return tempfile.NamedTemporaryFile().name

elif isfile(filename):
name, _ = splitext(basename(filename))
return join(dirname(filename), name + "." + ext)

This comment has been minimized.

@bryevdv

bryevdv Mar 3, 2017 Member

If we've gotten here the directory is accessible [*] but I wonder if it is also worth checking whether the final filename exists and is not writable. Maybe the standard exception that python will raise if trying to write to an unwritable file is actually the clearest thing, though.

[*] Not exactly, it's not impossible that the directory becomes inaccessible between when we check the dir , and here, or when the save happens, but this is probably fine in practice.

bokeh/io.py Outdated
if filename and isfile(filename):

if filename is None:
return tempfile.NamedTemporaryFile().name

This comment has been minimized.

@bryevdv

bryevdv Mar 3, 2017 Member

Can NamedTemporaryFile ever fail? If so maybe wrap in try/except and return None if it fails.

This comment has been minimized.

@bryevdv

bryevdv Mar 3, 2017 Member

Otherwise, if we just want to let any exceptions propagate, the the lines that check for None from this function are no longer needed.

bokeh/io.py Outdated
@@ -393,14 +394,21 @@ def _detect_filename(ext):
None if the script could not be found (e.g. interactive mode).
"""

This comment has been minimized.

@bryevdv

bryevdv Mar 3, 2017 Member

This docstring can be updated/clarified.

bokeh/io.py Outdated
directory = dirname(filename) or curdir

if not os.access(directory, os.W_OK | os.X_OK):
return tempfile.NamedTemporaryFile().name

This comment has been minimized.

@mattpap

mattpap Mar 3, 2017 Contributor

This redundancy has to be removed. Also make sure that all code paths given a path with the same extension.

This comment has been minimized.

@jsignell

jsignell Mar 3, 2017 Author Contributor

Which redundancy? The tempfile.NamedTemporaryFile().name? Having filename is None or os.access(directory, os.W_OK | os.X_OK) seemed less legible, but is that preferred?

This comment has been minimized.

@bryevdv

bryevdv Mar 3, 2017 Member

I'm OK with either but I do agree that the tmp file should have the requested ext in all cases

bokeh/io.py Outdated
if filename is None:
return tempfile.NamedTemporaryFile().name

# dirname(filename) returns empty str if file called from within directory

This comment has been minimized.

@mattpap

mattpap Mar 3, 2017 Contributor

This shouldn't be unexpected outcome, thus the later or curdir doesn't seem necessary, unless you're worried that between path = _detect_filename() and using path current directory changes.

This comment has been minimized.

@jsignell

jsignell Mar 3, 2017 Author Contributor

I am just trying to check the wherever the file will get written to that dir is writeable. So if dirname(filename) returns empty string then we need to check that the current directory is writeable.

This comment has been minimized.

@bryevdv

bryevdv Mar 3, 2017 Member

I can confirm that that this function returns "foo.py" if run e.g as python foo.py and "somedir/foo.py" if run as python somedir/foo.py in which in the former case the dirname ends up returning the empty string. I can also confirm the os.access does not interpret the empty string as the current dir:

In [9]: os.access(curdir, os.W_OK | os.X_OK)
Out[9]: True

In [10]: os.access("", os.W_OK | os.X_OK)
Out[10]: False

So I think testing against curdir makes sense.

@bryevdv bryevdv added status: accepted and removed status: WIP labels Mar 3, 2017
@bryevdv
Copy link
Member

bryevdv commented Mar 3, 2017

This LGTM. It's probably worth adding a migration note about this (just trying to develop good fastidious habits around reporting things that change) This is the file:

https://github.com/bokeh/bokeh/blob/master/sphinx/source/docs/releases/0.12.5.rst

A new section and short sentence or two under "Migration Guide" Once that's there I think we can merge!

@bryevdv bryevdv merged commit f766a37 into bokeh:master Mar 3, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.