Move Task queuing to Taskhubs by hmacdope · Pull Request #80 · OpenFreeEnergy/alchemiscale

hmacdope · 2023-02-03T04:36:58Z

Fixes #78

Moves task queuing from a linked list with deterministic ordering to a hub-spoke model with stochastic pickup of tasks based on weights.

@dotsdl one key question is that currently the [:ACTIONS] relationships are built only when the tasks are queued (makes sense), meaning weights cannot be set or interrogated before then. This is not explicitly checked for at this stage but are we happy with this limitation?

codecov-commenter · 2023-02-03T04:48:37Z

Codecov Report

Base: 90.87% // Head: 91.10% // Increases project coverage by +0.22% 🎉

Coverage data is based on head (1abee2a) compared to base (a76613e).
Patch coverage: 95.34% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #80      +/-   ##
==========================================
+ Coverage   90.87%   91.10%   +0.22%     
==========================================
  Files          42       42              
  Lines        3300     3416     +116     
==========================================
+ Hits         2999     3112     +113     
- Misses        301      304       +3

Impacted Files	Coverage Δ
alchemiscale/interface/client.py	`93.63% <ø> (ø)`
alchemiscale/interface/api.py	`50.32% <20.00%> (ø)`
alchemiscale/compute/api.py	`74.24% <77.77%> (ø)`
alchemiscale/storage/statestore.py	`91.90% <92.85%> (+0.44%)`	⬆️
alchemiscale/base/client.py	`97.36% <100.00%> (ø)`
alchemiscale/compute/client.py	`100.00% <100.00%> (ø)`
alchemiscale/compute/service.py	`63.36% <100.00%> (ø)`
alchemiscale/storage/models.py	`90.29% <100.00%> (ø)`
.../integration/compute/client/test_compute_client.py	`100.00% <100.00%> (ø)`
...integration/compute/client/test_compute_service.py	`100.00% <100.00%> (ø)`
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

hmacdope · 2023-02-07T18:53:03Z

@dotsdl did you push the changes from our pair session ?

dotsdl · 2023-02-07T19:41:00Z

Apologies, I didn't. Just pushed now.

dotsdl · 2023-02-07T19:44:29Z

I don't know why I don't see the commits here. ⁉️

dotsdl · 2023-02-07T19:45:21Z

I see them on the branch: https://github.com/hmacdope/alchemiscale/tree/taskhub

dotsdl · 2023-02-07T20:02:25Z

Oh nevermind, it's further up because the commits were 18 hours ago. 🤦

dotsdl · 2023-02-08T03:20:19Z

I can go ahead and fix the merge conflicts here, unless you'd prefer to?

hmacdope · 2023-02-08T03:33:41Z

Yep please go ahead. let me know if anything is unclear as we did a bit of renaming.
After merge conflicts resolved I will go through and fix up the test suite and implement the changes we discussed.

Add error for querying the weights when they are null or unable to be set because the :ACTIONS relationship is not created.
Add a weights interface to action_taskhub_tasks.

dotsdl · 2023-02-08T06:51:10Z

Okay, think I've got the merge conflicts sorted. Seeing test failures in the claiming method in statestore. Can you address?

hmacdope · 2023-02-08T06:53:54Z

Yep for sure.

hmacdope · 2023-02-08T07:09:22Z

@dotsdl My tests are hanging locally, I'll investigate

hmacdope · 2023-02-11T03:45:09Z

-        assert task_sks2 == all_task_sks[1:]
+        assert task_sks[0] in all_task_sks
+
+        # TODO: Currently we only allow tasks to be added to a taskhub with an EXTENDS


This had to be modified due to #84 blocking adding of :EXTENDS tasks using action_tasks. Will be fixed in a separate PR.

I added the first task in the chain for each of the three transformations in the n4js_preloaded fixture, so now we have multiple tasks to work with for this test.

hmacdope · 2023-02-11T04:34:13Z

@dotsdl I think this is probably ready for another review 😃

Simplified where possible, adjustments to docs, grouping queries under transactions, etc.

dotsdl

Great work @hmacdope! Some comments, but none blocking. Pushing my changes noted then proceeding to merge.

dotsdl · 2023-02-13T22:40:34Z

+        depth = lambda L: isinstance(L, list) and max(map(depth, L)) + 1
+        if depth(tasks) > 1:
+            raise ValueError(
+                "tasks must be a flat list of ScopedKeys, list with depth > 1 not allowed"
+            )


Not sure this is strictly necessary, since we say we need a List[ScopedKey] for tasks in the signature; I think this is fast though so should be okay?

Also this is deep in the storage layer, so by this point we shouldn't have to make too many sanity checks here; just adds expense.

Yeah I agree, too internal to need this kind of sanity checking.

dotsdl · 2023-02-14T03:05:36Z

+                SET ar.weight = {w}
+                RETURN ar
+                """
+                with self.transaction() as tx:


Moved transaction outside of loop to use a single transaction for all operations.

dotsdl · 2023-02-14T03:19:07Z

+                with self.transaction() as tx:
+                    tx.run(q)
+
+        # TODO: add exception for nothing returned for matches, or some Tasks missing


Added return of Tasks we set weights for; None for those we did not.

dotsdl · 2023-02-14T04:05:30Z

+
+    def get_task_weights(
+        self, tasks: List[ScopedKey], taskhub: ScopedKey
+    ) -> Dict[ScopedKey, float]:


Switched this over to return List[Union[float, None]] to allow for case where a Task ScopedKey is given but doesn't resolve to an actual Task.

dotsdl · 2023-02-14T04:13:46Z


-    def claim_taskqueue_tasks(
-        self, taskqueue: ScopedKey, claimant: str, count: int = 1
+    def get_taskhub_unclaimed_tasks(


Do we have a use case for this method in mind? I'm fine with leaving it in, but not sure we use it anywhere yet?

Ah, we do use it in tests; this is fine.

I was anticipating it might be useful for tests as you saw, but also possibly if one wanted to query what tasks are still waiting on the TaskHub eg for checks on the total volume of work etc. Can always YAGNI it away if not needed.

dotsdl · 2023-02-14T06:31:25Z

-        assert task_sks2 == all_task_sks[1:]
+        assert task_sks[0] in all_task_sks
+
+        # TODO: Currently we only allow tasks to be added to a taskhub with an EXTENDS


I added the first task in the chain for each of the three transformations in the n4js_preloaded fixture, so now we have multiple tasks to work with for this test.

dotsdl · 2023-02-14T06:36:03Z

        assert all([i is None for i in task_sks_fail])

-    def test_cancel_task(self, n4js, network_tyk2, scope_test):
+    def test_action_task_nested_fails(self, n4js: Neo4jStore, network_tyk2, scope_test):


Not sure we really need this test; see comments on action_tasks check.

Yep makes sense

We'll probably rework this into a relationship instead pretty soon.

hmacdope added 12 commits January 31, 2023 16:02

try and change all refs

4e25700

finish renaming

8ad4eb3

few final fixes

a519bb7

Merge remote-tracking branch 'upstream/main' into taskhub

16b108e

black

4f50a22

test unclaimed tasks

038c42b

switch to using get_unclaimed_tasks

f7960f1

add getting and setting of ACTIONS weights

fff26ab

in progress

5513f05

add changes

7922472

get priority right way round

15d60c4

add test for none

8edacdb

hmacdope added the component-statestore label Feb 3, 2023

hmacdope added this to the Release 0.2.0 milestone Feb 3, 2023

hmacdope requested a review from dotsdl February 3, 2023 04:36

hmacdope changed the title ~~Move Task queing to Taskhubs~~ Move Task queuing to Taskhubs Feb 3, 2023

Black

b50388f

hmacdope mentioned this pull request Feb 4, 2023

Added results retrieval approach #56

Merged

Some edits from discussion

6635864

dotsdl assigned hmacdope Feb 7, 2023

Merge branch 'main' into taskhub

71ac8b0

dotsdl reviewed Feb 8, 2023

View reviewed changes

Comment thread alchemiscale/storage/statestore.py Outdated

dotsdl reviewed Feb 8, 2023

View reviewed changes

Comment thread alchemiscale/storage/statestore.py Outdated

dotsdl added 2 commits February 7, 2023 23:32

Merge branch 'main' into taskhub

2433540

Merge branch 'taskhub-merge' into taskhub

2f4131a

Queues -> Hubs

7f5f810

hmacdope added 3 commits February 10, 2023 15:09

fix task actioning and claiming

cda1ddf

fix claiming test

84675cf

clean up and add final tests

1070e97

hmacdope commented Feb 11, 2023

View reviewed changes

black!

1abee2a

Modifications from review

78b6c0f

Simplified where possible, adjustments to docs, grouping queries under transactions, etc.

dotsdl approved these changes Feb 14, 2023

View reviewed changes

dotsdl added 5 commits February 13, 2023 23:45

Black!

de4f40a

Removed nested check on task actioning

cc7b82c

Attempt to fix claim machinery for Task

60d0c6d

We'll probably rework this into a relationship instead pretty soon.

Fix key comparison in interface client test

024bf3e

Black!

8d8c6a2

dotsdl merged commit 3378128 into OpenFreeEnergy:main Feb 14, 2023

hmacdope mentioned this pull request Feb 19, 2023

Ability to set weight on a per-Task basis? #23

Closed

Uh oh!

Conversation

hmacdope commented Feb 3, 2023

Uh oh!

codecov-commenter commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hmacdope commented Feb 7, 2023

Uh oh!

dotsdl commented Feb 7, 2023

Uh oh!

dotsdl commented Feb 7, 2023

Uh oh!

dotsdl commented Feb 7, 2023

Uh oh!

dotsdl commented Feb 7, 2023

Uh oh!

Uh oh!

dotsdl commented Feb 8, 2023

Uh oh!

hmacdope commented Feb 8, 2023

Uh oh!

Uh oh!

dotsdl commented Feb 8, 2023

Uh oh!

hmacdope commented Feb 8, 2023

Uh oh!

hmacdope commented Feb 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hmacdope commented Feb 11, 2023

Uh oh!

dotsdl 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!

dotsdl Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Feb 3, 2023 •

edited

Loading

dotsdl Feb 14, 2023 •

edited

Loading