Skip to content

Graceful message and exit for wrong value of phase#84

Merged
sonalgoyal merged 3 commits into
zinggAI:mainfrom
navinrathore:zPhaseValues
Dec 23, 2021
Merged

Graceful message and exit for wrong value of phase#84
sonalgoyal merged 3 commits into
zinggAI:mainfrom
navinrathore:zPhaseValues

Conversation

@navinrathore
Copy link
Copy Markdown
Contributor

#81 wrong value of phase shows stack trace - should be handled gracefully

Copy link
Copy Markdown
Member

@sonalgoyal sonalgoyal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cant we just throw an exception and exit with the right message?

private ClientOptions options;

public static final Log LOG = LogFactory.getLog(Client.class);
public static final int EXIT_STATUS_ILLEGAL_CMD = 127;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not needed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

System.exit(0);
}
String phase = options.get(ClientOptions.PHASE).value.trim();
if (ZinggOptions.getByValue(phase) == null) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw exception with right message

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the message as part of thrown exception
Apologies for this message. Zingg has encountered an error. 'findTrainingDat' is not a valid phase. Valid phases are: train|match|trainMatch|findTrainingData|label|link

int exitStatusExpected = 127; // error code for an illegal command e.g. typo
try {
String zinggInvalidPhaseCmd = "scripts/zingg.sh --phase findTrainingDat --conf examples/febrl/config.json";
Process p = Runtime.getRuntime().exec(zinggInvalidPhaseCmd, null, new File(System.getenv("ZINGG_HOME")));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we just call the client with the args instead of execing a new process here? and simply assert that an exception happens ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testcases have been updated to test the new functionality that is refactored into ZinggOptions.verifyPhase(phase).
ZinggOptions has been chosen for it is the central place to maintain zingg options or phases.

@sonalgoyal sonalgoyal merged commit 53c05ff into zinggAI:main Dec 23, 2021
@navinrathore navinrathore deleted the zPhaseValues branch January 3, 2022 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants