Delay the evaluation of SAGE_TMP in order to facilitate forking in doctesting

[roed]

In order to use multiprocessing in the new doctesting framework (#12415), sage's infrastructure for dealing with temporary files needs to change.

Apply

• trac_13147-rebased-to-13681.patch​.

[roed]

David Roe/Robert? Bradshaw's original solution: I don't like it as much as Mike's

[roed]

I like Mike's solution a lot more, but it requires speaklater. I think this is in the new flasque notebook? Can we use it from there?

[ohanar]

+1 for Mike's solution, I know nothing about the flask notebook, but we certainly shouldn't include speaklater in the manner it is done in the patch.

Also, why are we using temp files and directories in DOT_SAGE still, I see no reason to not use the tempfile module with Mike's patch.

[jhpalmieri]

While you're rewriting the definition of SAGE_TMP, you should change lines like

d = '%s/temp/%s/%s/'%(DOT_SAGE, HOSTNAME, os.getpid()) 

to use os.path.join.

[roed]

Mike Hansen's solution

[kini]

Andrew, are you referring to the fact that Mike creates speaklater.py in the Sage library?

[roed]

Yeah, I think Andrew's referring to the fact that Mike just included speaklater.

patchbot: apply 13147_lazy.patch.

[kini]

patchbot: apply 13147_lazy.patch

[jdemeyer]

sage/misc/speaklater.py has no documentation at all.

Why the name speaklater and not lazy_string (or whatever)?

[kini]

Because it's a file ripped directly from the package ​speaklater. Obviously we should just depend on speaklater and not incorporate it into the Sage library.

[jdemeyer]

Replying to kini:

Because it's a file ripped directly from the package ​speaklater.

At the very least, document this fact in the file speaklater.py. Also, you should credit the author and specify the license.

[mhansen]

When I wrote that patch, including the speaklater code was just something to get things as a proof of concept. I didn't think that that code would ever try to go in as is.

[jhpalmieri]

Isn't speaklater included with the new notebook? So this ticket could depend on #11080 and then it would not include a copy of speaklater.py.

[jdemeyer]

Replying to jhpalmieri:

Isn't speaklater included with the new notebook? So this ticket could depend on #11080 and then it would not include a copy of speaklater.py.

I think it's philosophically wrong to make Sage depend on the Sage Notebook. Since it's a small file, I don't mind the duplication much.

[jhpalmieri]

I think it's philosophically wrong for the Sage notebook to depend on Sage: that's the whole point of the notebook project. But Sage can and should depend on its various component packages, so I don't see anything wrong with Sage depending on files included in the sagenb spkg. But maybe I'm misunderstanding your point.

[jdemeyer]

Okay, I think it's wrong to depend on the Sage Notebook for stuff which isn't notebook-related.

I also don't think that speaklater should be included in the sagenb spkg. Technically, it probably should be a separate spkg, but that seems way overkill given that it's essentially one small file.

[kini]

I don't think the sagenb spkg should contain a bunch of tarballs either. I think we should start using the package management system from lmonade and steal Gentoo's ebuilds for most of our packages, so there will be very minimal effort required to package additional pieces of software (such as speaklater) for Sage. But that is far off, I guess. At least it will need to be after we drop SPKGs and complete #13015.

[roed]

So what's the plan for this ticket? The options seem to be:

1. Include speaklater.py with no doctests; include attribution and license.
2. Include speaklater.py with doctests (I'm not going to write them...); include attribution and license.
3. Depend on #11080 and import speaklater from sagenb
4. Make an spkg.
5. Write a simplified lazy string with doctests.

I think my vote would be for 3.

[jdemeyer]

I vote for 1.

[kini]

I vote for 4.

[kini]

Here's an SPKG: ​http://wstein.org/home/keshav/files/speaklater-1.2-fbc7a37.spkg

[ohanar]

I also vote for the spkg. The one Keshav provided looks fine.

[ohanar]

Lazy string solution for use with spkg

[kini]

We should also patch deps since this would become a standard SPKG.

[roed]

We also need a vote on sage-devel for the inclusion of speaklater as a standard spkg. I'm a bit concerned that we'll get objections since

• speaklater has no doctests
• we haven't made it an optional package first
• we're adding another dependency of sage

But I'm happy to be proved wrong. Keshav and Andrew, do one of you want to write an e-mail to sage-devel or should I?

[jhpalmieri]

• Upstream source is not required to have doctests.
• speaklater was already included in the flask notebook, so we're really just moving it into its own spkg.

So I don't think it should be a big deal.

[kini]

Replying to roed:

• speaklater has no doctests

Not required, as John said.

• we haven't made it an optional package first

Not really a big deal I think

• we're adding another dependency of sage

No we're not - we're including another package in Sage.

By the way, I asked the author of speaklater on IRC to release a new version containing the fix for the pickling problem we ran across in sagenb many months ago, and a couple of days ago he seems to have released 1.3, so here's a new SPKG: ​http://wstein.org/home/keshav/files/speaklater-1.3.spkg

[kini]

Oops, forgot to update SPKG.txt - done now (same URL).

[roed]

Given the opposition to speaklater as an spkg on sage-devel, I've added doctests to speaklater.py, make a couple changes and added it as sage.misc.lazy_string.

patchbot: apply 13147_new.patch

[roed]

Missed something; I'll fix it in a few hours.

[roed]

Alright, fixed.

[jhpalmieri]

Line 23 of cleaner.py:

    F = os.path.join(misc.SAGE_TMP, 'spawned_processes') 

Should it say str(SAGE_TMP)?

[jhpalmieri]

I think that the file lazy_string.py should have a bit more documentation at the top, for example (from the ​speaklater website:

A module that provides lazy strings for translations. Basically you get an
object that appears to be a string but changes the value every time the value
is evaluated based on a callable you provide.

A few examples would be nice, too.

(Right now it looks like a typical, underdocumented, Python module (well, it's much better than this because of the doctests you added), whereas it should look like a typical, well-documented, Sage module.)

[kini]

All gettext stuff has been ripped out, so that exact text is no longer appropriate. Also I see several examples in the docstrings. What did you have in mind? Or did you mean a module-level docstring with one large example set walking you how to use all the functionality of the module?

[roed]

Replying to jhpalmieri:

Line 23 of cleaner.py:

    F = os.path.join(misc.SAGE_TMP, 'spawned_processes') 

Should it say str(SAGE_TMP)?

There was a typo in Mike's version of speaklater.py (__eq__ was broken); os.path.join now works without the explicit string cast.

os.path.exists doesn't though (and I'm not sure exactly why, since I can't find the source code of os.stat).

[jhpalmieri]

Keshav: I'm thinking that when I read the documentation, either for the whole module or just for _LazyString or lazy_string, I should see something less terse than "Creates a lazy string by invoking func with args". Maybe just one more sentence like: "A typical usage might be ...". Maybe another sentence like: "Sage uses this code in constructing the path for the directory SAGE_TMP -- see sage.misc.misc."

And this could either be in the documentation for lazy_string or in a module-level docstring. I guess my point is this: I see the docstrings here, and I still have to think for a bit and read the examples to understand what this does and why you might want it. I'd rather not have to think. I also don't know why this is in Sage at all; I certainly think that there is some cruft in the Sage which should be removed, so I'd like to know why this is here. Regarding the existing doctests, they don't quite convince me why I would want to use this instead of just constructing the strings when I need them.

[roed]

Replying to kini:

All gettext stuff has been ripped out, so that exact text is no longer appropriate. Also I see several examples in the docstrings. What did you have in mind? Or did you mean a module-level docstring with one large example set walking you how to use all the functionality of the module?

I wasn't sure what the __gettext__ method did; it can be readded if desired.

[roed]

Replying to jhpalmieri:

Keshav: I'm thinking that when I read the documentation, either for the whole module or just for _LazyString or lazy_string, I should see something less terse than "Creates a lazy string by invoking func with args". Maybe just one more sentence like: "A typical usage might be ...". Maybe another sentence like: "Sage uses this code in constructing the path for the directory SAGE_TMP -- see sage.misc.misc."

And this could either be in the documentation for lazy_string or in a module-level docstring. I guess my point is this: I see the docstrings here, and I still have to think for a bit and read the examples to understand what this does and why you might want it. I'd rather not have to think. I also don't know why this is in Sage at all; I certainly think that there is some cruft in the Sage which should be removed, so I'd like to know why this is here. Regarding the existing doctests, they don't quite convince me why I would want to use this instead of just constructing the strings when I need them.

Makes sense. I'm going to be out of communication for a few days but I can add some more documentation when I get back. Keshav, if you want to do it before then that would be cool too. ;-)

[jdemeyer]

(never mind)

[jhpalmieri]

Ping.

I think this just needs a bit of work with the documentation.

[roed]

Adds speaklater.py to sage/misc with doctests.

[roed]

I've rebased against 5.4.rc1 and addressed John's concerns about documentation by adding to the module's docstring.

[jdemeyer]

This will likely need to be rebased to #13579.

[jhpalmieri]

Looks good to me now. I have attached a very small referee patch.

Should Keshav be listed as an author and/or a reviewer?

[jhpalmieri]

Wait, I'm getting some doctest failures. I have to investigate some more.

[jhpalmieri]

Okay, the problem is the line

return os.path.join(SAGE_TMP, 'spyx', os.sep) 

According to the documentation for ​os.path.join:

If any component is an absolute path, all previous components are thrown away, and joining continues.

So if you use os.sep (which is '/', an absolute path), everything before that is discarded. I've provided a revised referee's patch to fix this. My changes need review.

[ohanar]

John, your changes look good to me.

[jdemeyer]

Has it been verified that removing the trailing slash from SPYX_TMP and SAGE_TMP_INTERFACE doesn't change the functionality of the things using those strings?

[jhpalmieri]

The previous version passed doctests, but I see that it modified the behavior slightly because of the missing slash at the end of SAGE_TMP_INTERFACE. Here's a new patch. The variables SPYX_TMP and SAGE_TMP_INTERFACE are not used in local/bin or spkg/, so I think this new patch should take care of things.

[roed]

These are the only two locations that refer to SPYX_TMP and SAGE_TMP_INTERFACE. I'm happy that removing the trailing "/" won't cause additional problems, though it would be nice if the patchbot would run the tests....

[jhpalmieri]

Rebased to #13681.

[vbraun]

Initial patch

[vbraun]

Doctesting still litters files around than don't get deleted because SAGE_TESTDIR is not a the expected DOT_SAGE/tmp/hostname/pid/. The trac_13681_root.patch fixes this. Bumped from #13681 so we can finally ship Sage-5.4.

[jhpalmieri]

Volker, can you describe a situation in which doctesting leaves files lying around?

[vbraun]

Pretty much always. Just run make ptest and you end up with files in SAGE_TESTDIR. Perhaps they have to fail an/or be interrupted? I don't remember having to do anything special. Right now I have, for example,

[vbraun@laptop hg]$ ll ~/.sage/tmp/
-rw-rw-r--. 1 vbraun vbraun  82814 Nov  6 15:04 ring_3259.py

[jhpalmieri]

Failed doctests are supposed to leave files lying around. For example, when I run sage -tp ... and there is a failure, I see a message

The temporary doctesting directory
   /Users/palmieri/.sage/tmp/jpalmieri538-76123
was not removed: it is not empty, presumably because doctests
failed or doctesting was interrupted.

With your change, the files get left in subdirectories of tmp/jpalmieri538/ which are then cleaned up the next time Sage runs (or maybe when it exits). Since we have in the past decided to intentionally not delete the failing files, I'm not sure it's appropriate to automatically delete them.

I would suggest moving the Sage root patch and corresponding discussion to #12415, since that ticket modifies the doctesting framework considerably. I think that temporary files are not even created anymore during doctesting with the patches there. (Maybe you can help out with the interrupt issues, too.)

[vbraun]

I see. I do end up with a lot of cruft in .sage/tmp though, and I do think that this needs fixing. But its not urgent. So lets do the new doctesting stuff first and see what happens.