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 )
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)
Change History (75)
Changed 7 years ago by
comment:1 Changed 7 years ago by
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
- Status changed from new to needs_review
comment:3 Changed 7 years ago by
- Description modified (diff)
comment:4 Changed 7 years ago by
+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
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
.
comment:6 Changed 7 years ago by
Andrew, are you referring to the fact that Mike creates speaklater.py in the Sage library?
comment:7 Changed 7 years ago by
- 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
patchbot: apply 13147_lazy.patch
comment:9 Changed 7 years ago by
- 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: ↓ 11 Changed 7 years ago by
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
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
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: ↓ 14 Changed 7 years ago by
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
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
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
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
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
So what's the plan for this ticket? The options seem to be:
- Include speaklater.py with no doctests; include attribution and license.
- Include speaklater.py with doctests (I'm not going to write them...); include attribution and license.
- Depend on #11080 and import speaklater from sagenb
- Make an spkg.
- Write a simplified lazy string with doctests.
I think my vote would be for 3.
comment:19 Changed 7 years ago by
I vote for 1.
comment:20 Changed 7 years ago by
I vote for 4.
comment:21 Changed 7 years ago by
Here's an SPKG: http://wstein.org/home/keshav/files/speaklater-1.2-fbc7a37.spkg
comment:22 Changed 7 years ago by
I also vote for the spkg. The one Keshav provided looks fine.
comment:23 Changed 7 years ago by
We should also patch deps since this would become a standard SPKG.
comment:24 follow-up: ↓ 26 Changed 7 years ago by
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
- 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
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
Oops, forgot to update SPKG.txt - done now (same URL).
comment:28 Changed 7 years ago by
- 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
- Status changed from needs_review to needs_work
Missed something; I'll fix it in a few hours.
comment:31 follow-up: ↓ 34 Changed 7 years ago by
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
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: ↓ 36 Changed 7 years ago by
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
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: ↓ 37 Changed 7 years ago by
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
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
Replying to jhpalmieri:
Keshav: I'm thinking that when I read the documentation, either for the whole module or just for
_LazyString
orlazy_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:39 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:40 Changed 7 years ago by
Ping.
I think this just needs a bit of work with the documentation.
comment:41 Changed 7 years ago by
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
This will likely need to be rebased to #13579.
Changed 7 years ago by
comment:43 Changed 7 years ago by
- Dependencies set to #13579
- Description modified (diff)
comment:44 Changed 7 years ago by
- 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
- 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
- Status changed from needs_work to needs_review
comment:47 Changed 7 years ago by
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
- Status changed from needs_review to positive_review
John, your changes look good to me.
comment:49 Changed 7 years ago by
- Milestone changed from sage-5.4 to sage-5.5
comment:50 Changed 7 years ago by
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
- Status changed from positive_review to needs_work
comment:52 Changed 7 years ago by
- 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
comment:53 Changed 7 years ago by
- 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
- Status changed from needs_review to positive_review
comment:55 Changed 7 years ago by
- Milestone changed from sage-5.4 to sage-5.5
comment:56 Changed 7 years ago by
- 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
- Merged in sage-5.5.beta1 deleted
- Resolution fixed deleted
- Status changed from closed to new
comment:58 Changed 7 years ago by
- Dependencies changed from #13579 to #13579, #13681
Changed 7 years ago by
comment:60 Changed 7 years ago by
- Description modified (diff)
- Status changed from needs_review to positive_review
comment:61 Changed 7 years ago by
- 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
- Status changed from needs_work to needs_review
comment:63 Changed 7 years ago by
Volker, can you describe a situation in which doctesting leaves files lying around?
comment:64 Changed 7 years ago by
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
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
- 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
- Merged in set to sage-5.5.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
David Roe/Robert? Bradshaw's original solution: I don't like it as much as Mike's