Langsupport: Validation refactoring and better language support for autograder test cells.

[sigurdurb]

• Refactors repeated code. The code could maybe be refactored even more.
• Better validation of errors, now supports Gnu Octave.
  To get the kernel name I use lang = nb.metadata.get('kernelspec',{}).get("language","python"). Even though it says python it gets the real name of the kernel. So 'octave' is returned for the octave kernel for example. So contributers can now use the lang variable to do custom error handling for their kernel if it does not support the default functionality.
  Octave kernel error matching now works, The code for this is in is_validate_error in utils.py, Octave does not have a cell output_type 'error' but we re.match the error message from the "text" output that has the "stream" output_type. That is the only way I found that works for Octave. Just don't put display('error: ') in a your autograder test cell as a teacher, those cells are read only for students anyway so for students that should not be a problem.

[sigurdurb]

Continuation of PR:#1038 and Issue: #1039

[jhamrick]

Thanks for the work you put into this!

However, I'm not sure this is something that I want to merge into master as is, as I would really like to keep all of the code in nbgrader as language agnostic as possible, and I would like to avoid any special casing for particular languages.

It seems to me that this is more of an issue with the octave kernel: it should be returning errors using the "error" output type rather than a stream output. I guess though for some kernels it might be challenging to do this (e.g. for bash)---really all you have is the stream output. In that case, I would be ok with merging a PR which checks to see if there is a stream with "name": "stderr" and if so treating that as an error, i.e. something like:

def get_errors(cell):
    errors = []
    for output in cell.outputs:
        if output.output_type == "stream" and output.name == "stderr":
            errors.append(output.text)
        elif output.output_type == 'error':
            errors.append(output.traceback)
    return errors

However, I do not want to merge something that checks for a language type explicitly.

Also, it unfortunately looks like these changes have undone some other stuff surrounding the logic of self.validate_all. The logic of _get_failed_cells and _get_passed_cells is legitimately different so I don't think it makes sense to factor that out into a shared method. Would you mind undoing those changes, and just making changes individually in those two methods for appropriately handling errors?

[sigurdurb]

There were two use cases with this PR:

• If there is a use case use case to having a variable for language, "lang" being passed to determine_grade. But if nbgrader is language agnostic it would probably not be used in that function unless it is just an option for people to make their own custom changes more easily for kernels like octave.

• A general refactoring PR could be useful. The utils.is_validate_error function I added refactored the error checks for:

for output in cell.outputs:
    if output.output_type == "error":
         errors.append("\n".join(output.traceback))

That were in utils.determine_grade and validator._extract_error (That now also have a check for partial credit)

is_validate_error first checked if the language was 'octave'. Also in _extract_error there is a check for 'octave' because there the error is in the cell text but else it is in the cell traceback. But it is not ideal to have octave hardcoded in two places.

But if anyone wants to make language support for weird kernels they could use this code for where to implement changes.

[sigurdurb]

This code would need to be separated into several PR's. Going to close this PR and refer it to people who are interest in custom support for languages.

[williamstein]

I have no choice but to deal with this same problem in CoCalc (because paying customers expect to be able to use nbgrader + octave). In case you're curious, here's how it works there (like you, I check for error: at the beginning of the line):

https://github.com/sagemathinc/cocalc/blob/master/src/smc-webapp/jupyter/nbgrader/autograde.ts#L219

Note that this is Typescript code, rather than Python.

Also, note that there are subtle similar problems that happen when using the R kernel.

[williamstein]

I also agree that this is all working around bugs in these kernels. However, I'm not optimistic that these bugs will be fixed in a timely manner.