Opened 11 months ago

Last modified 6 months ago

#26968 new defect

ECL test sometimes fails to create maxima directory

Reported by: gh-timokau Owned by:
Priority: major Milestone:
Component: doctest framework Keywords: random_fail ecl
Cc: embray, saraedum, jdmeyer Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

I just got this test failure (and I'm pretty sure I've seen it before):

File "/nix/store/br51f1j23cnxmbf19yn62rn5pp2rmqdh-sage-src-8.5/src/doc/ca/intro/index.rst", line 492, in doc.ca.intro.index
Failed example:
    solve([x + y == 6, x - y == 4], x, y)
Exception raised:
    Traceback (most recent call last):
      File "/nix/store/mfbhcnzdxiyj831pr3hg1wwh556a6lv3-python-2.7.15-env/lib/python2.7/site-packages/sage/doctest/forker.py", line 671, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/nix/store/mfbhcnzdxiyj831pr3hg1wwh556a6lv3-python-2.7.15-env/lib/python2.7/site-packages/sage/doctest/forker.py", line 1086, in compile_and_execute
        exec(compiled, globs)
      File "<doctest doc.ca.intro.index[1]>", line 1, in <module>
        solve([x + y == Integer(6), x - y == Integer(4)], x, y)
      File "/nix/store/mfbhcnzdxiyj831pr3hg1wwh556a6lv3-python-2.7.15-env/lib/python2.7/site-packages/sage/symbolic/relation.py", line 1107, in solve
        m = maxima(f)
      File "sage/misc/lazy_import.pyx", line 354, in sage.misc.lazy_import.LazyImport.__call__ (build/cythonized/sage/misc/lazy_import.c:3682)
        return self.get_object()(*args, **kwds)
      File "sage/misc/lazy_import.pyx", line 189, in sage.misc.lazy_import.LazyImport.get_object (build/cythonized/sage/misc/lazy_import.c:2347)
        return self._get_object()
      File "sage/misc/lazy_import.pyx", line 221, in sage.misc.lazy_import.LazyImport._get_object (build/cythonized/sage/misc/lazy_import.c:2584)
        self._object = getattr(__import__(self._module, {}, {}, [self._name]), self._name)
      File "/nix/store/mfbhcnzdxiyj831pr3hg1wwh556a6lv3-python-2.7.15-env/lib/python2.7/site-packages/sage/interfaces/maxima_lib.py", line 113, in <module>
        ecl_eval("(set-pathnames)")
      File "sage/libs/ecl.pyx", line 1326, in sage.libs.ecl.ecl_eval (build/cythonized/sage/libs/ecl.c:10224)
        cpdef EclObject ecl_eval(str s):
      File "sage/libs/ecl.pyx", line 1341, in sage.libs.ecl.ecl_eval (build/cythonized/sage/libs/ecl.c:10163)
        o=ecl_safe_eval(o)
      File "sage/libs/ecl.pyx", line 350, in sage.libs.ecl.ecl_safe_eval (build/cythonized/sage/libs/ecl.c:5221)
        raise RuntimeError("ECL says: {}".format(
    RuntimeError: ECL says: Could not create directory "/build/sage-home/.sage/maxima"
    C library error: "File exists"
**********************************************************************

This cannot be reliably reproduced. Probably some parallelism issue (tests were run with 4 threads).

Change History (19)

comment:1 Changed 11 months ago by gh-timokau

  • Cc embray saraedum added

It would be nice to give every test process its own DOT_SAGE directory. Would that be possible? I tried doing that, but got confused as to how the doctest processes are actually started and how isolated they are from the "master" process. I tried setting DOT_SAGE in env.py based on DOCTEST_MODE, but that apparently always is False.

comment:2 Changed 11 months ago by embray

I have also argued in the past that the tests should use a clean .sage. It's a problem for some other tests as well. I don't know if there's a good reason not to, other than "nobody's done it yet".

comment:3 follow-up: Changed 11 months ago by gh-timokau

  • Cc jdmeyer added

The only reason I could think of is that some weird configurations wouldn't be tested. But I would think that is more of a feature: Having doctests depend on the user configuration makes everything much more difficult and relying on users to pick up issues when testing with their configuration doesn't sound like a particularly good practice either.

comment:4 in reply to: ↑ 3 Changed 11 months ago by embray

Replying to gh-timokau:

The only reason I could think of is that some weird configurations wouldn't be tested. But I would think that is more of a feature: Having doctests depend on the user configuration makes everything much more difficult and relying on users to pick up issues when testing with their configuration doesn't sound like a particularly good practice either.

Yes, if there are any bugs resulting from user configuration those should be then reported by users and captured as a regression test: Tests should be isolated as possible and reproducible.

comment:5 Changed 11 months ago by gh-timokau

Do you happen to know how practical it would be to implement then? As I mentioned earlier, it seems like DOCTEST_MODE is only set to true *after* env.py was evaluated.

comment:6 follow-up: Changed 11 months ago by embray

I think it would be quite practical; just nobody's been sufficiently motivated enough before now.

Probably somewhere in sage.doctest.forker we would just create a tempdir and set sage.env.DOT_SAGE to that tempdir before running any examples in a test file.

Last edited 11 months ago by embray (previous) (diff)

comment:7 Changed 11 months ago by gh-timokau

But that would set sage.env.DOT_SAGE for all processes right?

comment:8 Changed 11 months ago by embray

No, not if it were done after fork.

comment:9 Changed 11 months ago by embray

  • Milestone changed from sage-8.6 to sage-8.7

Retarging tickets optimistically to the next milestone. If you are responsible for this ticket (either its reporter or owner) and don't believe you are likely to complete this ticket before the next release (8.7) please retarget this ticket's milestone to sage-pending or sage-wishlist.

comment:10 in reply to: ↑ 6 Changed 11 months ago by gh-timokau

Replying to embray:

I think it would be quite practical; just nobody's been sufficiently motivated enough before now.

Probably somewhere in sage.doctest.forker we would just create a tempdir and set sage.env.DOT_SAGE to that tempdir before running any examples in a test file.

I now remembered why that wouldn't work. There are other variables that depend on sage.env.DOT_SAGE which wouldn't be re-evaluated. We'd probably need to either change env.py to use @propertys or spawn completely independent sage instances.

comment:11 Changed 11 months ago by gh-timokau

Although that wouldn't even fix this issue since MAXIMA_USERDIR is set in sage-env, not env.py.

comment:12 Changed 11 months ago by embray

I still haven't looked at it that deeply but I suspect you're overthinking it. Though the concern about MAXIMA_USERDIR is definitely valid. That's the kind of problem I wanted to address in #22652, but every time it's come up it's been easier to just implement a narrow fix rather than a general solution.

comment:13 follow-up: Changed 11 months ago by gh-timokau

Repeating my comment I accidentally posted in #22652 here:

Replying to embray:

I still haven't looked at it that deeply but I suspect you're overthinking it.

env.py contains these lines:

_add_variable_or_fallback('PYTHON_EGG_CACHE',   opj('$DOT_SAGE', '.python-eggs'))
_add_variable_or_fallback('SAGE_STARTUP_FILE',  opj('$DOT_SAGE', 'init.sage'))

Which is admittedly way less occurrences than I expected. Still, those would need to be re-evaluated or their usage changed.

Though the concern about MAXIMA_USERDIR is definitely valid. That's the kind of problem I wanted to address in #22652,

#22652 would be amazing. I don't see why it would necessary fix these issues though. Of course it would make sense to tackle both issues at the same time.

but every time it's come up it's been easier to just implement a narrow fix rather than a general solution.

Speaking of narrow fixes, for this specific use-case, for this particular issue that should do it (how do I get that nice patch highlighting on trac?):

diff --git a/src/sage/doctest/forker.py b/src/sage/doctest/forker.py
index 02e18e67e7..115f748727 100644
--- a/src/sage/doctest/forker.py
+++ b/src/sage/doctest/forker.py
@@ -668,7 +668,14 @@ class SageDocTestRunner(doctest.DocTestRunner, object):
 
             try:
                 # Don't blink!  This is where the user's code gets run.
+                # print("MAXIMA_USERDIR is: {}".format(os.environ['MAXIMA_USERDIR']))
+                os.environ['MAXIMA_USERDIR'] = "/tmp/maxima-userdir-{}".format(os.getpid())
                 self.compile_and_execute(example, compiler, test.globs)
             except SystemExit:
                 raise
             except BaseException:

I'll probably adopt that for nix. I don't want to fix the same issue on our build server every week. Should I submit that upstream or is that too "hacky"?

comment:15 in reply to: ↑ 13 Changed 11 months ago by jhpalmieri

Replying to gh-timokau:

(how do I get that nice patch highlighting on trac?)

On the line right after the triple braces {{{, add the line #!diff. (See also https://trac.sagemath.org/wiki/WikiProcessors for more options like #!sh, #!html, etc.)

Last edited 11 months ago by jhpalmieri (previous) (diff)

comment:16 Changed 11 months ago by jdemeyer

You seem too much focusing on the doctest framework here: this is not a bug in the doctest framework, so it shouldn't be fixed in the doctest framework.

Ideally, this should be reported to ECL upstream.

comment:17 Changed 11 months ago by gh-timokau

They seem to assume only one ecl process is running at a time, which may or may not be a bug.

comment:18 Changed 9 months ago by embray

  • Milestone changed from sage-8.7 to sage-8.8

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

comment:19 Changed 6 months ago by embray

  • Milestone sage-8.8 deleted

As the Sage-8.8 release milestone is pending, we should delete the sage-8.8 milestone for tickets that are not actively being worked on or that still require significant work to move forward. If you feel that this ticket should be included in the next Sage release at the soonest please set its milestone to the next release milestone (sage-8.9).

Note: See TracTickets for help on using tickets.