Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix capture + several minor bug fixes #1546

Merged
merged 23 commits into from
Dec 3, 2020

Conversation

mahrud
Copy link
Member

@mahrud mahrud commented Oct 18, 2020

This PR began with the intention of reducing the time it takes to install Macaulay2Doc from >1hr to <15min, but that feature is disabled temporarily until this line is removed:

and false -- TODO: this is temporarily here, to be removed after v1.17 is released

@mahrud
Copy link
Member Author

mahrud commented Oct 19, 2020

It went decently well, till this happened:

 -- capturing example results for index
../../../../Macaulay2/m2/methods.m2:37:35:(1):[15]: error: no method found for applying index to:
     argument   :  a (of class Symbol)
../../../../Macaulay2/m2/methods.m2:133:19:(1):[14]: --back trace--
currentString:2:1:(3):[13]: --back trace--
 -- making example results for index
 -- capturing example results for forceGB(..., SyzygyMatrix => ...)

   -- registering polynomial ring 212 at 0x7fd17524c100

   -- registering gb 312 at 0x7fd174f89540

   -- [gb]{2}(1)m{3}(1)m{4}(1)m{5}(1)z{6}(1)z{7}(1)znumber of (nonminimal) gb elements = 3
   -- number of monomials                = 9
   -- #reduction steps = 6
   -- #spairs done = 6
   -- ncalls = 0
   -- nloop = 0
   -- nsaved = 0
   --  -- capturing example results for Symbol _ Ring

   -- registering polynomial ring 213 at 0x7fd17524c000

   -- registering polynomial ring 214 at 0x7fd174d3af00
 -- capturing example results for pairs
 -- capturing example results for end

 -- removing polynomial ring 76 at 0x7fd175084d00
--loaded ../../../../Macaulay2/packages/Macaulay2Doc/demo3.m2
--loaded ../../../../Macaulay2/packages/Macaulay2Doc/demo1.m2
../../../../Macaulay2/packages/Macaulay2Doc/demo1.m2:8:12:(3):[15]: error: division by zero
../../../../Macaulay2/packages/Macaulay2Doc/demo1.m2:8:12:(3):[15]: --entering debugger (type help to see debugger commands)

i5 : 

@mahrud
Copy link
Member Author

mahrud commented Oct 20, 2020

This error appears at the very end of installing Macaulay2Doc, but running it again doesn't produce any errors!

../../../../Macaulay2/m2/debugging.m2:20:6:(1):[12]: error: internal error: documentation node not processed yet: Grassmannian2
../../../../Macaulay2/m2/document.m2:349:10:(1):[11]: --back trace--
../../../../Macaulay2/m2/installPackage.m2:434:21:(1):[10]: --back trace--
../../../../Macaulay2/m2/installPackage.m2:419:5:(1):[9]: --back trace--
../../../../Macaulay2/m2/installPackage.m2:753:31:(1):[8]: --back trace--
../../../../Macaulay2/m2/methods.m2:119:80:(1):[7]: --back trace--
../../../../Macaulay2/m2/option.m2:16:8:(1):[6]: --back trace--
../../../../Macaulay2/m2/installPackage.m2:572:5:(1):[5]: --back trace--
../../../../Macaulay2/m2/methods.m2:119:80:(1):[4]: --back trace--
../../../../Macaulay2/m2/option.m2:16:8:(1):[3]: --back trace--
currentString:1:1:(3):[2]: --back trace--
../../../../Macaulay2/m2/debugging.m2:20:6:(1):[3]: error: package not current: User
../../../../Macaulay2/m2/packages.m2:455:76:(1):[2]: --back trace--
../../../../Macaulay2/m2/packages.m2:274:18:(1):[1]: --back trace--

1 :

The entire process takes under 10 minutes.

@mahrud
Copy link
Member Author

mahrud commented Oct 26, 2020

@DanGrayson I think this should be working now, except capture isn't storing stderr. Do you know what is the change required to make that work?

@mahrud mahrud force-pushed the quickfix/capture branch 2 times, most recently from f1e6647 to 252cf66 Compare October 27, 2020 08:13
@DanGrayson
Copy link
Member

@DanGrayson I think this should be working now, except capture isn't storing stderr. Do you know what is the change required to make that work?

Yes, somehow we have to set it up so that the output sent to either stdError or to stdIO is buffered in the same buffer, for use by the "capture" function.

@mahrud
Copy link
Member Author

mahrud commented Nov 4, 2020

@DanGrayson do you have any thoughts on this PR so far? In particular, I had to add a -* no-capture-flag *- to some examples to avoid them being run with capture. Eventually I'd like that to be unnecessary, but I think the remaining action in this PR is getting stderr to be captured.

@DanGrayson
Copy link
Member

Well, how many examples actually print to stderr? Probably not many, and you could find them and mark them, too. Just search for "stderr", and also mark the ones that demonstrate the use of the debugger after an error.

By the way, is the no-capture-flag filtered out so it doesn't appear in the documentation?

@mahrud
Copy link
Member Author

mahrud commented Nov 4, 2020

Well, how many examples actually print to stderr? Probably not many, and you could find them and mark them, too. Just search for "stderr", and also mark the ones that demonstrate the use of the debugger after an error.

That was the cause of most of the ones that I had to exclude.

By the way, is the no-capture-flag filtered out so it doesn't appear in the documentation?

No, but I can do it.

@DanGrayson
Copy link
Member

By the way, is the no-capture-flag filtered out so it doesn't appear in the documentation?

No, but I can do it.

Something like that would be good. What about instead letting EXAMPLE be a function with options, and introducing an option, such as Capture => false ?

@mahrud
Copy link
Member Author

mahrud commented Nov 4, 2020

Something like that would be good. What about instead letting EXAMPLE be a function with options, and introducing an option, such as Capture => false ?

I'd prefer to eventually make capture work for everything. I don't see any intrinsic value in wanting an example to be run externally.

@DanGrayson
Copy link
Member

Something like that would be good. What about instead letting EXAMPLE be a function with options, and introducing an option, such as Capture => false ?

I'd prefer to eventually make capture work for everything. I don't see any intrinsic value in wanting an example to be run externally.

Okay, good point. But, hmm, do these examples still work when capturing?: https://faculty.math.illinois.edu/Macaulay2/doc/Macaulay2-1.16/share/doc/Macaulay2/Macaulay2Doc/html/_the_spdebugger.html

@mahrud
Copy link
Member Author

mahrud commented Nov 4, 2020

Okay, good point. But, hmm, do these examples still work when capturing?: https://faculty.math.illinois.edu/Macaulay2/doc/Macaulay2-1.16/share/doc/Macaulay2/Macaulay2Doc/html/_the_spdebugger.html

Not currently, which is why I added the no-capture-flag for it. But once a few issues are resolved, I don't see why it wouldn't?

@DanGrayson
Copy link
Member

By the way, is the no-capture-flag filtered out so it doesn't appear in the documentation?

No, but I can do it.

... waiting for this update.

@mahrud
Copy link
Member Author

mahrud commented Nov 19, 2020

By the way, is the no-capture-flag filtered out so it doesn't appear in the documentation?

No, but I can do it.

... waiting for this update.

I'm waiting for capturing stderr to work. Without it, you'd have to check all packages to see if any examples print errors that should be captured.

@DanGrayson
Copy link
Member

By the way, is the no-capture-flag filtered out so it doesn't appear in the documentation?

No, but I can do it.

... waiting for this update.

I'm waiting for capturing stderr to work. Without it, you'd have to check all packages to see if any examples print errors that should be captured.

Why wait for that? What I meant was, that while processing the documentation, we strip such comments out, so they don't appear in the documentation, because the comments will confuse the readers.

@mahrud
Copy link
Member Author

mahrud commented Nov 19, 2020

Why wait for that? What I meant was, that while processing the documentation, we strip such comments out, so they don't appear in the documentation, because the comments will confuse the readers.

That's a one line change, but this PR isn't complete even with that. Currently the documentation for any node that intentionally prints to stderr is incomplete, and it's too much work and maintenance to regularly check which examples print to stderr.

@DanGrayson
Copy link
Member

Why wait for that? What I meant was, that while processing the documentation, we strip such comments out, so they don't appear in the documentation, because the comments will confuse the readers.

That's a one line change, but this PR isn't complete even with that. Currently the documentation for any node that intentionally prints to stderr is incomplete, and it's too much work and maintenance to regularly check which examples print to stderr.

If the PR isn't complete, why pull it? Won't that make the documentation of nodes that print to stderr also look bad?

@mahrud mahrud marked this pull request as draft November 19, 2020 18:52
@mahrud
Copy link
Member Author

mahrud commented Nov 19, 2020

I never said pull this now! It's not done!

@DanGrayson
Copy link
Member

I never said pull this now! It's not done!

Oops, okay.

@mahrud mahrud mentioned this pull request Dec 1, 2020
1 task
@mahrud mahrud linked an issue Dec 1, 2020 that may be closed by this pull request
@mahrud
Copy link
Member Author

mahrud commented Dec 1, 2020

@DanGrayson the CMake builds finish in under 1hr. I excluded a bunch of examples involving printing and stderr from being captured. If you have some time to take a look at why print and File << Thing don't seem to behave well with capture (and also missing stderr), this PR could go into this release, otherwise probably would be safer to merge after the release so there's more time to look for potential documentation issues.

@mahrud
Copy link
Member Author

mahrud commented Dec 1, 2020

Oh, actually, since this PR solves a few other issues as well, a far better thing to do is to temporarily add false and on this line:

-- try capturing in the same process
if not match("no-capture-flag", inputs) -- this flag is really necessary, but only sometimes

and merge this now. This was the documentation gets built without capture, but capture is available for use and those other bugs are (like #963) are also resolved.

@DanGrayson
Copy link
Member

@DanGrayson the CMake builds finish in under 1hr. I excluded a bunch of examples involving printing and stderr from being captured. If you have some time to take a look at why print and File << Thing don't seem to behave well with capture (and also missing stderr), this PR could go into this release, otherwise probably would be safer to merge after the release so there's more time to look for potential documentation issues.

I'll take a look...

@mahrud mahrud requested a review from DanGrayson December 2, 2020 02:29
@mahrud
Copy link
Member Author

mahrud commented Dec 2, 2020

@DanGrayson I manually disabled capturing example results, so it can be merged without any risk of harming the documentation.

As a bonus, I managed to do what @mikestillman had asked for, so running the examples looks like this:

[536/545] Installing package Matroids
 -- making example results for "hyperplanes"                                 -- 1.48583 seconds elapsed
 -- making example results for "representationOf"                            -- 1.12504 seconds elapsed
 -- making example results for "getIsos"                                     -- 1.01062 seconds elapsed
 -- making example results for "independenceComplex(Matroid)"                -- 1.27673 seconds elapsed
 -- making example results for "quickIsomorphismTest"                        -- 1.61963 seconds elapsed

@DanGrayson
Copy link
Member

@DanGrayson I manually disabled capturing example results, so it can be merged without any risk of harming the documentation.

As a bonus, I managed to do what @mikestillman had asked for, so running the examples looks like this:

[536/545] Installing package Matroids
 -- making example results for "hyperplanes"                                 -- 1.48583 seconds elapsed
 -- making example results for "representationOf"                            -- 1.12504 seconds elapsed
 -- making example results for "getIsos"                                     -- 1.01062 seconds elapsed
 -- making example results for "independenceComplex(Matroid)"                -- 1.27673 seconds elapsed
 -- making example results for "quickIsomorphismTest"                        -- 1.61963 seconds elapsed

So this pull request offers no advantages over the status quo? In that case, you could submit it when it's ready.

@mahrud
Copy link
Member Author

mahrud commented Dec 2, 2020

So this pull request offers no advantages over the status quo? In that case, you could submit it when it's ready.

There are several issues that this PR fixes, even without using capture! Take a look at the diff.

@mahrud mahrud changed the title Use capture for examples instead of an external M2 process Fix capture + several minor bug fixes Dec 2, 2020
@DanGrayson DanGrayson merged commit 2aa08f1 into Macaulay2:development Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'help "RingElement"' fails after defining a polynomial ring
3 participants