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

NodesTreeWidget: Red icon for failed processes #336

Merged
merged 4 commits into from
Aug 5, 2022

Conversation

danielhollas
Copy link
Contributor

This has bugged me for a while. We were displaying green icon for processes that finished but had non-zero exit status.

BEFORE:
obrazek

AFTER:
obrazek

@yakutovicha yakutovicha requested a review from csadorf August 2, 2022 13:24
@yakutovicha
Copy link
Member

I let @csadorf review it, as he mainly contributed to this part of the code.

@csadorf csadorf added the bug Something isn't working label Aug 4, 2022
Copy link
Member

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

Generally no objections, but it seems a bit odd that we need to special case this. Do you know what the value of process_node.process_state is in case that the process_node.is_failed condition is True?

@danielhollas
Copy link
Contributor Author

danielhollas commented Aug 4, 2022

It is unfortunate, but the ProcessState is FINISHED in this case so there's no way to distinguish this condition based on this. Note that also the method 'is_finished' returns true if the process finished with non-zero exit status. 'is_finished_ok' method is used to really check whether there where no issues.

I am pretty sure I saw some bugs related to this in the aiidalab-qe app as well, wil laubmit PRs when I get to it.

danielhollas and others added 2 commits August 4, 2022 16:23
Co-authored-by: Carl Simon Adorf <carl.simon.adorf@gmail.com>
@csadorf
Copy link
Member

csadorf commented Aug 4, 2022

@danielhollas Sorry, looks like I introduced a bug with my suggestion: https://results.pre-commit.ci/run/github/135423835/1659623008.H_oLFNBBSs-SpAAAzru51A

@danielhollas
Copy link
Contributor Author

@csadorf thanks, I've fixed the code and tested locally so this should be good to go.

@csadorf csadorf merged commit 758e4f2 into aiidalab:master Aug 5, 2022
@csadorf
Copy link
Member

csadorf commented Aug 5, 2022

@danielhollas Thanks a lot!

@danielhollas danielhollas deleted the failed-node branch August 13, 2022 15:47
danielhollas added a commit to ispg-group/aiidalab-ispg that referenced this pull request Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants