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.

Sign up for GitHub

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

Jump to bottom

Have GUI output callback error on realization hover #8398

Merged
merged 1 commit into from
Aug 16, 2024
Merged

Have GUI output callback error on realization hover #8398

merged 1 commit into from
Aug 16, 2024

Conversation

jonathan-eq
Copy link
Contributor

@jonathan-eq jonathan-eq commented Aug 6, 2024 •
edited
Loading

Issue
Resolves #3578

Approach
Short description of the approach

(Screenshot of new behavior in GUI if applicable)
image

  • PR title captures the intent of the changes, and is fitting for release notes.
  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Make sure tests pass locally (after every commit!)

When applicable

  • When there are user facing changes: Updated documentation
  • New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
  • Large PR: Prepare changes in small commits for more convenient review
  • Bug fix: Add regression test for the bug
  • Bug fix: Create Backport PR to latest release

@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 61.90476% with 8 lines in your changes missing coverage. Please review.

Project coverage is 90.62%. Comparing base (24d9d2d) to head (3b954e8).
Report is 1 commits behind head on main.

Files Patch % Lines
src/ert/gui/simulation/view/realization.py 14.28% 6 Missing ⚠️
src/ert/gui/model/snapshot.py 60.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #8398      +/-   ##
==========================================
- Coverage   90.64%   90.62%   -0.02%     
==========================================
  Files         344      344              
  Lines       20970    20987      +17     
==========================================
+ Hits        19008    19020      +12     
- Misses       1962     1967       +5
Flag Coverage Δ
gui-tests 75.91% <57.14%> (-0.01%) ⬇️
integration-tests 53.90% <52.38%> (+<0.01%) ⬆️
unit-tests 68.61% <57.14%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jonathan-eq jonathan-eq force-pushed the improve-callback-status branch from 07c2a08 to 786b0bf Compare August 7, 2024 06:51
@oyvindeide
Copy link
Collaborator

Think this looks like a good improvement! But dont think it solves the issue it is linked to? If there is an issue in the callback, that does not end up in the job error file, but is passed back as an event.

@jonathan-eq
Copy link
Contributor Author

Think this looks like a good improvement! But dont think it solves the issue it is linked to? If there is an issue in the callback, that does not end up in the job error file, but is passed back as an event.

Oh, then I misunderstood the issue. Thanks for clarifying 👍

@xjules
Copy link
Contributor

xjules commented Aug 8, 2024

Think this looks like a good improvement! But dont think it solves the issue it is linked to? If there is an issue in the callback, that does not end up in the job error file, but is passed back as an event.

Oh, then I misunderstood the issue. Thanks for clarifying 👍

Yes, it should be

ert/src/ert/scheduler/job.py

Line 248 in 6b7b218

self._callback_status_msg += f"\nstatus from done callback: {status_msg}"

@jonathan-eq jonathan-eq force-pushed the improve-callback-status branch from 786b0bf to 224ae75 Compare August 13, 2024 10:16
@jonathan-eq jonathan-eq changed the title Have GUI output stderr on realization hover Have GUI output callback error on realization hover Aug 13, 2024
@jonathan-eq
Copy link
Contributor Author

image

@jonathan-eq jonathan-eq force-pushed the improve-callback-status branch from 224ae75 to 4fd95e0 Compare August 13, 2024 10:40
@jonathan-eq jonathan-eq force-pushed the improve-callback-status branch 2 times, most recently from b8152b7 to 2c1769b Compare August 15, 2024 05:59
xjules
xjules reviewed Aug 15, 2024
View reviewed changes
src/ert/ensemble_evaluator/snapshot.py
@@ -113,7 +113,9 @@ def __init__(self) -> None:
] = defaultdict(dict)
"""A shallow dictionary of realization states. The key is a string with
realization number, pointing to a dict with keys active (bool),
start_time (datetime), end_time (datetime) and status (str)."""
start_time (datetime), end_time (datetime), callback_status_message (str) and status (str).
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

@xjules
Copy link
Contributor

xjules commented Aug 15, 2024

It looks very good! Just not sure if there should be a test for callback_status_message, maybe at least that the event contains data["callback_status_message"]?

@jonathan-eq jonathan-eq force-pushed the improve-callback-status branch 2 times, most recently from 5255f02 to 390bdf6 Compare August 15, 2024 13:18
xjules
xjules approved these changes Aug 16, 2024
View reviewed changes
Copy link
Contributor

@xjules xjules left a comment

Choose a reason for hiding this comment

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

Nice work @jonathan-eq! 🚀

@jonathan-eq jonathan-eq force-pushed the improve-callback-status branch from 390bdf6 to b29c611 Compare August 16, 2024 11:42
@jonathan-eq jonathan-eq force-pushed the improve-callback-status branch from b29c611 to 3b954e8 Compare August 16, 2024 12:00
@jonathan-eq jonathan-eq merged commit 5d016d9 into equinor:main Aug 16, 2024
35 checks passed
@jonathan-eq jonathan-eq deleted the improve-callback-status branch August 16, 2024 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@berland berland berland left review comments

@xjules xjules xjules approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pass callback status as event and show in GUI
5 participants
@jonathan-eq @codecov-commenter @oyvindeide @xjules @berland