Skip to content

Conversation

yngve-sk
Copy link
Contributor

@yngve-sk yngve-sk commented May 23, 2025

Issue
Resolves #10684
Resolves #9059

Approach
Make separate endpoints for params and responses

PS: Will look into adding webviz ert plots into ert plotter to not maintain backward compat. The removals of some endpoints in this PR can be considered blocked by that.

@yngve-sk yngve-sk force-pushed the 25.05.remove-records branch from dc8d853 to e648a5c Compare May 23, 2025 12:44
@yngve-sk yngve-sk force-pushed the 25.05.remove-records branch 5 times, most recently from 815dc92 to febf012 Compare June 4, 2025 13:41
@yngve-sk yngve-sk self-assigned this Jun 5, 2025
@yngve-sk yngve-sk force-pushed the 25.05.remove-records branch 6 times, most recently from 82c8ce6 to 4d8d655 Compare June 6, 2025 11:17
@equinor equinor deleted a comment from codspeed-hq bot Jun 6, 2025
@yngve-sk yngve-sk force-pushed the 25.05.remove-records branch 9 times, most recently from 3885865 to 30e88fa Compare June 11, 2025 12:40
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 like a nice improvement! I have some questions, but they might not be introduced in this PR, and as such could be fixed in follow up issues.

@yngve-sk yngve-sk force-pushed the 25.05.remove-records branch 3 times, most recently from 5c4c839 to 1c5d629 Compare June 17, 2025 07:42
@equinor equinor deleted a comment from codspeed-hq bot Jun 18, 2025
@yngve-sk yngve-sk force-pushed the 25.05.remove-records branch from b4a931b to 4b7d2d9 Compare June 18, 2025 08:26
@yngve-sk yngve-sk force-pushed the 25.05.remove-records branch from 4b7d2d9 to 19e2334 Compare June 18, 2025 08:27
Copy link

codspeed-hq bot commented Jun 18, 2025

CodSpeed Performance Report

Merging #10941 will improve performances by ×6

Comparing yngve-sk:25.05.remove-records (19e2334) with main (6288f33)

Summary

⚡ 2 improvements
✅ 20 untouched benchmarks
⁉️ 4 (👁 4) dropped benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
👁 test_direct_dark_performance[gen_x: 20, sum_x: 20 reals: 10-gen_data-get_parameters] 254.3 µs N/A N/A
👁 test_direct_dark_performance[gen_x: 20, sum_x: 20 reals: 10-gen_data_with_obs-get_parameters] 252.6 µs N/A N/A
👁 test_direct_dark_performance[gen_x: 20, sum_x: 20 reals: 10-summary-get_parameters] 258.8 µs N/A N/A
👁 test_direct_dark_performance[gen_x: 20, sum_x: 20 reals: 10-summary_with_obs-get_parameters] 254.2 µs N/A N/A
test_direct_dark_performance_with_storage[gen_x: 20, sum_x: 20 reals: 10-gen_data-get_record_observations] 1,530.7 µs 255.4 µs ×6
test_direct_dark_performance_with_storage[gen_x: 20, sum_x: 20 reals: 10-summary-get_record_observations] 1,441.5 µs 253.4 µs ×5.7

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.

LGTM! One minor comment

Comment on lines +116 to +117
assert response_key is not None
assert response_type is not None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can they actually be None? Or is the typing wrong?

Copy link
Contributor Author

@yngve-sk yngve-sk Jun 18, 2025

Choose a reason for hiding this comment

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

Suggest deferring to separate issue to remove the None typing.
Webviz-ert fails when it is removed, giving this:

  File "webviz-ert/webviz_ert/data_loader/__init__.py", line 307, in compute_misfit
    response_key, report_step = response_name.split("@", 1)
    ^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: not enough values to unpack (expected 2, got 1)

, and these tests fail:
test_data_for_response_doesnt_mistake_history_for_response
test_plot_api_handles_urlescape

Screenshot 2025-06-18 at 14 17 15

@yngve-sk yngve-sk merged commit 816d8e8 into equinor:main Jun 18, 2025
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get rid of records concept from dark storage Bug/error-prone GUI-dark_storage "*H" requests control flow
2 participants