Exception handling in PipeUtil::read() #229

navinrathore · 2022-04-29T11:34:58Z

Here is a replaced Fraft PR to handle PipeUtil::Read() exception handling.

The ZinggClientException has been used for the purpose. Should there be different exception used? If yes, it should better be derived from Exception
The two variations. read() and readWithException() may not be needed. As our code is already handling these scenario using catching "Exception" and do nothing. single read() with exception is fine.
Use of ZinggClientException in function signatures is done to satisfy exception throw mechanism and compiler
error messages like below is printed on cli.

zingg.client.Client - Apologies for this message. Zingg has encountered an error. Path does not exist: file:/home/work/product/final/zingg-1/models/900/trainingData/marked

Please provide your inputs on the approach.
Note: Though major flows have been tested. Still more testing required.

…rn null never.

sonalgoyal · 2022-04-29T12:03:38Z

core/src/main/java/zingg/LabelUpdater.java

@@ -41,12 +41,12 @@ public void execute() throws ZinggClientException {

 	public void processRecordsCli(Dataset<Row> lines) throws ZinggClientException {
 		LOG.info("Processing Records for CLI updateLabelling");
-		getMarkedRecordsStat(lines);
-		printMarkedRecordsStat();
 		if (lines == null || lines.count() == 0) {


we need to have a clean flow without returns - please put in if lines != null as discussed earlier

Removed Return.
Earlier thought that following code block was already very big. It should not be cluttered further.

sonalgoyal · 2022-04-29T12:05:40Z

core/src/main/java/zingg/Labeller.java

@@ -75,11 +75,11 @@ protected void getMarkedRecordsStat(Dataset<Row> markedRecords) {

 	public void processRecordsCli(Dataset<Row> lines) throws ZinggClientException {
 		LOG.info("Processing Records for CLI Labelling");
-		printMarkedRecordsStat();


see earlier comment about returns

removed return statement.

sonalgoyal · 2022-04-29T12:06:04Z

core/src/main/java/zingg/Labeller.java

@@ -51,7 +51,7 @@ public Dataset<Row> getUnmarkedRecords() throws ZinggClientException {
 			unmarkedRecords = PipeUtil.read(spark, false, false, PipeUtil.getTrainingDataUnmarkedPipe(args));
 			try {
 				markedRecords = PipeUtil.read(spark, false, false, PipeUtil.getTrainingDataMarkedPipe(args));
-			} catch (Exception e) {


dont we still need to catch the other expcetions?

read() throws only ZinggClientException.
Earlier, why didnot read() mandate to ctch Exception everywhere it is called. Is there any difference in Exception and ZinggClientException?

yes ZCE extends from throwable

sonalgoyal · 2022-04-29T12:06:46Z

core/src/main/java/zingg/Matcher.java

@@ -41,11 +41,12 @@ public Matcher() {
        setZinggOptions(ZinggOptions.MATCH);
    }

-	protected Dataset<Row> getTestData() {
-		return PipeUtil.read(spark, true, args.getNumPartitions(), true, args.getData());
+	protected Dataset<Row> getTestData() throws ZinggClientException{


why this change?

To satisfy the requirement of catching or throwing the ZinggClientException. All the intermediate functions are mandating to add this.
In this regard, are types Exception and ZinggClientException different?

sonalgoyal · 2022-04-29T12:12:15Z

core/src/main/java/zingg/util/PipeUtil.java

-			input = reader.load();
-		}
-		if (addSource) {
-			input = input.withColumn(ColName.SOURCE_COL, functions.lit(p.getName()));			


if we throw an exception here, how will we handle cases where we dont want exceptions - eg findtrainingData

We aren't throwing any new exception here. Catching one and throwing another.
So if an exception is already thrown (that is indeed done in spark.read() in this case), it is handled somewhere (first catch block). So with this change, there will not be any impact in program behavior. Just things made explicit to have the exception handled by the caller. the specific type of the exception. Else, we are loosing the error message in the process.

Still, we have to see which one to use 'Exception' or 'ZinggClientException' in the flow.
We are already handling Exception in top level classes. Why are we loosing ex.getMessage(). I note that intermediate function calls do not h ave explicit exception specification.

ok, makes sense. but the message here has to be that we could not read the and also wrap the actual exception thrown

Used wrapped exception.

… of functions

sonalgoyal · 2022-05-10T19:31:15Z

core/src/main/java/zingg/LabelUpdater.java

-		getMarkedRecordsStat(lines);
-		printMarkedRecordsStat();
-		if (lines == null || lines.count() == 0) {
-			LOG.info("There is no marked record for updating. Please run findTrainingData/label jobs to generate training data.");


we need to still print this out in else, no?

Yes. made changes in Labeller and UpdateLabeller.

…or empty

core/src/main/java/zingg/util/PipeUtil.java

Pipe's props is initialized with blank hashmap. makes getProps() never return null

navinrathore added 2 commits April 29, 2022 16:49

Exception handling in PipeUtil::read()

fe052df

Pipe's props is initialized with blank hashmap. makes getProps() retu…

4619542

…rn null never.

navinrathore mentioned this pull request May 2, 2022

Pipe's props is initialized with blank hashmap. makes getProps() never return null navinrathore/zingg-1#2

Merged

sonalgoyal reviewed May 2, 2022

View reviewed changes

Changed to wrapped ZinggClientException; Removed 'return' from middle…

191f810

… of functions

navinrathore marked this pull request as ready for review May 5, 2022 10:39

sonalgoyal requested changes May 10, 2022

View reviewed changes

In Labeller & UpdateLabeller, added 'else' for input records be null …

086a29f

…or empty

sonalgoyal reviewed May 16, 2022

View reviewed changes

core/src/main/java/zingg/util/PipeUtil.java Show resolved Hide resolved

sonalgoyal and others added 2 commits May 16, 2022 10:44

Merge branch 'main' into zProperErrors

9ea14e4

Merge pull request #2 from navinrathore/pipeUtilProps

b4d292a

Pipe's props is initialized with blank hashmap. makes getProps() never return null

sonalgoyal merged commit cb39e21 into zinggAI:main May 16, 2022

navinrathore mentioned this pull request May 18, 2022

PipeUtil should check for null props #211

Closed

navinrathore deleted the zProperErrors branch June 1, 2022 04:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exception handling in PipeUtil::read() #229

Exception handling in PipeUtil::read() #229

navinrathore commented Apr 29, 2022

sonalgoyal Apr 29, 2022

navinrathore May 5, 2022

sonalgoyal Apr 29, 2022

navinrathore May 5, 2022

sonalgoyal Apr 29, 2022

navinrathore May 5, 2022

sonalgoyal May 5, 2022

sonalgoyal Apr 29, 2022

navinrathore May 5, 2022

sonalgoyal Apr 29, 2022

navinrathore May 2, 2022

sonalgoyal May 2, 2022

navinrathore May 5, 2022

sonalgoyal May 10, 2022

navinrathore May 11, 2022

Exception handling in PipeUtil::read() #229

Exception handling in PipeUtil::read() #229

Conversation

navinrathore commented Apr 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment