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

Use ropt events from queue #10213

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Use ropt events from queue #10213

wants to merge 1 commit into from

Conversation

frode-aarstad
Copy link
Contributor

Issue
Resolves #9957

Approach
Short description of the approach

(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: Create Backport PR to latest release

@frode-aarstad frode-aarstad added the release-notes:improvement Automatically categorise as improvement in release notes label Mar 5, 2025
@frode-aarstad frode-aarstad self-assigned this Mar 5, 2025
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Mar 5, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #10213 will improve performances by 12.69%

Comparing frode-aarstad:ropt-events (e28db96) with main (122dae5)

Summary

⚡ 1 improvements
✅ 24 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
⚡ test_direct_dark_performance_with_storage[gen_x: 20, sum_x: 20 reals: 10-summary-get_record_observations] 1.4 ms 1.3 ms +12.69%

@frode-aarstad frode-aarstad force-pushed the ropt-events branch 3 times, most recently from 6f3109e to 0548ae8 Compare March 5, 2025 12:31
oyvindeide
oyvindeide reviewed Mar 6, 2025
View reviewed changes
Copy link
Collaborator

@oyvindeide oyvindeide left a comment

Choose a reason for hiding this comment

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

Looks really good! Seeing as there were no tests that picked up that we stopped printing the opt results, it would be good to add a test to the monitor, could build on this: tests/everest/test_monitor.py, and just add another event to the list and check the output.

tests/everest/test_everest_runmodel_events.py Outdated
e.model_dump()
e.model_dump(
exclude={"results"}
) # the results are not that interesting in this context
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will they change, or are they static? If they are static, we would probably like to keep them in to make sure that they are serializable with results in the events?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was just rounding problems. Fixed now

src/everest/detached/__init__.py
@@ -249,6 +249,33 @@ def server_is_running(url: str, cert: str, auth: tuple[str, str]) -> bool:
return True


def get_opt_status_from_batch_result_event(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest removing the old function getting from storage: #10225, so we have full separation between the frontend and the backend. But if you prefer I can wait with my PR to after this and add cleaning up of tests in that PR.

@frode-aarstad frode-aarstad force-pushed the ropt-events branch 3 times, most recently from f59945a to ba51e22 Compare March 10, 2025 07:25
@yngve-sk
Copy link
Contributor

Some open questions:

  1. Do we want to pass through the entire raw result from ROPT, and let ROPT be responsible for giving us the results formatted in the way that is most in line with us?
  2. How will we be using these events in the future? Right now all we would need is total objective and control values I think, in the future.. I'm not sure? Most of it would/should come from storage.
  3. Should we maybe have the same schema for the data within events and in storage?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oyvindeide oyvindeide oyvindeide left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@frode-aarstad frode-aarstad

Labels
release-notes:improvement Automatically categorise as improvement in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use ropt events instead of querying server for optimization status
3 participants
@frode-aarstad @yngve-sk @oyvindeide