feat: --assert_all_regions_parallelizable flag that has the same func… #722
Conversation
YUUU23
force-pushed
the
assert-all-parallelizable-region-flag
branch
from
cd4c265 to
349efe6
Compare
|
OS:ubuntu-20.04 |
|
OS = |
|
LGTM :-) |
compiler/cli.py
Outdated
| ) | ||
| self.add_argument( | ||
| "--assert_all_regions_parallelizable", | ||
| help="assert that the compiler succeeded with all regions being parallelizable and no general error occuring (used to make tests more robust)", |
There was a problem hiding this comment.
Maybe we can add a comment that this is strictly stricter than --assert_compiler_success
compiler/pash_compilation_server.py
Outdated
| if compile_success: | ||
| response = server_util.success_response( | ||
| f"{process_id} {compiled_script_file} {var_file} {input_ir_file}" | ||
| ) | ||
| elif not all_region_parallelizable: | ||
| # send specified message to say not all regions are parallelizable instead of general exception caught | ||
| response = server_util.error_response(f"{process_id} not all regions are parallelizable; failed to compile") |
There was a problem hiding this comment.
The response can be more specific, and slightly reworded. At this point we know precisely which region is not parallelizable, and we can respond with that (instead of a more generic "not all regions"). Also, the all_region_parallelizable flag could be renamed to current_region_parallelizable or parallelizability_success since it refers to a specific region and not all regions.
There was a problem hiding this comment.
would it be okay to response with "current region" along with the process_id? I'm not entirely sure how to refer to a region
Thank you for the reviews!
|
OS = |
|
OS:ubuntu-20.04 |
…tion as old assert_compiler_success flag; new assert_compiler_success flag now will not exit with error when regions are unparallelizable and exit with error when general exceptions are caught Signed-off-by: YUUU23 <[email protected]>
Signed-off-by: YUUU23 <[email protected]>
YUUU23
force-pushed
the
assert-all-parallelizable-region-flag
branch
from
9e67a36 to
536bd8e
Compare
|
OS = |
|
OS:ubuntu-20.04 |
|
Thank you for this fix!! |
To introduce the
--assert_all_regions_parallelizableflag (should exit with error when bothUnparallelizableErrorand general exception are caught) and changing the original--assert_compiler_successflag to exiting with error only when general exceptions are caught (does not exit with error whenUnparallelizableErrorare caught), the following were added:compiler/orchestrator_runtime/pash_init_setup.sh,compiler/cli.py: set-up and add information regarding--assert_all_regions_parallelizableflag for pash to recognizecompiler/custom_error.py: introduceNotAllRegionParallelizableErrorto be raised whenUnparallelizableErrorare caught in thecompile.irfunction inpash/compiler/pash_compiler.py;NotAllRegionParallelizableErrorwill be caught incompiler/pash_compilation_server.pywhen thecompile.irfunction is called; if this error is caught, the new variableall_region_parallelizableincompiler/pash_compilation_server.pywill be marked falseerror_responsewith a different error message is saved in the compiler serverassert_compiler_successflag is on andcompile_successis false butall_region_parallelizableis true (some general exceptions caught) ; orassert_all_regions_parallelizableflag on andcompile_successis false because of both general exceptions or unparallelizable regioncompiler/orchestrator_runtime/pash_prepare_call_compiler.sh: marks variablepash_all_region_parallelizableiferror_responsecontains message regarding regions not being parallelizable. We should exit on error when,assert_all_regions_parallelizableflag on andruntime_return_code != 0: general exceptionassert_all_regions_parallelizableflag on andpash_all_region_parallelizable != 0: some region is not parallelizableassert_compiler_success_flagflag on andruntime_return_code != 0andpash_all_region_parallelizable == 0: general exception (and not error because some regions were not parallelizable)evaluation/tests/test_evaluation_scripts.sh: when executing tests, change--assert_compiler_successto--assert_all_regions_parallelizable, since what assert_compiler_success originally does is now carried out by--assert_all_regions_parallelizableTested the above for expected behavior by running some intro scripts in the directory, and it does seem like we are exiting with error for
--assert_compiler_successonly when general exceptions are caught and not the unparallelizable ones. We are also exiting on error for both reasons when--assert_all_regions_parallelizableflag is on. When both flags are used at the same time, we follow the behavior of--assert_all_regions_parallelizable. PaSh testing scripts locally also seems to pass all tests.