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

Set initial state in gui for forward model steps #9864

Merged
merged 2 commits into from
Jan 29, 2025

Conversation

larsevj
Copy link
Contributor

@larsevj larsevj commented Jan 24, 2025

Introduces a new state equal to pending which is the initial state of a forward model step. When we receive a fm_step_start event we should instead display running.

This also allows for modifying the logic in fm_step_reporter such that it does not send a running event immediately after a start event. This should reduce the number of events that we receive from jobs that run < 5 seconds.

  • 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 unit tests pass locally after every commit (git rebase -i main --exec 'pytest tests/ert/unit_tests tests/everest -n auto --hypothesis-profile=fast -m "not integration_test"')

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

Copy link

codspeed-hq bot commented Jan 24, 2025

CodSpeed Performance Report

Merging #9864 will not alter performance

Comparing larsevj:set_pre_start_state_in_gui (3a0898f) with main (3b6f51e)

Summary

✅ 25 untouched benchmarks

@larsevj larsevj self-assigned this Jan 24, 2025
@larsevj larsevj force-pushed the set_pre_start_state_in_gui branch 2 times, most recently from 56a53b3 to 444d6d0 Compare January 24, 2025 13:41
Copy link
Contributor

@jonathan-eq jonathan-eq left a comment

Choose a reason for hiding this comment

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

Very good change! Just a small question, see comment 👍

@larsevj larsevj added the release-notes:maintenance Automatically categorise as maintenance change in release notes label Jan 29, 2025
@@ -126,7 +127,7 @@ def test_memory_profile_is_logged_as_csv():
with open(scriptname, "w", encoding="utf-8") as script:
script.write(
"""#!/bin/sh
exit 0
sleep 6
Copy link
Contributor

Choose a reason for hiding this comment

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

Why sleep 6?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To get the running event, and therefore the memory data. But should probably mock the memory poll interval value instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would go for mocking the poll interval, yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how to fix this, since we launch the reporter in another process so is it possible at all to mock the value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by calling main function instead of using subprocess.

@xjules
Copy link
Contributor

xjules commented Jan 29, 2025

Very nice PR @larsevj !
Some general comments:

  • I'd suggest to improve the commit messages explaining why new state is introduced and why the first running msg is postponed
  • have you looked at tests/ert/unit_tests/forward_model_runner/test_event_reporter.py if this can be a place to include a test for the new state?

@larsevj larsevj force-pushed the set_pre_start_state_in_gui branch 2 times, most recently from 176811d to c429eb4 Compare January 29, 2025 12:06
Copy link
Contributor

@jonathan-eq jonathan-eq left a comment

Choose a reason for hiding this comment

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

Very good! Have you tested manually with bigpoly?

…del steps

- Introduces an initial state for the forward model steps
 equal to "pending" to differentiate from the starting state
- Changes the starting state from "pending" to "running" since
 we already start populating the duration field in the gui.
…step

- This avoids sending the running message directly after sending the
 start message. For short running fm_steps this will avoid sending 3
 events in short succesion. Downside is that this will not provide the
 metadata from the running message such as memory consumption, but this
 is observed as unreliable for short running jobs anyway.
@larsevj larsevj force-pushed the set_pre_start_state_in_gui branch from c429eb4 to 3a0898f Compare January 29, 2025 12:47
@larsevj
Copy link
Contributor Author

larsevj commented Jan 29, 2025

Very good! Have you tested manually with bigpoly?

Can confirm that this has been tested manually with bigpoly, reduces running time by ~10-20%, and cannot see much difference in gui behaviour apart from missing max memory usage for the small running steps.

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 @larsevj ! Let's ship it 🚀

@larsevj larsevj merged commit 30f2fb9 into equinor:main Jan 29, 2025
27 checks passed
@larsevj larsevj deleted the set_pre_start_state_in_gui branch January 29, 2025 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:maintenance Automatically categorise as maintenance change in release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants