Improving exception handling for debugging

[festutz]

When debugging the annotations framework, it is sometimes a bit annoying that all exceptions are caught but using -assert_compiler_success is not possible for all scripts. I suppose debugging for other parts might have similar issues. Here, I try to identify the underlying root cause and suggest a possible approach to handle exceptions in a better way.

Current Situation

The flag --assert_compiler_success can be used to require that compilation is successful. Success solely requires that no exceptions was caught (at a certain program point) and does not claim anything about parallelization. For the test cases in script_microbenchmarks from compiler/test_evaluation_scripts.py, some parts of the regions simply cannot be parallelized, e.g. because of side effects and the "compiler"/parallelization transformations do not fail here in the sense of something has actually gone wrong. Nevertheless, running pa.sh with --assert_compiler_success fails and does not output anything since having side effects yields an exception that is caught at the same point as other exceptions. For debugging, it would be good to be able distinguish why the transformation failed: because of "unparallelizability" (I) (like side effects) or other issues like IndexError or ValueError (II).

Suggestion

We could define a dedicated class for Exceptions from (I), throw these at the respective places and catch them before catching all other exceptions which would also cover (II). It seems to make sense to change the behavior of --assert_compiler_success to only report exceptions (II).

To get reports for (I) when debugging, one could simply uncomment/introduce traceback.print_exc(), which is basically what I do currently for all exceptions, or we could add another flag for that as well.

[angelhof]

It seems that this is an issue with many people that are trying to extend pash. I don't have time to do this currently but if anyone wants to get involved it shouldn't be a very hard fix. One approach would be to follow Felix's proposal but maybe always catch all errors normally and only when in debug mode letting type II errors be thrown to the user. Happy to give more context w.r.t. that!

[YUUU23]

Hello!

I started working on this issue a bit by:

• adding on a custom exception class called UnparallelizableError in custom_error.py
• changing some of the general Exceptions(message) to this custom exception in ir.py, pash_compiler.py and ast_to_ir.py
• catch UnparallelizableError before all other Exceptions in the compile_ir function from pash_compiler.py

Here is also the pull request for the above: https://github.com/binpash/pash/pull/720

Currently, with --debug running PaSh and some of the testing scripts in the repo, these UnparallelizableError will be logged differently than general exceptions by adding on "Exception caught because some region(s) were unparallelizable : specific reason... "

I'm not entirely sure regarding if:

• as suggested above, --assert_compiler_success should report type (II) errors as in it should not exit with error or return as a error response when only UnparallelizableError are caught?
• there seems to be some custom exceptions already in scripts like expand.py, which now seems to be caught in the general Exception case (type (II)). Should these exceptions be considered in the UnparallelizableError case? On top of this, I am not sure if I know where every general exception that has to do with unparallelizability is located to change all those general exceptions to UnparallelizableError.

I may not be on the right track regarding any of these, but more context on the above and some clarifications may be helpful for resolving this issue!

Thank you!! : )

[angelhof]

Great work @YUUU23 !! I left some comments in the PR! I think it is pretty good other than the comments that I left. Regarding your two questions:

• I think --assert_compiler_success should indeed only report type II errors, but if we do that we should also add a new flag that checks for both type I and type II errors, i.e., whether all regions of a script were parallelizable, e.g., --assert_all_regions_parallelizable. The reason would be to not regress in our tests, since in several tests we check for that property using the original flag. This could also be done in a separate PR btw
• Regarding expansion errors, the custom ones should all be UnparallelizableErrors (if we don't manage to expand a region then we don't have any chance to parallelize it. I would make all the custom errors in that file subclasses of an ExpansionError class and then catch all ExpansionErrors similarly to how you do with the UnparallelizableError ones! If you think this is too much work for this PR, feel free to do it in a different one.

[YUUU23]

Thank you so much for all the suggestions @angelhof !! I really appreciate them! I've fixed most of the comments (with question on one of them) and added the ExpansionErrors class all in this new PR that is based off of the future branch instead of main.

For the suggestion regarding the flag, I want to clarify if the vision is to basically carry the function of --assert_compiler_success to a new flag --assert_all_regions_parallelizable and modify --assert_compiler_success be the flag where only type II errors are reported?

[angelhof]

For the suggestion regarding the flag, I want to clarify if the vision is to basically carry the function of --assert_compiler_success to a new flag --assert_all_regions_parallelizable and modify --assert_compiler_success be the flag where only type II errors are reported?

Yes that is exactly right!

[angelhof]

I left a new review! The changes look great now. The final change that needs to happen is to do the changes to expand.py in the proper sh-expand repo instead of in PaSh (https://github.com/binpash/sh-expand).

[YUUU23]

Got it! Thank you so much!! I left a question on the sh-expand comment you provided in the PR for you when you have time to take a look : )

[angelhof]

Thanks, I responded to your questions!

[YUUU23]

@angelhof Thanks for the patience on the --assert_all_regions_parallelizable flag! I opened a PR for this with an attempt on adding this flag and changing the --assert_compiler_success to the expected behavior outlined above. I would love any feedbacks for changes if needed anytime you are available :) Thanks again!