daphne-eu / daphne

DAPHNE: An Open and Extensible System Infrastructure for Integrated Data Analysis Pipelines
Apache License 2.0
67 stars 62 forks source link

stop() built-in/op/kernel #658

Closed pdamme closed 5 months ago

pdamme commented 9 months ago

DaphneDSL should offer a stop() built-in function which stops the execution of the script (similar to the one in Apache SystemDS). This would be very helpful, e.g., since it would allow UDFs to react to invalid arguments. In such cases, the program should not simply continue (and printing a warning could easily be overlooked), the program should stop.

Example:

def foo(x:si64) {
    if(x < 0)
        stop("x must not be less than zero");
    return 1 + sqrt(x);
}

We need:

Hints:

Garic152 commented 5 months ago

I'd like to work on this issue.

pdamme commented 5 months ago

Great, thanks. Go ahead!

Garic152 commented 5 months ago

I've just pushed the first version of the stop() function to my fork: Fork: dev-issue-658.

The functionality is as follows:

Custom Input

File:

stop("Custom error");

Output:

[error]: Execution error: The kernel-function _stop__char failed during runtime with the following message [ System stopped: Custom Error ]
    | Source file -> "/daphne/test.daph":1:0
    |
  1 | stop("Custom Error");
    | ^~~

No Input

File:

stop();
[error]: Execution error: The kernel-function _stop__char failed during runtime with the following message [ System stopped: Unspecified error occurred. ]
    | Source file -> "/daphne/test.daph":1:0
    |
  1 | stop();
    | ^~~

When throwing the std::runtime_error, the function does show the failing lines inside the .daph file, but also mentions _stop__char. Should only the second part of the message be printed, and if so, do you have any suggestions on how to implement this?

Apart from that, if there aren't any major concerns in how I've implemented this functionality, I'll continue working on the Python API implementation as well as the unit/script tests.

pdamme commented 5 months ago

Thanks for the update. The error message looks fine as it is for now. The overall format is generated by our error handling facilities. That includes the kernel name (_stop__char), because it's useful information for users and developers. Furthermore, that way, the error message is consistent with other errors. So having the message from the stop() function inside the general error message is totally fine.

Regarding the Python API: Thinking about it again now, adding the stop() function there will most likely not work out at the moment. This is because stop() doesn't have any results (values it returns). Therefore, it will not be included in the generated DaphneDSL script. That is a general limitation of DaphneLib at the moment (see #712), which we will fix later. Thus, you can skip the DaphneLib part for this issue.

The code in your fork looks very good already. The main thing that's still missing are indeed the test cases.

Garic152 commented 5 months ago

The stop() function now works like requested and also includes the unit/script-level tests. The script-level tests are located in ControlFlowTest.cpp, as this seemed like the most logical place.

When running ./test.sh -nb -d yes, I encountered errors in DistributedTest and MatrixLiteralTest. These errors should be unrelated to my recent commit because I didn't change any files related to these tests. If there is a correlation though, please let know.

If there are any issues with my implementation, please let me know.

pdamme commented 5 months ago

Thanks for the update. At a quick glance, the tests in your fork look good. It would be great if you could employ the stop() built-in function in the DaphneDSL scripts as mentioned in the hints above. Other than that, feel free to create a pull request :) .

Regarding the errors: the DistributedTest also sometimes fails on my system (for unknown reasons), but regarding the MatrixLiteralTest, I'm not aware of any problems. But thanks for letting us know; sometimes there are non-deterministic errors that do not occur in all environments. I agree that your changes are unrelated, though.