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

bug in try, alarm, and ??= (augmented null coalescing assignment operator) #3524

Closed
mahrud opened this issue Oct 18, 2024 · 10 comments
Closed

Comments

@mahrud
Copy link
Member

mahrud commented Oct 18, 2024

Try this, taken from tests/normal/alarm.m2:

R = QQ[vars(0..24)]
f = () -> (alarm 4; try res coker vars R else "ran out of time")
time f()

cokernel Matrix := Module => f -> f.cache.cokernel ??= subquotient(null, f)

R = QQ[vars(0..24)]
f = () -> (alarm 4; try res coker vars R else "ran out of time")
time f()

After the first run we get:

i3 : time f()
 -- used 5.55149s (cpu); 3.82313s (thread); 0s (gc)

o3 = ran out of time

But the second run never finishes.
I believe this is what's failing in #3517 and #3522 (removed commits: c07017e and 9b29e3d).

@d-torrance could you take a look?

@mahrud mahrud added this to the version 1.24.11 milestone Oct 18, 2024
@d-torrance
Copy link
Member

I think I've zeroed in on the problem a bit more. Certain errors break alarm using try or ??:

i1 : alarm 1; try ZZ.foo else sleep 2

o2 = 0

i3 : alarm 1; ZZ.foo ?? sleep 2

o4 = 0

But other errors are fine:

i5 : alarm 1; try 1/0 else sleep 2
stdio:3:9:(3): error: alarm occurred

i7 : alarm 1; 1/0 ?? sleep 2
stdio:4:13:(3): error: alarm occurred

@mahrud
Copy link
Member Author

mahrud commented Oct 26, 2024

That's interesting!
(ps: I know I added this to 1.24.11 milestone, but if you don't have time, it's not an absolute necessity)

@d-torrance
Copy link
Member

I should have some time this week -- I'd like to get this figured out!

It looks like it's just "key not found" errors. Also, I checked the 1.24.05 package in Debian unstable and this bug wasn't present. So this might be related to the changes from #3409...

@d-torrance
Copy link
Member

I think I figured it out! When keys aren't found, we call Thing.RobustPrintMethod, which itself calls try:

i1 : debug Core

i2 : code Thing.RobustPrintMethod

o2 = /usr/share/Macaulay2/Core/robust.m2:62:26-63:76: --source code:
     Thing.RobustPrintMethod = (msg, obj) -> try toString stack { msg | ":",
         horizontalJoin("\t", silentRobustNetWithClass(60, 5, 3, obj)) } else msg

So I think the alarm error is getting handled by that try and not the outer one.

@mahrud
Copy link
Member Author

mahrud commented Oct 26, 2024

Ah that makes sense! Should alarm break through all try's or just one? I think probably one. Can we remove the try .. else msg part from top-level and do exactly that in the interpreter? i.e. call Thing.RobustPrintMethod(msg, obj) and if it returns an error return msg.

@mahrud
Copy link
Member Author

mahrud commented Oct 26, 2024

Oh actually a simpler fix is to just remove try .. else msg! The d part is already correct.

@mahrud
Copy link
Member Author

mahrud commented Oct 26, 2024

Wait no this doesn't fix it. I think the inner try is part of it, but isn't the entire issue.

@d-torrance
Copy link
Member

Figured it out! silentRobustNetWithClass calls silentRobustNet, which calls timelimit, which calls alarm, cancelling the previous call.

timelimit := (t,f) -> (alarm t; r := f(); alarm 0; r)

Maybe we should have some kind of alarm stack?

@mahrud
Copy link
Member Author

mahrud commented Oct 26, 2024

Ah I hate alarm ... yes, that's certainly one possibility

However, maybe for now a simpler solution is to check in KeyNotFound whether we're inside a try or ?? clause and if so just return without processing the error. (Or maybe a similar thing somewhere else so no error messages are processed in side try or ??)

@mahrud
Copy link
Member Author

mahrud commented Oct 26, 2024

By the way, I don't think this is quite the same as #3371, but if there's a solution that fixes both issues it would be great. Also related is #2596.

d-torrance added a commit to d-torrance/M2 that referenced this issue Oct 26, 2024
The robust error message involves calling "alarm", which will cancel
any alarms we might be trying to catch using "try" or ??.

Closes: Macaulay2#3524
d-torrance added a commit that referenced this issue Oct 27, 2024
The robust error message involves calling "alarm", which will cancel
any alarms we might be trying to catch using "try" or ??.

Closes: #3524
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants