Add marine_pool by assafrabin · Pull Request #19 · tomlegkov/marine-python

assafrabin · 2020-04-02T19:29:02Z

No description provided.

yehonatanz

In addition to previous comments, I'd like to see at least one test dedicated to marine pool.

yehonatanz · 2020-04-05T07:14:36Z

tests/fixtures/marine/marine_fixtures.py

+    return MarinePool(marine_so_path)
+
+
+@pytest.fixture(scope="session", params=[False, True], ids=["marine", "marine_pool"])


Wouldn't it be more straight forward to pass the fixture names as params and use request.getfixturevalue?

I don't think so. You add a line and a parameter (request) for each test

I meant in here, not in every test

yehonatanz · 2020-04-05T07:16:34Z

marine/marine_pool.py

+            return []
+
+        ctx = multiprocessing.get_context("spawn")
+        pool = ctx.Pool(self.process_count)


Are you spawning a pool each time filter_and_parse is called?
Please don't

yehonatanz · 2020-04-05T07:18:01Z

marine/marine_pool.py

+        if len(packets) == 0:
+            return []
+
+        ctx = multiprocessing.get_context("spawn")


Why not just use pool = multiprocessing.Pool(count)?

The default fork_method is "fork" and not "spawn". I assumed @tomlegkov chose spawn for a reason...

Yeah, he explained it on slack.
Add a comment explaining it so some wise-ass won't change it in the future without understanding.

yehonatanz · 2020-04-05T07:19:43Z

marine/marine_pool.py

+        ctx = multiprocessing.get_context("spawn")
+        pool = ctx.Pool(self.process_count)
+        chunk_size = int(math.ceil(len(packets) / float(self.process_count)))
+        packet_chunks = [


multiprocessing.Pool.map already splits the input to batches for you and exposes a clean API.

yehonatanz · 2020-04-05T07:21:05Z

marine/__init__.py

@@ -1,4 +1,5 @@
 from .marine import Marine
+from .marine_pool import MarinePool


<NotSerious>
MarinePool should be called Lagoon
</NotSerious>

yehonatanz

I had some notes.
I still think that make init_marine idempotent on the C level (with a is_initialized flag or something) is the best option though.
@tomlegkov What do you think?

yehonatanz · 2020-04-05T16:38:00Z

marine/marine_pool.py

+
+
+class MarinePool:
+    _marine_instance = None


`_marine_instance: ClassVar[Optional[Marine]] = None

yehonatanz · 2020-04-05T16:39:45Z

marine/marine_pool.py

+        )
+
+    @staticmethod
+    def _init_marine(lib_path):


Switch to classmethod if Pool supports it.

yehonatanz · 2020-04-05T16:40:21Z

marine/marine_pool.py

+        ctx = multiprocessing.get_context("spawn")
+        # Using spawn so child processes won't get the already initialized marine from the parent process.
+        self.pool = ctx.Pool(self.process_count)
+        self.pool.map(MarinePool._init_marine, repeat(self._lib_path, self.process_count))


Pool accepts an initializer argument, exactly for that reason

yehonatanz · 2020-04-05T16:40:57Z

marine/marine_pool.py

+        MarinePool._marine_instance = Marine(lib_path)
+
+    @staticmethod
+    def _filter_and_parse(*args):


Switch to classmethod if Pool supports it.

yehonatanz · 2020-04-05T16:43:01Z

tests/fixtures/marine/marine_fixtures.py

+    return MarinePool(marine_so_path)
+
+
+@pytest.fixture(scope="session", params=[False, True], ids=["marine", "marine_pool"])


I meant in here, not in every test

tomlegkov

Please also add MarinePool to the benchmark

tomlegkov · 2020-04-06T20:10:23Z

marine/marine_pool.py

+
+    def __init__(self, lib_path: str, process_count: int = 4):
+        self._lib_path = lib_path
+        self.process_count = process_count


Why is process_count not private? Do we want to support editing it outside of the constructor?

marine/marine_pool.py

tomlegkov · 2020-04-06T20:12:22Z

marine/marine_pool.py

+        MarinePool._marine_instance = Marine(lib_path)
+
+    @staticmethod
+    def _filter_and_parse(*args):


What's the advantage of using *args over explicitly stating the parameters?
Also missing return type

tomlegkov · 2020-04-06T20:12:49Z

marine/marine_pool.py

+        )
+
+    @staticmethod
+    def _init_marine(lib_path):


tomlegkov · 2020-04-06T20:17:26Z

To add to what @yehonatanz said, I think what's missing is a complete test for MarinePool, showing that packets stay sorted even when some don't pass the filter (maybe with different process counts), etc.

tests/marine/test_marine_pool.py

assafrabin requested review from tomlegkov and yehonatanz April 2, 2020 19:29

tomlegkov linked an issue Apr 4, 2020 that may be closed by this pull request

Add option to run Marine in a process pool #9

Closed

yehonatanz requested changes Apr 5, 2020

View reviewed changes

assafrabin force-pushed the feature/marine_pool branch from e3de20f to a53b964 Compare April 5, 2020 15:49

yehonatanz requested changes Apr 5, 2020

View reviewed changes

tomlegkov requested changes Apr 6, 2020

View reviewed changes

Assaf Rabinowitz added 3 commits April 19, 2020 12:50

Add marine_pool

542cf7e

Fixed CR

56e8e70

CR Fixes + black

7166079

yehonatanz approved these changes Apr 19, 2020

View reviewed changes

Add test_marine_pool_preserves_order

fb729fd

assafrabin force-pushed the feature/marine_pool branch from 5dfc53a to fb729fd Compare April 19, 2020 10:18

tomlegkov requested changes Apr 19, 2020

View reviewed changes

tests/marine/test_marine_pool.py Outdated Show resolved Hide resolved

tests/marine/test_marine_pool.py Show resolved Hide resolved

Fix marine pool test according to CR

2044729

tomlegkov approved these changes Apr 19, 2020

View reviewed changes

Align marine_pool to epan_auto_reset_count addition

8b5c429

assafrabin force-pushed the feature/marine_pool branch from 2c41cec to 8b5c429 Compare April 19, 2020 18:07

assafrabin merged commit 7807c76 into master Apr 19, 2020

tomlegkov deleted the feature/marine_pool branch February 5, 2021 18:43

		return MarinePool(marine_so_path)


		@pytest.fixture(scope="session", params=[False, True], ids=["marine", "marine_pool"])

		@@ -1,4 +1,5 @@
		from .marine import Marine
		from .marine_pool import MarinePool

Conversation

assafrabin commented Apr 2, 2020

Uh oh!

yehonatanz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yehonatanz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomlegkov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomlegkov commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tomlegkov commented Apr 6, 2020 •

edited

Loading