-
Notifications
You must be signed in to change notification settings - Fork 39
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
Issue 632 #720
Conversation
…t was unparallelizable or general other exceptions in the compiler through providing more specific log messages
…t was unparallelizable or general other exceptions in the compiler through providing more specific log messages
OS = |
OS:ubuntu-20.04 |
compiler/pash_compiler.py
Outdated
@@ -92,10 +93,13 @@ def compile_ir(ir_filename, compiled_script_file, args, compiler_config): | |||
ret = None | |||
try: | |||
ret = compile_optimize_output_script(ir_filename, compiled_script_file, args, compiler_config) | |||
except UnparallelizableError as e: | |||
log("WARNING: Exception caught because some region(s) are unparallelizable:", e) | |||
log(traceback.format_exc()) # get detailed reports for (I) when debugging, one could simply uncomment/introduce traceback.print_exc() ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this we might prefer to leave that commented and make sure to pass all the necessary information in the custom error message. The hope would be that a PaSh user does not see exception traces if something is unparellizable, but rather sees an informative message.
except Exception as e: | ||
log("WARNING: Exception caught:", e) | ||
# traceback.print_exc() | ||
|
||
log(traceback.format_exc()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this will now happen less often it might be OK to let the traceback be logged.
compiler/ir.py
Outdated
@@ -960,7 +961,7 @@ def introduce_aggregators_for_consec_chunks(self, fileIdGen, in_aggregator_ids, | |||
# TODO: turn node into cmd_invocation_with_io_vars since this is the only thing required in this function | |||
self.create_generic_aggregator_tree(original_cmd_invocation_with_io_vars, parallelizer, map_in_aggregator_ids, out_aggregator_id, fileIdGen) | |||
else: | |||
raise Exception("aggregator kind not yet implemented") | |||
raise UnparallelizableError("aggregator kind not yet implemented") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add more information in this message. For example, which command does this refer to?
compiler/ir.py
Outdated
|
||
def apply_round_robin_parallelization_to_node(self, node_id, parallelizer, fileIdGen, fan_out, | ||
r_split_batch_size): | ||
# TODO: this control flow should move done to aggregators once we implement them; | ||
# currently, this cannot be done since splitter etc. would be added... | ||
aggregator_spec = parallelizer.get_aggregator_spec() | ||
if aggregator_spec.is_aggregator_spec_adj_lines_merge(): | ||
raise Exception("adj_lines_merge not yet implemented in PaSh") | ||
raise UnparallelizableError("adj_lines_merge not yet implemented in PaSh") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 3 errors are actually general exceptions. Or rather NotImplemented errors, so I would make a new error type and catch them separately.
compiler/ir.py
Outdated
@@ -741,19 +742,19 @@ def apply_parallelization_to_node(self, node_id, parallelizer, fileIdGen, fan_ou | |||
elif splitter.is_splitter_consec_chunks(): | |||
self.apply_consecutive_chunks_parallelization_to_node(node_id, parallelizer, fileIdGen, fan_out) | |||
else: | |||
raise Exception("Splitter not yet implemented") | |||
raise UnparallelizableError("Splitter not yet implemented") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, add which command the splitter doesn't exist for.
if io_info.has_other_outputs(): | ||
raise Exception(f"Command {format_arg_chars(command)} has outputs other than streaming.") | ||
raise UnparallelizableError(f"Command {format_arg_chars(command)} has outputs other than streaming.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also print what is the other output?
compiler/ast_to_ir.py
Outdated
@@ -141,7 +142,7 @@ def combine_pipe(ast_nodes): | |||
else: | |||
## If any part of the pipe is not an IR, the compilation must fail. | |||
log("Node: {} is not pure".format(ast_nodes)) | |||
raise Exception('Not pure node in pipe') | |||
raise UnparallelizableError('Not pure node in pipe') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I would include the node in the message.
compiler/ast_to_ir.py
Outdated
@@ -132,7 +133,7 @@ def combine_pipe(ast_nodes): | |||
else: | |||
## If any part of the pipe is not an IR, the compilation must fail. | |||
log("Node: {} is not pure".format(ast_nodes[0])) | |||
raise Exception('Not pure node in pipe') | |||
raise UnparallelizableError('Not pure node in pipe') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would include more information here (like the node), even though it is logged above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything other than the comments looks good. I would suggest rebasing your commits on future
instead of main
though. This is where we actually merge new changes (and every now and then we merge on main
.
…t_to_ir.py, change errors to be logged when general exceptions are caught and not unparallelizable ones, introduce AdjLineNotImplementedError
OS = |
OS = |
OS:ubuntu-20.04 |
OS:ubuntu-20.04 |
Signed-off-by: Forthoney <[email protected]>
Signed-off-by: Forthoney <[email protected]>
Signed-off-by: Forthoney <[email protected]>
Signed-off-by: Forthoney <[email protected]>
Signed-off-by: Forthoney <[email protected]>
Signed-off-by: Anirudh Narsipur <[email protected]>
Signed-off-by: Forthoney <[email protected]>
Signed-off-by: Forthoney <[email protected]>
Signed-off-by: Forthoney <[email protected]>
Pin pash-annotations version
Signed-off-by: Evangelos Lamprou <[email protected]>
OS = |
OS:ubuntu-20.04 |
#632