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

Support environments without a home dir or writable file system #1824

Merged
merged 17 commits into from Apr 16, 2013

Conversation

mgiuca-google
Copy link
Contributor

This patch makes it possible to import and use matplotlib on a system where the user has no $HOME directory, and furthermore, on which there is no writable file system. It does this by:

  • Falling back to creating a temporary config directory instead of raising RuntimeError if the user has no $HOME directory.
  • Falling back to not writing to the config dir if it is not possible to create a temporary directory. (In this case, all caching mechanisms, including texmanager, font_manager and finance, are disabled.)

Partial fix for Issue #1823.

t.write(ascii('1'))
finally:
t.close()
return os.access(p, os.W_OK)
except OSError: return False
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind separating this line into 2.

Copy link
Member

Choose a reason for hiding this comment

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

I believe there is a corner case in which os.access will not do the right thing, hence the convoluted approach you see here... just navigated the git history to no avail. At the very least, this should use realpath of p to resolve any symbolic links.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pelson: Done.

@mdboom: Yeah the code was written in 2005 and wasn't really changed since. I assume the corner case is what the Python manual for os.access hints at: "Note: I/O operations may fail even when access() indicates that they would succeed, particularly for operations on network filesystems which may have permissions semantics beyond the usual POSIX permission-bit model."

I don't know what this means in practice. Is there a concrete example of access() returning True for a directory but you can't create files in it? Creating and deleting a temporary file seems like a messy thing to do if there is a function that tells you directly whether a directory is writable. But I am happy to put it back. The App Engine corner case is a failure in the other direction: TemporaryFile succeeds even though the file system is not writable (since TemporaryFile just creates a StringIO, ignoring the dir argument). So it should be fine to have both checks: os.access() to check if it is theoretically writable (which works for App Engine), followed up by TemporaryFile to test if it is writable in practice. Should I go ahead and do this?

At the very least, this should use realpath of p to resolve any symbolic links.

I don't think so. From man 2 access:

If pathname is a symbolic link, it is dereferenced.

I tested this on Linux and it seemed to work (created a non-writable directory and a writable symlink to the directory; os.access with W_OK on the symlink returns False).

Copy link
Member

Choose a reason for hiding this comment

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

I think doing both (as you suggest) sounds like a good suggestion.

On Sun, Mar 17, 2013 at 7:50 PM, Matt Giuca notifications@github.comwrote:

In lib/matplotlib/init.py:

@@ -215,13 +215,8 @@ def _is_writable_dir(p):
try: p + '' # test is string like
except TypeError: return False
try:

  •    t = tempfile.TemporaryFile(dir=p)
    
  •    try:
    
  •        t.write(ascii('1'))
    
  •    finally:
    
  •        t.close()
    
  •    return os.access(p, os.W_OK)
    
    except OSError: return False

@pelson https://github.com/pelson: Done.

@mdboom https://github.com/mdboom: Yeah the code was written in 2005
and wasn't really changed since. I assume the corner case is what the
Python manual for os.access hints at: "Note: I/O operations may fail even
when access() indicates that they would succeed, particularly for
operations on network filesystems which may have permissions semantics
beyond the usual POSIX permission-bit model."

I don't know what this means in practice. Is there a concrete example of
access() returning True for a directory but you can't create files in it?
Creating and deleting a temporary file seems like a messy thing to do if
there is a function that tells you directly whether a directory is
writable. But I am happy to put it back. The App Engine corner case is a
failure in the other direction: TemporaryFile succeeds even though the file
system is not writable (since TemporaryFile just creates a StringIO,
ignoring the dir argument). So it should be fine to have both checks:
os.access() to check if it is theoretically writable (which works for App
Engine), followed up by TemporaryFile to test if it is writable in
practice. Should I go ahead and do this?

At the very least, this should use realpath of p to resolve any symbolic
links.

I don't think so. From man 2 access:

If pathname is a symbolic link, it is dereferenced.

I tested this on Linux and it seemed to work (created a non-writable
directory and a writable symlink to the directory; os.access with W_OK on
the symlink returns False).


Reply to this email directly or view it on GitHubhttps://github.com//pull/1824/files#r3406610
.

Michael Droettboom
http://www.droettboom.com/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Also I added an extra check that it is actually a directory!

@pelson
Copy link
Member

pelson commented Mar 15, 2013

Thanks @mgiuca-google - this looks good to me. Thanks for putting this work in.

I'm guessing you've tried this out on G.A.E.? That would be the best way to give me confidence that this is doing the right thing for those circumstances.

@ghost ghost assigned pelson Mar 15, 2013
@pelson
Copy link
Member

pelson commented Mar 15, 2013

Also, would you mind adding details in the whats new section (making a sing and a dance that Google app engine should now work). If you're keen I'd also love to see a write-up at http://matplotlib.org/faq/howto_faq.html#matplotlib-in-a-web-application-server about how to go about setting up your App engine instance for mpl?

if not self.texcache:
raise RuntimeError(
('Cannot create TexManager, as there is no cache directory '
'available'))
Copy link
Member

Choose a reason for hiding this comment

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

This probably doesn't affect Google AppEngine (which I suspect doesn't have a TeX installation anyway), but it would be nice to continue to be able to get TeX snippets without them being cached in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my reading of the code, this whole module doesn't work if there is no writable directory. The so-called "texcache" is not really a cache at all, it's also a working directory, where intermediate files are generated and processed with dvipng. So even if we assume a system that does have a TeX installation and the ability to launch external commands, but does not have any writable file system, we won't be able to run these commands. (This isn't a simple matter of disabling caching.)

Correct me if I'm wrong about that.

@mdboom
Copy link
Member

mdboom commented Mar 15, 2013

For the font cache: ideally if .matplotlib is not writable, it should fall back to a font cache created at install time and installed in mpl-data/fonts -- that way the user would still get fast access to the fonts that ship with matplotlib. I don't know if we need that level of finesse in this first round of PR, but it would certainly deal with 90% of use cases and have them be as fast as ever.

@mgiuca-google
Copy link
Contributor Author

@pelson:

Thanks @mgiuca-google - this looks good to me. Thanks for putting this work in.

No problem, thanks for looking at these GAE-specific patches. This should make our (GAE Python team's) lives easier in the future.

I'm guessing you've tried this out on G.A.E.? That would be the best way to give me confidence that this is doing the right thing for those circumstances.

Yeah (well, the dev appserver). I basically wrote this patch by running it on the dev appserver and then fixing things until it worked. This, combined with my other two PRs (#1825 and #1826), allows Matplotlib to run on the dev appserver. There is a caveat, though: because of the recent change to Matplotlib to remove pytz and dateutil, and require them as external dependencies, I had to hack the devappserver code to whitelist those modules. I think that is something we'll have to fix when the time comes to upgrade to Matplotlib 1.3.0, not something that you can fix on your end.

Also, would you mind adding details in the whats new section (making a sing and a dance that Google app engine should now work). If you're keen I'd also love to see a write-up at http://matplotlib.org/faq/howto_faq.html#matplotlib-in-a-web-application-server about how to go about setting up your App engine instance for mpl?

Yeah, I'd be happy to write a short "matplotlib with Google App Engine" section there. The general advice in that section is pretty much all you need for App Engine (though obviously you can't use fig.savefig('test.png'); you need to write to stdout instead). I'll address that in a separate PR. Note that Matplotlib already runs on the production servers (since I have already applied similar patches to the above on our local version); this PR just allows it to run on the dev appserver.

@mdboom:

For the font cache: ideally if .matplotlib is not writable, it should fall back to a font cache created at install time and installed in mpl-data/fonts -- that way the user would still get fast access to the fonts that ship with matplotlib. I don't know if we need that level of finesse in this first round of PR, but it would certainly deal with 90% of use cases and have them be as fast as ever.

But as far as I can tell, the font cache is not actually caching the font files, it's caching the font list. I don't think that mpl-data/fonts contains a font list file, does it? (It doesn't on my system.)

@mdboom
Copy link
Member

mdboom commented Mar 18, 2013

Yes -- the fontList.cache is just a directory of the font attributes to font paths. I was suggesting that we generate a font directory at build time of only the fonts that ship with matplotlib, and use that as a fallback if we can't write a font directory at import time.

@mgiuca-google
Copy link
Contributor Author

Yes -- the fontList.cache is just a directory of the font attributes to font paths. I was suggesting that we generate a font directory at build time of only the fonts that ship with matplotlib, and use that as a fallback if we can't write a font directory at import time.

OK. Well can we do that in another PR? This PR is not going to make things worse on this front (since all of these cases are going from "won't work at all" to "won't have access to the cache".

@mdboom
Copy link
Member

mdboom commented Mar 19, 2013

Right -- I was suggesting that having a default cache could be another PR.

@mgiuca-google
Copy link
Contributor Author

OK. I've rebased the branch to the current master.

@dmcdougall
Copy link
Member

@mgiuca-google Looks like you need another rebase. That might be my fault; I just firehosed a bunch of PRs.

@mgiuca-google
Copy link
Contributor Author

Rebased.

(It was my fault: I conflicted with my own change in another branch!)

This is a more direct approach and avoids strange behaviour on systems that
provide a fake implementation of TemporaryFile. In particular, on Google App
Engine, TemporaryFile is equivalent to StringIO, so this would have always
succeeded, despite the fact that the directory is not actually writable.

It is also not particularly nice to go creating and deleting random files in the
user's home directory.
matplotlib.get_home now returns None if there is no home directory, instead of
raising RuntimeError. Updated code in several places to handle this gracefully.

matplotlib.get_configdir now returns a temporary directory if there is no home
directory, instead of raising RuntimeError.
Previously, it would raise NotImplementedError. This is necessary on restricted
platforms such as Google App Engine that do not provide gettempdir.
Each case is either handled gracefully, or raises a RuntimeError.
Instead of raising RuntimeError, now avoids reading and writing the font cache.
…pon import.

Instead of raising RuntimeError, now avoids creating the cache dir. This permits
texmanager to be imported in a restricted environment such as Google App Engine.

Actually constructing a TexManager object will fail, but it can be imported.
Instead of raising RuntimeError, now avoids reading and writing the cache.
Previously, it would not cache if there was no cachedir, even if cachename was
supplied.
os.access does not raise this exception.
This fails either if the OS tells us it isn't writable, or if a file cannot be
created there.
@mgiuca-google
Copy link
Contributor Author

Hey guys, we haven't had any traction on this issue for about a month. I thought I'd wait until the 1.2.1 release before pinging.

It looked like we were almost at a consensus. Are there any more issues? I have just rebased again against the latest HEAD, and there were no conflicts.

pelson added a commit that referenced this pull request Apr 16, 2013
Support environments without a home dir or writable file system
@pelson pelson merged commit 91b1d9f into matplotlib:master Apr 16, 2013
@pelson
Copy link
Member

pelson commented Apr 16, 2013

Done. Another excellent piece of work @mgiuca-google - thanks again!

@mgiuca-google mgiuca-google deleted the no-home-dir branch April 16, 2013 08:39
@mgiuca-google
Copy link
Contributor Author

Thanks, Phil. That's all the patches I have lined up for now. It's been great working with you.

@pelson
Copy link
Member

pelson commented Apr 16, 2013

That's all the patches I have lined up for now. It's been great working with you.

Shame! Has this been a work-driven or a hobby-driven project? We've plenty of space for people willing to get their hands dirty improving the codebase 😉 .

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

5 participants