Replaced reuse_addr with something more insisting #2757

BjarneHerland · 2022-01-23T21:32:53Z

Issue
Resolves #2728

Approach
Renamed ™reuse_addr" to "will_close_then_reopen_socket" to make users of this flag conscious of why it is there.

Added documentation with reasoning and cleaned up some rather clunky logic in find_available_port(). Cleaned up unnecessary use of the flag in tests and code.

Pre review checklist

Added appropriate labels

BjarneHerland · 2022-01-23T21:36:08Z

In particular I would appreciate input on the approach and docs.

codecov-commenter · 2022-01-23T22:13:38Z

Codecov Report

Merging #2757 (f826b75) into main (f20fec4) will increase coverage by 0.14%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##             main    #2757      +/-   ##
==========================================
+ Coverage   65.20%   65.35%   +0.14%     
==========================================
  Files         652      652              
  Lines       53630    53502     -128     
  Branches     4818     4752      -66     
==========================================
- Hits        34970    34965       -5     
+ Misses      17058    16934     -124     
- Partials     1602     1603       +1

Impacted Files	Coverage Δ
ert_shared/ensemble_evaluator/config.py	`95.40% <ø> (ø)`
ert_shared/services/_storage_main.py	`24.77% <0.00%> (ø)`
ert_shared/ensemble_evaluator/ensemble/prefect.py	`90.58% <100.00%> (+0.05%)`	⬆️
ert_shared/port_handler.py	`100.00% <100.00%> (+3.84%)`	⬆️
ert_gui/ertwidgets/validationsupport.py	`79.45% <0.00%> (-19.18%)`	⬇️
ert_gui/ertwidgets/__init__.py	`75.00% <0.00%> (-2.28%)`	⬇️
ert3/console/_console.py	`88.29% <0.00%> (-1.26%)`	⬇️
ert3/workspace/_workspace.py	`89.24% <0.00%> (-0.76%)`	⬇️
libres/lib/res_util/matrix.cpp	`65.40% <0.00%> (-0.28%)`	⬇️
libres/lib/enkf/enkf_node.cpp	`52.16% <0.00%> (-0.26%)`	⬇️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f20fec4...f826b75. Read the comment docs.

frode-aarstad · 2022-01-24T08:04:50Z

Looks good. The comment is informative and helps explain the different issues with sockets in the implementation.
I guess its not easy to create a test testing the fix since its mostly seen on clusters.

ert_shared/ensemble_evaluator/ensemble/prefect.py

oysteoh · 2022-01-24T08:44:01Z

ert_shared/ensemble_evaluator/ensemble/prefect.py

@@ -57,14 +57,15 @@ async def _eq_submit_job(self, script_filename):


 def _get_executor(custom_port_range, name="local"):
-    _, port, _ = find_available_port(custom_range=custom_port_range, reuse_addr=True)
    if name == "local":
        cluster_kwargs = {
            "silence_logs": "debug",


what is the reason of the removal of the port when running locally?

I don't think the LocalDaskExecutor runs a server, hence does not need a port

It does (I think), but when running locally we are not restricted by infrastructure firewall 🙂

Ahh - ok. So it's free to pick whatever port id wants then, which it does when we do not provide a port-number.

BjarneHerland · 2022-01-24T15:29:03Z

Following up on @frode-aarstad 's challenge 😄 I started to write tests for this and discovered quite a lot of funky behaviour.

Hopefully these tests with docs adequately describes current behaviour and provides users of this module some insight into what to expect...

frode-aarstad · 2022-01-25T08:02:48Z

Following up on @frode-aarstad 's challenge 😄 I started to write tests for this and discovered quite a lot of funky behaviour.

Hopefully these tests with docs adequately describes current behaviour and provides users of this module some insight into what to expect...

Looks very good. Tested and worked fine on WSL2

ert_shared/ensemble_evaluator/ensemble/prefect.py

oysteoh · 2022-01-25T09:00:00Z

Looks very good - also good with more tests verifying the behavior! 👏

joakim-hove · 2022-01-25T09:46:23Z

@BjarneHerland : I tried to find an issue/PR with questions of thread-pool; could not find now. Easiest to reach me on email joakim.hove AT opm-op.com Might be a bit tight today. Cool if you post a link to the issue/PR here. Thank you.

BjarneHerland · 2022-01-25T11:12:50Z

test ert please

BjarneHerland · 2022-01-25T12:08:38Z

test ctest please

oysteoh · 2022-01-25T12:50:26Z

@BjarneHerland : I tried to find an issue/PR with questions of thread-pool; could not find now. Easiest to reach me on email joakim.hove AT opm-op.com Might be a bit tight today. Cool if you post a link to the issue/PR here. Thank you.

@joakim-hove the issue you are looking for is located here #2650

BjarneHerland · 2022-01-26T07:42:45Z

test this please

awaiting further improvements wrt testing

BjarneHerland · 2022-01-26T20:37:47Z

test ert please

BjarneHerland · 2022-01-27T21:50:50Z

@oysteoh , @sondreso mind taking a look? I hope to merge in the (narrow) window between green tests and need for rebase... 😄

oysteoh

Very good! Nice with good documented testing verifying the behavior of this! 🚀

sondreso

LGTM! Only had a small question

ert_shared/ensemble_evaluator/ensemble/prefect.py

Rename "reuse_addr" to "will_close_then_reopen_socket" to make users of this flag conscious about why it is there. Added reasoning and cleaned up some rather clunky logic in find_available_port(). Cleaned up unnecessary use of the flag in tests and code. Added extensive tests.

BjarneHerland · 2022-02-01T07:31:08Z

If no other questions or concerns, I'll just merge this one. Would be interesting, though, to identify one or two tests crucial in this context.

BjarneHerland added the flaky-test label Jan 23, 2022

BjarneHerland requested review from frode-aarstad and mortalisk January 23, 2022 21:33

BjarneHerland force-pushed the remove_reuse_addr_argument branch from 68aeca8 to eefdf1c Compare January 23, 2022 21:35

oysteoh reviewed Jan 24, 2022

View reviewed changes

ert_shared/ensemble_evaluator/ensemble/prefect.py Outdated Show resolved Hide resolved

oysteoh reviewed Jan 24, 2022

View reviewed changes

BjarneHerland force-pushed the remove_reuse_addr_argument branch from eefdf1c to e662866 Compare January 24, 2022 15:24

BjarneHerland requested review from sondreso and oysteoh January 24, 2022 15:29

oysteoh reviewed Jan 25, 2022

View reviewed changes

ert_shared/ensemble_evaluator/ensemble/prefect.py Outdated Show resolved Hide resolved

oysteoh reviewed Jan 25, 2022

View reviewed changes

ert_shared/ensemble_evaluator/ensemble/prefect.py Outdated Show resolved Hide resolved

BjarneHerland force-pushed the remove_reuse_addr_argument branch from e662866 to ccedbea Compare January 25, 2022 08:56

oysteoh previously approved these changes Jan 25, 2022

View reviewed changes

BjarneHerland force-pushed the remove_reuse_addr_argument branch from ccedbea to 05ebcff Compare January 26, 2022 15:27

BjarneHerland requested a review from oysteoh January 26, 2022 15:27

BjarneHerland force-pushed the remove_reuse_addr_argument branch from 05ebcff to 773c653 Compare January 26, 2022 15:32

oysteoh approved these changes Jan 28, 2022

View reviewed changes

sondreso reviewed Jan 28, 2022

View reviewed changes

ert_shared/ensemble_evaluator/ensemble/prefect.py Outdated Show resolved Hide resolved

BjarneHerland force-pushed the remove_reuse_addr_argument branch from 773c653 to f826b75 Compare January 28, 2022 10:35

BjarneHerland force-pushed the remove_reuse_addr_argument branch from f826b75 to fa4f4d9 Compare January 28, 2022 15:06

BjarneHerland requested review from sondreso and oysteoh February 1, 2022 07:31

BjarneHerland merged commit d6cd336 into equinor:main Feb 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replaced reuse_addr with something more insisting #2757

Replaced reuse_addr with something more insisting #2757

BjarneHerland commented Jan 23, 2022

BjarneHerland commented Jan 23, 2022

codecov-commenter commented Jan 23, 2022 •

edited

frode-aarstad commented Jan 24, 2022

oysteoh Jan 24, 2022

BjarneHerland Jan 24, 2022

sondreso Jan 24, 2022 •

edited

BjarneHerland Jan 24, 2022

BjarneHerland commented Jan 24, 2022

frode-aarstad commented Jan 25, 2022 •

edited

oysteoh commented Jan 25, 2022

joakim-hove commented Jan 25, 2022

BjarneHerland commented Jan 25, 2022

BjarneHerland commented Jan 25, 2022

oysteoh commented Jan 25, 2022

BjarneHerland commented Jan 26, 2022

BjarneHerland commented Jan 26, 2022

BjarneHerland commented Jan 27, 2022

oysteoh left a comment

sondreso left a comment

BjarneHerland commented Feb 1, 2022

Replaced reuse_addr with something more insisting #2757

Replaced reuse_addr with something more insisting #2757

Conversation

BjarneHerland commented Jan 23, 2022

Pre review checklist

BjarneHerland commented Jan 23, 2022

codecov-commenter commented Jan 23, 2022 • edited

Codecov Report

frode-aarstad commented Jan 24, 2022

oysteoh Jan 24, 2022

Choose a reason for hiding this comment

BjarneHerland Jan 24, 2022

Choose a reason for hiding this comment

sondreso Jan 24, 2022 • edited

Choose a reason for hiding this comment

BjarneHerland Jan 24, 2022

Choose a reason for hiding this comment

BjarneHerland commented Jan 24, 2022

frode-aarstad commented Jan 25, 2022 • edited

oysteoh commented Jan 25, 2022

joakim-hove commented Jan 25, 2022

BjarneHerland commented Jan 25, 2022

BjarneHerland commented Jan 25, 2022

oysteoh commented Jan 25, 2022

BjarneHerland commented Jan 26, 2022

BjarneHerland commented Jan 26, 2022

BjarneHerland commented Jan 27, 2022

oysteoh left a comment

Choose a reason for hiding this comment

sondreso left a comment

Choose a reason for hiding this comment

BjarneHerland commented Feb 1, 2022

codecov-commenter commented Jan 23, 2022 •

edited

sondreso Jan 24, 2022 •

edited

frode-aarstad commented Jan 25, 2022 •

edited