Randomize solver document listing #2625

mayaCostantini · 2022-04-12T12:50:04Z

Related Issues and Dependencies

Related to thoth-station/thoth-application#2513

make sure the sync method in thoth-storages can randomize document listing so that we can run multiple sync jobs in parallel

This introduces a breaking change

No

Randomizing the solver documents listing is optional.

This should yield a new module release

Yes

This Pull Request implements

Introduce an optional argument to the sync_solver_documents method in storages to randomize solver document listing.

fridex · 2022-04-13T07:43:59Z

thoth/storages/sync.py

    processed, synced, skipped, failed = 0, 0, 0, 0
-    for document_id in document_ids or solver_store.get_document_listing():
+    for document_id in document_ids or document_listing:


Please shuffle both to be consistent.

fridex · 2022-04-13T07:44:53Z

thoth/storages/sync.py

 ) -> tuple:
    """Sync solver documents into graph."""
+    document_listing = list(solver_store.get_document_listing())


This looks like an unnecessary step if document_ids is provided.

fridex · 2022-04-13T07:44:53Z

thoth/storages/sync.py

 ) -> tuple:
    """Sync solver documents into graph."""
+    document_listing = list(solver_store.get_document_listing())


This looks like an unnecessary step if document_ids is provided.

fridex · 2022-04-13T07:45:34Z

thoth/storages/sync.py

@@ -98,8 +99,11 @@ def sync_solver_documents(
    graceful: bool = False,
    graph: Optional[GraphDatabase] = None,
    is_local: bool = False,
+    randomize_listing: Optional[bool] = False,


Can we have this configurable via environment variables on a library level? That way we can control this behavior directly from deployment manifests.

fridex · 2022-04-13T07:45:34Z

thoth/storages/sync.py

@@ -98,8 +99,11 @@ def sync_solver_documents(
    graceful: bool = False,
    graph: Optional[GraphDatabase] = None,
    is_local: bool = False,
+    randomize_listing: Optional[bool] = False,


Can we have this configurable via environment variables on a library level? That way we can control this behavior directly from deployment manifests.

mayaCostantini · 2022-04-13T08:41:49Z

Thanks for the review @fridex 💯 Should we also randomize the listing in other sync methods?

fridex · 2022-04-13T10:34:11Z

Thanks for the review @fridex 100 Should we also randomize the listing in other sync methods?

+1, that would be great

Gregory-Pereira · 2022-04-14T22:46:55Z

Will we need to write a separate script that will do this conversion /migration for everything currently in the DB?

Gregory-Pereira · 2022-04-15T18:52:40Z

Also shouldn't this micro version be bumped with this feature.

fridex · 2022-04-13T07:43:59Z

thoth/storages/sync.py

    processed, synced, skipped, failed = 0, 0, 0, 0
-    for document_id in document_ids or solver_store.get_document_listing():
+    for document_id in document_ids or document_listing:


Please shuffle both (if shuffling is enabled) to be consistent.

fridex · 2022-04-19T12:51:24Z

thoth/storages/sync.py

@@ -38,6 +39,7 @@
 from .graph import GraphDatabase

 _LOGGER = logging.getLogger(__name__)
+RANDOMIZE_LISTING = os.getenv("RANDOMIZE_LISTING", 0)


All the environment variables in Thoth share THOTH_ prefix, ideally stating which component or library should take the configuration:

Suggested change

RANDOMIZE_LISTING = os.getenv("RANDOMIZE_LISTING", 0)

_RANDOMIZE_LISTING = bool(int(os.getenv("THOTH_STORAGES_RANDOMIZE_LISTING", 0)))

Also, no need to have this public outside of the module.

fridex · 2022-04-19T12:54:09Z

thoth/storages/sync.py

    processed, synced, skipped, failed = 0, 0, 0, 0
-    for document_id in document_ids or adviser_store.get_document_listing():
+    for document_id in document_ids or document_listing:


The snippet above might be prone to programmatic errors as the if-else is basically done twice. It might be a good idea to be explicit and state which listing is actually used just once:

Suggested change

for document_id in document_ids or document_listing:

if _RANDOMIZE_LISTING:

listing = random.shuffle(document_ids or document_listing)

for document_id in listing:

fridex · 2022-04-25T17:01:39Z

thoth/storages/sync.py

@@ -61,8 +63,13 @@ def sync_adviser_documents(
        adviser_store = AdvisersResultsStore()
        adviser_store.connect()

+    listing = list(adviser_store.get_document_listing()) or document_ids


Can we cast the generator to a list only if randomization is on? I know it might be nitpicking, but in some cases it might improve perf :)

No you're right, thanks! 💯 Added in 03acd31

fridex

lgtm, thanks! 💯 👍🏻

sesheta · 2022-04-25T21:48:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fridex

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [fridex]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Randomize solver document listing

b7f9bac

sesheta requested review from harshad16 and KPostOffice April 12, 2022 12:50

sesheta added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 12, 2022

fridex reviewed Apr 13, 2022

View reviewed changes

randomizing listing in env variable and fixes

60fafe0

Randoomize document listing in all sync methods

bbbe8e1

sesheta added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 13, 2022

fridex reviewed Apr 19, 2022

View reviewed changes

Post review fixes

9538b06

fridex reviewed Apr 25, 2022

View reviewed changes

Cast to list if randomizing

03acd31

fridex approved these changes Apr 25, 2022

View reviewed changes

sesheta added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 25, 2022

sesheta merged commit 1ce78b4 into thoth-station:master Apr 25, 2022

mayaCostantini deleted the randomize-document-listing branch April 26, 2022 05:59

Gregory-Pereira mentioned this pull request Apr 28, 2022

[3pt] Create sync method in thoth-storages can randomize document listing thoth-station/thoth-application#2529

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Randomize solver document listing #2625

Randomize solver document listing #2625

mayaCostantini commented Apr 12, 2022

fridex Apr 13, 2022

fridex Apr 13, 2022

fridex Apr 13, 2022

fridex Apr 13, 2022

fridex Apr 13, 2022

mayaCostantini commented Apr 13, 2022

fridex commented Apr 13, 2022

Gregory-Pereira commented Apr 14, 2022

Gregory-Pereira commented Apr 15, 2022

fridex Apr 13, 2022

fridex Apr 19, 2022

fridex Apr 19, 2022 •

edited

Loading

fridex Apr 25, 2022

mayaCostantini Apr 25, 2022

fridex left a comment

sesheta commented Apr 25, 2022

	RANDOMIZE_LISTING = os.getenv("RANDOMIZE_LISTING", 0)
	_RANDOMIZE_LISTING = bool(int(os.getenv("THOTH_STORAGES_RANDOMIZE_LISTING", 0)))

-    for document_id in document_ids or document_listing:
+    if _RANDOMIZE_LISTING:
+        listing = random.shuffle(document_ids or document_listing)
+    for document_id in listing:

Randomize solver document listing #2625

Randomize solver document listing #2625

Conversation

mayaCostantini commented Apr 12, 2022

Related Issues and Dependencies

This introduces a breaking change

This should yield a new module release

This Pull Request implements

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayaCostantini commented Apr 13, 2022

fridex commented Apr 13, 2022

Gregory-Pereira commented Apr 14, 2022

Gregory-Pereira commented Apr 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fridex Apr 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fridex left a comment

Choose a reason for hiding this comment

sesheta commented Apr 25, 2022

fridex Apr 19, 2022 •

edited

Loading