CSSE1001 / MyPyTutor

Interactive tutorial application for Python3.
Other
7 stars 12 forks source link

input in "Writing a Function" problem freezes MPT #155

Closed pjritee closed 9 years ago

pjritee commented 9 years ago

For this code

def square(n): return n*n n = input("enter num ")

When you press F5 MPT freezes and unfortunately if the student has synced this then every time they choose this problem MPT freezes.

I thought we had fixed this problem. We should add a check for the use of input in problems that don't require it in the static analysis but we should fix the check independently of this.

sapi commented 9 years ago

Oh, right, students are adding input statements outside the function. That's just silly!

The problem is that input redirection hasn't yet happened when the code is compiled. I might have to look at replacing just stdin during compilation. It will require messing with the globals dict but should be possible.

Alternatively we could replace input with a function that raises an exception when used. (That should theoretically be ok for all problems, as it's always an error to call input at global scope when we haven't explicitly asked for it, in which case their code is wrapped in a function.)

In the mean time, the student can just delete his code in the local answers directory.

On Thursday, March 12, 2015, Peter Robinson notifications@github.com wrote:

For this code

def square(n): return n*n n = input("enter num ")

When you press F5 MPT freezes and unfortunately if the student has synced this then every time they choose this problem MPT freezes.

I thought we had fixed this problem. We should add a check for the use of input in problems that don't require it in the static analysis but we should fix the check independently of this.

— Reply to this email directly or view it on GitHub https://github.com/CSSE1001/MyPyTutor/issues/155.

pjritee commented 9 years ago

Yes it's silly but it's sort of the reverse of the problem we used to have - now that the early problems have input in them students then think they have to keep using it somehow. This is the problem with testing student code - it's hard to anticipate all the strange things that students will do - especially students who have done no programming before. It's partly the reason why using static analysis has evolved over time - when I saw a "silly" way of doing something I would try to add a static analysis test. Obviously, this will continue to evolve.

jgat commented 9 years ago

Why not take the place where we replace sys.stdin with a StringIO(''), and do that before the "compilation" step?

sapi commented 9 years ago

We need to replace sys.stdin etc on a per-test basis (so that we can show the correct output for each individual test), but compilation only happens once per test class. We can't replace sys.stdin with its final incarnation at the time of compilation, as we don't know what the value of the StringIO should be (and that value will differ for each test).

This is exactly the kind of thing that the Alarm timeout should be catching, though, so I want to work out why that's not working first.

sapi commented 9 years ago

Doh. I always forget that KeyboardInterrupt doesn't inherit from Exception...

Actually forget that. It is being caught :/

sapi commented 9 years ago

So the root cause of the problem is that the KeyboardInterrupt from the Alarm is not being caught properly, but is instead being raised to tk, without any traceback:

Traceback (most recent call last):
  File "/usr/local/Cellar/python3/3.4.1_1/Frameworks/Python.framework/Versions/3.4/lib/python3.4/tkinter/__init__.py", line 1482, in __call__
    def __call__(self, *args):
KeyboardInterrupt
sapi commented 9 years ago

My guess is that this is a threading issue; the Alarm class (which was carried over from the old MPT without changes) interrupts the main thread, but it's possible that that doesn't work during exec or compile.

If we replace the student's code with an infinite loop inside the function, then the alarm triggers the interrupt successfully. The same behaviour is observed with an infinite loop at global scope, though...

Maybe it's something peculiar to input and/or blocking IO?

pjritee commented 9 years ago

Is it possible to go the other way and always provide "some input" so that if the student uses the input function something will happen? Ah - maybe that's what the StringIO comment was about

sapi commented 9 years ago

Not really. The input function is seeing the real sys.stdin at that point, and I don't really want to mess with that.

It might be easier just to replace input during compilation with:

def _input(prompt):
    raise InvalidInputError(
        'You asked for user input with the prompt {!r}, but no input was expected.'.format(prompt)
    )
pjritee commented 9 years ago

What does compilation do?

sapi commented 9 years ago

It's the call to

exec(compile(code_text, '<student_code>', 'exec'), lcls)
pjritee commented 9 years ago

Ah - does this mean in Python3 exec doesn't work on a string representing code?

sapi commented 9 years ago

It probably does, but I don't think that would make much difference here.

I think all that compile does for our purposes is let us have nicer tracebacks (by being able to 'name' the compiled code)

pjritee commented 9 years ago

What if the question requires the use of input and compilation "replaces" input by _input. I guess you get a chance later on to override the defn of _input if required - is that what you are getting at?

sapi commented 9 years ago

Yeah. Have a look at the changes in db8df62 to see.

pjritee commented 9 years ago

OK - a bit tricky but I get it.

sapi commented 9 years ago

Sadly less tricky than getting the students not to do this! :)

pjritee commented 9 years ago

Just tested the updated MPT on the offending problem and it seems to work fine. Also works as before on problem that has input.