Macaulay2 / M2

The primary source code repository for Macaulay2, a system for computing in commutative algebra, algebraic geometry and related fields.
https://macaulay2.com
345 stars 231 forks source link

Improving capture #1689

Open mahrud opened 3 years ago

mahrud commented 3 years ago

This is a continuation of #1469.

Currently many examples in the documentation of "Macaulay2Doc" are being generated using capture, however, there are still a number of remaining issues when using capture for generating examples, each one is being avoided by one of the lines below: https://github.com/Macaulay2/M2/blob/2d48ff6c533e565a62a98ae4e8107c6a10deb4c5/M2/Macaulay2/m2/examples.m2#L159-L171 I think it's fairly safe to use capture for other packages as well, but I think it would require a closer review before doing that for every package.

  1. The string -* no-capture-flag *- is used to single out specific examples that aren't excluded by any other keyword. Currently, only the following examples have this tag:

    • (Grassmannian, ZZ, ZZ) and "finite fields", because they use the option Variable => T in the following lines:
      GF (ZZ/2[T]/(T^9+T+1), Variable => T) -* no-capture-flag *-",
      J = Grassmannian(2,5, CoefficientRing => ZZ/31, Variable => T) -* no-capture-flag *-

      . As it turns out, the variable "T" (and only this one!) is particularly annoying. See https://github.com/Macaulay2/M2/issues/1627#issuecomment-736206735

    • "new", because it includes this line which has a permanent effect by adding a new method:
      new Type of BasicList from Function := (A,B,f) -> hashTable { net => f }; -* no-capture-flag *-
  2. capture still doesn't capture direct printing to either stderr or stdout, hence why any keyword that affects those is currently excluded, like gbTrace, stderr, stdio, print, <<.

  3. examples that use notify may be changed if a file is already loaded, so those are excluded, examples that use stopIfError and debuggingMode are also likely not going to work yet, unless there's some sort of time limit to interrupt capture.

  4. Certain actions have a permanent effect: GlobalAssignHook, GlobalReleaseHook, installMethod

  5. export, newPackage, read, run aren't well tested yet, the same with schedule, thread, *Task.

  6. load in effect prevents any of the checks above from working, so it has to be excluded

  7. temporaryFileName is buggy when the same process uses it repeatedly, so it is excluded, but can probably be fixed.

  8. ThreadedGB and RunExternalM2 packages probably shouldn't use capture anyway.

TODO:

A few things listed above can be fixed, but to reiterate, I think the following are the important ones:

I already have made changes in capture to save and restore the values of all exported mutable symbols from all packages, but I'm not confident that this entirely eliminates the risk of capturing one example from affecting the result of future examples.

pzinn commented 3 years ago

unrelated, but can we make this line slightly more mode-agnostic:

new Type of BasicList from Function := (A,B,f) -> hashTable { net => f, html => f }; -* no-capture-flag *-

otherwise the example in the "new" help makes no sense when run in WebApp mode.

mahrud commented 3 years ago

@DanGrayson thanks for the help, this now works:

i8 : format last capture ///stderr << "hi" << endl///

o8 = "
     i1 : stderr << \"hi\" << endl
     hi

     o1 = stdio

     o1 : File

     i2 : 
     "

However, I noticed that in a lot of places in the interpreter, stderr is used rather than stdError, here is an example: https://github.com/Macaulay2/M2/blob/0205d20ab465e8cb7721cf507e3febf9b580419d/M2/Macaulay2/d/stdiop.d#L127-L137 Changing stderr -> stdError fixes capturing of errors:

i2 : format last capture "error 4"

o2 = "
     i1 : error 4
     currentString:1:1:(3):[4]: error: 4
     "

But I'm not sure if there's a more important reason why stderr is used here or not. In particular, here is where stderr is defined in the interpreter: https://github.com/Macaulay2/M2/blob/0205d20ab465e8cb7721cf507e3febf9b580419d/M2/Macaulay2/d/errio.d#L2-L7 Is it safe to replace them? I did so only for printMessage in 8ad88cd.

DanGrayson commented 3 years ago

I think the point of stderr is that it can be used while debugging the more complicated stdError, so your replacement of it seems fine.