Opened 9 years ago

Last modified 9 years ago

#13147 closed enhancement

Delay the evaluation of SAGE_TMP in order to facilitate forking in doctesting — at Version 28

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

Status badges

Description (last modified by roed)

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

Apply attachment:13147_new.patch.

Change History (31)

Changed 9 years ago by roed

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

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

  • Status changed from new to needs_review

comment:3 Changed 9 years ago by roed

  • Description modified (diff)

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

Mike Hansen's solution

comment:6 Changed 9 years ago by kini

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

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

patchbot: apply 13147_lazy.patch

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

I vote for 1.

comment:20 Changed 9 years ago by kini

I vote for 4.

comment:22 Changed 9 years ago by ohanar

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

Changed 9 years ago by ohanar

Lazy string solution for use with spkg

comment:23 Changed 9 years ago by kini

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

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

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

comment:28 Changed 9 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

Note: See TracTickets for help on using tickets.