Skip to content

Remove event_from_dict function and ZMQ client from LegacyEnsemble #10634

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

Merged
merged 2 commits into from
Apr 23, 2025

Conversation

jonathan-eq
Copy link
Contributor

@jonathan-eq jonathan-eq commented Apr 15, 2025

Approach
This first commit in this PR removes the event_from_dict function and makes the callers create the Events directly instead. This increases the readability and makes it easier to find where the events are actually being created without having to jump through hoops and doing eight spins.

The second commit makes the LegacyEnsemble put events directly in the evaluator queue (which it has access to) instead of connecting to EE via ZMQ client and sending events. Before this commit, the client it used was also called dispatcher, which caused some confusion as to who was actually creating and sending Ensemble{Started,Cancelled,Stopped}.

(Screenshot of new behavior in GUI if applicable)

  • 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 'just rapid-tests')

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: Add backport label to latest release (format: 'backport release-branch-name')

@jonathan-eq jonathan-eq self-assigned this Apr 15, 2025
@jonathan-eq jonathan-eq added the release-notes:skip If there should be no mention of this in release notes label Apr 15, 2025
@jonathan-eq jonathan-eq moved this to In Progress in SCOUT Apr 15, 2025
@jonathan-eq jonathan-eq force-pushed the remove_legacy_workaround branch from c440e5b to a17dd24 Compare April 15, 2025 07:58
@jonathan-eq jonathan-eq moved this from In Progress to Ready for Review in SCOUT Apr 15, 2025
@jonathan-eq jonathan-eq marked this pull request as ready for review April 15, 2025 08:06
Copy link

codspeed-hq bot commented Apr 15, 2025

CodSpeed Performance Report

Merging #10634 will not alter performance

Comparing jonathan-eq:remove_legacy_workaround (5de884e) with main (36eb826)

Summary

✅ 25 untouched benchmarks

@jonathan-eq jonathan-eq force-pushed the remove_legacy_workaround branch from a17dd24 to 0b33cce Compare April 22, 2025 06:11
@jonathan-eq jonathan-eq force-pushed the remove_legacy_workaround branch from 0b33cce to 2f604c2 Compare April 22, 2025 08:44
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 improvement @jonathan-eq ! 🚀

@github-project-automation github-project-automation bot moved this from Ready for Review to Reviewed in SCOUT Apr 22, 2025
This commit changes LegacyEnsemble to put events directly in scheduler queue rather than sending it over ZMQ.
@jonathan-eq jonathan-eq force-pushed the remove_legacy_workaround branch from 2f604c2 to 5de884e Compare April 23, 2025 06:43
@jonathan-eq jonathan-eq enabled auto-merge (rebase) April 23, 2025 07:00
@jonathan-eq jonathan-eq merged commit 4d288fd into equinor:main Apr 23, 2025
26 checks passed
@github-project-automation github-project-automation bot moved this from Reviewed to Done in SCOUT Apr 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:skip If there should be no mention of this in release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants