Skip to content
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

Improve error message when ACE fails to start #181

Closed
goodmami opened this issue Sep 26, 2018 · 4 comments
Closed

Improve error message when ACE fails to start #181

goodmami opened this issue Sep 26, 2018 · 4 comments
Assignees
Milestone

Comments

@goodmami
Copy link
Member

Instantiating an AceProcessor with an old grammar image currently results in a stack trace with the error "Broken pipe" which hides the error from ACE. The delphin.interfaces.ace.AceProcess._open() method should check if the opened process terminated and raise a more appropriate error (this may require a new exception class in delphin.exceptions)

@goodmami goodmami added this to the v1.0.0 milestone Nov 14, 2018
@mcmillanmajora
Copy link
Contributor

I think I have a solution for this by checking if the Popen object's poll() value is not None, but I'm not sure what kind of file I would need write to replicate an old grammar image for testing.

@goodmami
Copy link
Member Author

Good :) And that sounds about right. Checking the poll() value is how I detect crashes in the _result_lines() method of AceProcess. As for testing, look how I use monkeypatch (a pytest thing) to override stdin in tests/commands_test.py. You can use this pattern with the expected output from ACE (the "version mismatch..." line below). It would be nice if you could also override the return code (shown by the $? variable in bash), but I'm not sure offhand how to do that.

goodmami@tpy:~/grammars$ ace -g erg-1214-x86-64-0.9.27.dat 
version mismatch: this is ACE version 0.9.29, but this grammar image was compiled by ACE version 0.9.27
goodmami@tpy:~/grammars$ echo "$?"
255

Also, I'm not sure if you've looked at the v1.0.0 branch yet, but I've moved to have a fairly minimal delphin.exceptions with module-specific exceptions defined in the modules themselves (in case you created a custom exception for this ACE error). Let me know if you have questions about this.

@goodmami
Copy link
Member Author

@mcmillanmajora could you submit a PR? It should be easier to figure out the remaining issues that way.

@goodmami goodmami assigned goodmami and unassigned mcmillanmajora Feb 18, 2019
@goodmami
Copy link
Member Author

Assigning to myself. I have a fix in my branch.

@goodmami goodmami modified the milestones: v1.0.0, v0.9.2 Feb 18, 2019
goodmami added a commit that referenced this issue Apr 1, 2019
resolves #180
resolves #181
resolves #186
fixes #200
fixes #203
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

No branches or pull requests

2 participants