Opened 7 years ago

Closed 7 years ago

#13147 closed enhancement (fixed)

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

Reported by: roed Owned by: mvngu
Priority: major Milestone: sage-5.5
Component: doctest coverage Keywords:
Cc: mhansen Merged in: sage-5.5.beta1
Authors: David Roe, Mike Hansen Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13579, #13681 Stopgaps:

Description (last modified by vbraun)

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

Apply

Attachments (8)

13147_move.patch (42.8 KB) - added by roed 7 years ago.
David Roe/Robert? Bradshaw's original solution: I don't like it as much as Mike's
13147_lazy.patch (7.5 KB) - added by roed 7 years ago.
Mike Hansen's solution
13147_lazy_spkg.patch (4.3 KB) - added by ohanar 7 years ago.
Lazy string solution for use with spkg
13147_new.patch (17.3 KB) - added by roed 7 years ago.
Adds speaklater.py to sage/misc with doctests.
13147_over_13579.patch (16.3 KB) - added by roed 7 years ago.
trac_13147-ref.patch (3.7 KB) - added by jhpalmieri 7 years ago.
trac_13147-rebased-to-13681.patch (15.3 KB) - added by jhpalmieri 7 years ago.
trac_13681_root.patch (714 bytes) - added by vbraun 7 years ago.
Initial patch

Download all attachments as: .zip

Change History (75)

Changed 7 years ago by roed

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

comment:1 Changed 7 years ago by 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?

comment:2 Changed 7 years ago by roed

  • Status changed from new to needs_review

comment:3 Changed 7 years ago by roed

  • Description modified (diff)

comment:4 Changed 7 years ago by 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.

comment:5 Changed 7 years ago by 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.

Changed 7 years ago by roed

Mike Hansen's solution

comment:6 Changed 7 years ago by kini

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

comment:7 Changed 7 years ago by roed

  • Cc mhansen added

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

patchbot: apply 13147_lazy.patch.

comment:8 Changed 7 years ago by kini

patchbot: apply 13147_lazy.patch

comment:9 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

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

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

comment:10 follow-up: Changed 7 years ago by 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.

comment:11 in reply to: ↑ 10 Changed 7 years ago by 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.

comment:12 Changed 7 years ago by 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.

comment:13 follow-up: Changed 7 years ago by 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.

comment:14 in reply to: ↑ 13 Changed 7 years ago by 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.

comment:15 Changed 7 years ago by 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.

comment:16 Changed 7 years ago by 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.

comment:17 Changed 7 years ago by 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.

comment:18 Changed 7 years ago by 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.

comment:19 Changed 7 years ago by jdemeyer

I vote for 1.

comment:20 Changed 7 years ago by kini

I vote for 4.

comment:22 Changed 7 years ago by ohanar

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

Changed 7 years ago by ohanar

Lazy string solution for use with spkg

comment:23 Changed 7 years ago by kini

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

comment:24 follow-up: Changed 7 years ago by 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?

comment:25 Changed 7 years ago by 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.

comment:26 in reply to: ↑ 24 Changed 7 years ago by 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

comment:27 Changed 7 years ago by kini

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

comment:28 Changed 7 years ago by roed

  • Description modified (diff)
  • Status changed from needs_work to needs_review

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

comment:29 Changed 7 years ago by roed

  • Status changed from needs_review to needs_work

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

comment:30 Changed 7 years ago by roed

  • Status changed from needs_work to needs_review

Alright, fixed.

comment:31 follow-up: Changed 7 years ago by jhpalmieri

Line 23 of cleaner.py:

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

Should it say str(SAGE_TMP)?

comment:32 Changed 7 years ago by 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.)

comment:33 follow-up: Changed 7 years ago by 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?

comment:34 in reply to: ↑ 31 Changed 7 years ago by 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).

comment:35 follow-up: Changed 7 years ago by 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.

comment:36 in reply to: ↑ 33 Changed 7 years ago by 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.

comment:37 in reply to: ↑ 35 Changed 7 years ago by 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. ;-)

comment:38 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

(never mind)

Last edited 7 years ago by jdemeyer (previous) (diff)

comment:39 Changed 7 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:40 Changed 7 years ago by jhpalmieri

Ping.

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

Changed 7 years ago by roed

Adds speaklater.py to sage/misc with doctests.

comment:41 Changed 7 years ago by roed

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

comment:42 Changed 7 years ago by jdemeyer

This will likely need to be rebased to #13579.

Changed 7 years ago by roed

comment:43 Changed 7 years ago by roed

  • Dependencies set to #13579
  • Description modified (diff)

comment:44 Changed 7 years ago by jhpalmieri

  • Description modified (diff)
  • Reviewers set to John Palmieri
  • Status changed from needs_review to positive_review

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

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

comment:45 Changed 7 years ago by jhpalmieri

  • Status changed from positive_review to needs_work

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

comment:46 Changed 7 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

comment:47 Changed 7 years ago by 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.

comment:48 Changed 7 years ago by ohanar

  • Status changed from needs_review to positive_review

John, your changes look good to me.

comment:49 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.4 to sage-5.5

comment:50 Changed 7 years ago by 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?

comment:51 Changed 7 years ago by jhpalmieri

  • Status changed from positive_review to needs_work

comment:52 Changed 7 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

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.

Changed 7 years ago by jhpalmieri

comment:53 Changed 7 years ago by roed

  • Milestone changed from sage-5.5 to sage-5.4

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....

comment:54 Changed 7 years ago by roed

  • Status changed from needs_review to positive_review

comment:55 Changed 7 years ago by roed

  • Milestone changed from sage-5.4 to sage-5.5

comment:56 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.5.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:57 Changed 7 years ago by jdemeyer

  • Merged in sage-5.5.beta1 deleted
  • Resolution fixed deleted
  • Status changed from closed to new

comment:58 Changed 7 years ago by jdemeyer

  • Dependencies changed from #13579 to #13579, #13681

comment:59 Changed 7 years ago by jhpalmieri

  • Status changed from new to needs_review

Rebased to #13681.

Changed 7 years ago by jhpalmieri

comment:60 Changed 7 years ago by jhpalmieri

  • Description modified (diff)
  • Status changed from needs_review to positive_review

Changed 7 years ago by vbraun

Initial patch

comment:61 Changed 7 years ago by vbraun

  • Description modified (diff)
  • Status changed from positive_review to needs_work

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.

comment:62 Changed 7 years ago by vbraun

  • Status changed from needs_work to needs_review

comment:63 Changed 7 years ago by jhpalmieri

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

comment:64 Changed 7 years ago by 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

comment:65 Changed 7 years ago by 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.)

comment:66 Changed 7 years ago by vbraun

  • Description modified (diff)
  • Status changed from needs_review to positive_review

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.

comment:67 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.5.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.