Add `weight` kwarg to `AlchemiscaleClient.action_tasks` method #209

ianmkenney · 2023-12-04T21:21:08Z

In this PR, the kwarg weight has been added to the AlchemiScaleClient.action_tasks method, where its value is added to the data payload. Validation of the weight (restricted to be between 0 and 1, see #208 ) is handled at the API layer. The Neo4jStore also now has a weight kwarg that defaults to 0.5 (note that the previous weight was 1 and was hard coded).

fixes #202

dotsdl

Great work @ianmkenney! In reviewing I realized we should pivot our approach from this a bit, using some existing functionality in Neo4jStore instead of modifying its action_tasks method.

Some additional points/questions:

We need an explicit test for Task weight setting via the AlchemiscaleClient in tests/integration/interface/client/test_client.py.
Let's change the default weight on the AlchemiscaleClient.action_tasks method to None, and have this mean that no change is made to weights for Tasks already actioned, and newly-actioned Tasks retain the default of 0.5.

dotsdl · 2023-12-07T12:48:03Z

alchemiscale/interface/api.py

    taskhub_sk = n4js.get_taskhub(sk)
-    actioned_sks = n4js.action_tasks(tasks, taskhub_sk)
+    actioned_sks = n4js.action_tasks(tasks, taskhub_sk, weight=weight)


In reviewing the Neo4jStateStore codebase, I realized that we already have a set_task_weights method. Instead of modifying Neo4jStore.action_tasks to also set the weight away from the default of 0.5, let's use the set_task_weights method to accomplish this following our use of action_tasks here.

dotsdl · 2023-12-07T12:49:52Z

alchemiscale/interface/api.py

+    if not 0 <= weight <= 1:
+        raise HTTPException(
+            status_code=status.HTTPS_400_BAD_REQUEST,
+            detail=f"weight must between 0.0 and 1.0 (inclusive), the "
+            "provided weight was: {weight}",
+        )


As we did in #212, can we put these bounds checks in Neo4jStore.set_task_weights? Then we'll try..except our call to that method and raise an HTTPException in the API endpoint here.

dotsdl · 2023-12-07T12:52:36Z

alchemiscale/interface/api.py

@@ -490,14 +490,22 @@ def action_tasks(
    network_scoped_key,
    *,
    tasks: List[ScopedKey] = Body(embed=True),
+    weight: float = Body(embed=True),


Since Neo4jStore.set_task_weights can also handle setting a different weight per Task, can we accept a list of weights here in addition to accepting a float? Could then make the typing: Union[List[float],float]. We would then build a dict in this method of {task_sk: weight} key-value pairs from the two lists tasks and weight.

dotsdl · 2023-12-07T12:53:23Z

alchemiscale/storage/statestore.py

-                CREATE (th)-[ar:ACTIONS {{weight: 1.0}}]->(task)
+                CREATE (th)-[ar:ACTIONS {{weight: {weight}}}]->(task)


Can revert changes to this method, since let's go ahead and use set_task_weights instead.

I'd assume we'd actually want this at 0.5 since it seems to be the new default. Or is there a reason to make it 1.0 here?

Sorry, yes. Make the new default 0.5.

dotsdl · 2023-12-07T12:55:22Z

alchemiscale/interface/client.py

@@ -612,7 +612,7 @@ def get_transformation_status(
        return status_counts

    def action_tasks(
-        self, tasks: List[ScopedKey], network: ScopedKey
+        self, tasks: List[ScopedKey], network: ScopedKey, weight: float = 0.5


Let's make this method also take weight as a list of floats, making the typing signature Union[List[float],float]. This will allow users to set a whole swath of Task weights with a single call to this method, which would be more performant overall. I think the rest of this method can then remain unchanged.

Let's also set the default value of weight to None. What this means for users:

using action_tasks without specifying weight will set the weight to 0.5 for newly-actioned Tasks; Tasks already actioned on the given AlchemicalNetwork will experience no change in their weights.

using action_tasks while specifying weight will set the weights of the actioned Tasks, even if they were already actioned.

dotsdl

A couple small notes.

dotsdl · 2023-12-07T13:18:39Z

alchemiscale/tests/integration/storage/test_statestore.py

@@ -918,12 +918,12 @@ def test_get_set_weights(self, n4js: Neo4jStore, network_tyk2, scope_test):

        # weights should all be the default 1.0


Fix comment.

dotsdl · 2023-12-07T13:18:46Z

alchemiscale/tests/integration/storage/test_statestore.py

@@ -918,12 +918,12 @@ def test_get_set_weights(self, n4js: Neo4jStore, network_tyk2, scope_test):

        # weights should all be the default 1.0
        weights = n4js.get_task_weights(task_sks, taskhub_sk)
-        assert all([w == 1.0 for w in weights])
+        assert all([w == 0.5 for w in weights])

        # set weights on the tasks to be all 10


Fix comment.

* Test the use of including weights with the action_task method in the AlchemiscaleClient

…2-weight-action_tasks

dotsdl

Looking great @ianmkenney! I think we need some additional test coverage for the AlchemiscaleClient.action_tasks usage, but otherwise looking very good!

Once you've added this additional coverage, we should be good to merge!

dotsdl · 2023-12-11T11:23:55Z

alchemiscale/tests/integration/interface/client/test_client.py

+                network_sk,
+                weight,
+            )
+


Can you add a test that calls action_tasks with weight None and checks with n4j_preloaded.get_task_weights that the weights are unchanged?

Can you also check that if you call action_tasks on already-actioned Tasks with new weights that these get set appropriately?

dotsdl · 2023-12-11T11:25:18Z

alchemiscale/tests/integration/interface/client/test_client.py

+        task_sks = user_client.create_tasks(transformation_sk, count=3)
+
+        if isinstance(weight, list):
+            weight = weight * len(task_sks)


This will create a list of all the same weights, which isn't bad, but I think it would be more thorough to test if we set a list of different weights that this works as expected. You can use n4js_preloaded.get_task_weights to get at this information.

Reworked how this is done

codecov-commenter · 2023-12-11T11:43:46Z

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (a7b0eab) 81.94% compared to head (8af3bb1) 82.03%.

Files	Patch %	Lines
alchemiscale/interface/api.py	0.00%	10 Missing ⚠️
alchemiscale/storage/statestore.py	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #209      +/-   ##
==========================================
+ Coverage   81.94%   82.03%   +0.09%     
==========================================
  Files          22       22              
  Lines        2780     2795      +15     
==========================================
+ Hits         2278     2293      +15     
  Misses        502      502

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dotsdl

Great work @ianmkenney! Please merge if satisfied!

dotsdl · 2023-12-15T18:49:27Z

alchemiscale/tests/integration/interface/client/test_client.py

+            # task weights
+            user_client.action_tasks(task_sks, network_sk, weight=None)
+
+            task_weights = n4js.get_task_weights(task_sks, th_sk)
+            assert task_weights == _weight


dotsdl · 2023-12-15T18:54:42Z

alchemiscale/tests/integration/interface/client/test_client.py

+            task_weights = n4js.get_task_weights(task_sks, th_sk)
+            assert task_weights == _weight
+
+    def test_action_tasks_update_weights(


Very nice! Thank you!

dotsdl · 2023-12-15T18:54:55Z

alchemiscale/tests/integration/interface/client/test_client.py

+        [
+            (None, False),
+            (1.0, False),
+            ([0.25, 0.5, 0.75], False),


Much better!

ianmkenney and others added 3 commits December 4, 2023 14:12

Exposed actioned task weights to user

00fd13b

Merge branch 'main' into 202-weight-action_tasks

27bd40d

Merge branch 'main' into 202-weight-action_tasks

e85042f

ianmkenney requested a review from dotsdl December 5, 2023 18:27

dotsdl requested changes Dec 7, 2023

View reviewed changes

ianmkenney added 2 commits December 7, 2023 13:16

Expand tests and allow greater user control

c8e8250

* Test the use of including weights with the action_task method in the AlchemiscaleClient

Merge remote-tracking branch 'origin/202-weight-action_tasks' into 20…

088c209

…2-weight-action_tasks

ianmkenney requested a review from dotsdl December 8, 2023 17:41

Doc adjustments in AlchemiscaleClient.action_tasks

d331fd7

dotsdl approved these changes Dec 11, 2023

View reviewed changes

ianmkenney added 2 commits December 12, 2023 11:06

Merge branch 'main' into 202-weight-action_tasks

315887f

Expand action_tasks testing

8af3bb1

ianmkenney requested a review from dotsdl December 12, 2023 23:18

ianmkenney mentioned this pull request Dec 14, 2023

Default ACTION weight difference between user interface and statestore levels #215

Closed

Added one more bit to test_action_tasks_update_weights

516ffb2

dotsdl approved these changes Dec 15, 2023

View reviewed changes

ianmkenney merged commit 8e98af3 into main Dec 15, 2023
4 checks passed

ianmkenney deleted the 202-weight-action_tasks branch December 15, 2023 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `weight` kwarg to `AlchemiscaleClient.action_tasks` method #209

Add `weight` kwarg to `AlchemiscaleClient.action_tasks` method #209

ianmkenney commented Dec 4, 2023

dotsdl left a comment

dotsdl Dec 7, 2023

dotsdl Dec 7, 2023

dotsdl Dec 7, 2023

dotsdl Dec 7, 2023

ianmkenney Dec 7, 2023

dotsdl Dec 7, 2023

dotsdl Dec 7, 2023

dotsdl Dec 7, 2023

dotsdl left a comment

dotsdl Dec 7, 2023

dotsdl Dec 7, 2023

dotsdl left a comment

dotsdl Dec 11, 2023

ianmkenney Dec 12, 2023

dotsdl Dec 11, 2023

ianmkenney Dec 12, 2023

codecov-commenter commented Dec 11, 2023 •

edited

Loading

dotsdl left a comment

dotsdl Dec 15, 2023

dotsdl Dec 15, 2023

dotsdl Dec 15, 2023

		CREATE (th)-[ar:ACTIONS {{weight: 1.0}}]->(task)
		CREATE (th)-[ar:ACTIONS {{weight: {weight}}}]->(task)

		@@ -918,12 +918,12 @@ def test_get_set_weights(self, n4js: Neo4jStore, network_tyk2, scope_test):

		# weights should all be the default 1.0

Add weight kwarg to AlchemiscaleClient.action_tasks method #209

Add weight kwarg to AlchemiscaleClient.action_tasks method #209

Conversation

ianmkenney commented Dec 4, 2023

dotsdl left a 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

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

dotsdl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotsdl left a 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

Choose a reason for hiding this comment

codecov-commenter commented Dec 11, 2023 • edited Loading

Codecov Report

dotsdl left a 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

Add `weight` kwarg to `AlchemiscaleClient.action_tasks` method #209

Add `weight` kwarg to `AlchemiscaleClient.action_tasks` method #209

codecov-commenter commented Dec 11, 2023 •

edited

Loading