Add getters and setters for Task priority in AlchemiscaleClient #213

ianmkenney · 2023-12-05T23:28:53Z

Fixes #200

This PR implements the set_tasks_priority and get_tasks_priority methods of the AlchemiscaleClient.

codecov-commenter · 2023-12-05T23:49:26Z

Codecov Report

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

Comparison is base (8e98af3) 81.76% compared to head (9e22c8c) 81.89%.

Files	Patch %	Lines
alchemiscale/interface/api.py	16.00%	21 Missing ⚠️
alchemiscale/interface/client.py	84.09%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #213      +/-   ##
==========================================
+ Coverage   81.76%   81.89%   +0.12%     
==========================================
  Files          22       22              
  Lines        2781     2834      +53     
==========================================
+ Hits         2274     2321      +47     
- Misses        507      513       +6

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

The reversed naming was deliberate. This reverts commit 6208602.

Implemented * tasks_get_priority (api) * _get_task_priority (client) * get_tasks_priority (client) * get_task_priority (state store) * test_get_tasks_priority (client integration)

dotsdl

Looking good so far @ianmkenney! Definitely on the right track.

dotsdl · 2023-12-07T22:28:09Z

alchemiscale/interface/client.py

+            return list(chain.from_iterable(priorities))
+
+        try:
+            return asyncio.run(async_request(self))


Generally better to create the async_request(self) coroutine outside of here, because if asyncio.run fails (such as in a Jupyter notebook), then the Python interpreter will complain about an unexecuted coroutine hanging around.

I see I didn't do that for get_tasks_status, but did for set_tasks_status. You're welcome to propagate that practice there too in this PR.

Since the code for the status and priority methods for Tasks are practically identical, this may also be an opportunity to create generalized methods that both the status and priority getters and setters can use.

ianmkenney · 2023-12-08T22:00:53Z

Waiting for #212 to be merged. There will be some conflicts that should auto-resolve with a main merge once that's done.

dotsdl

Looking great @ianmkenney! Thank you for the detailed work here!

There's a few changes I'd like you to pursue before we're good to merge. I think the lion's share of the work is already done, so these are mostly sculpting.

alchemiscale/interface/client.py

dotsdl · 2023-12-16T21:20:58Z

alchemiscale/interface/client.py

+        """Set the priority of multiple Tasks.
+


Can you add a note here on what kind of integers are allowed for priority, and what these values mean? As-in, priority 1 is the highest priority, 2 the next highest, and so on, with the lowest priority allowed being 2**64 / 2 (max value of java long, since this gets stored in Neo4j).

I was wrong about this; max value of a java long is 2**64/2 - 1, or 2**63 - 1. Fixing this myself now.

Also adding guardrail in Neo4jStore.

dotsdl · 2023-12-19T00:43:55Z

alchemiscale/storage/statestore.py

+    def set_task_priority(self, task: Union[ScopedKey, List[ScopedKey]], priority: int):
+        if not priority >= 0:
+            raise ValueError("priority cannot be negative")
+
+        if isinstance(task, ScopedKey):
+            task = [task]


Just to simplify a bit and make it consistent with other methods, let's axe this method's ability to take a single task, since it avoids the asymmetry of input sometimes being a non-iterable but output always being an iterable.

We generally try to take this pattern at this lowest layer of the stack; it doesn't need to be user-friendly, but we aim for a fairly logical consistency where possible.

This will simplify the tests for these methods as well, reducing the input space.

Can you also add -> List[Optional[ScopedKey]] as the output?

And rename task to tasks. 😁

Can you also add a docstring?

Just to simplify a bit and make it consistent with other methods, let's axe this method's ability to take a single task, since it avoids the asymmetry of input sometimes being a non-iterable but output always being an iterable.

We generally try to take this pattern at this lowest layer of the stack; it doesn't need to be user-friendly, but we aim for a fairly logical consistency where possible.

This will simplify the tests for these methods as well, reducing the input space.

Agreed. This will require changing compute tests using the single task inputs. I was trying to preserve previous functionality. Might be worthwhile doing a general consistency sweep before the next release.

dotsdl · 2023-12-19T00:46:43Z

alchemiscale/storage/statestore.py

+                task_results.append(ScopedKey.from_str(scoped_key))
+        return task_results
+
+    def get_task_priority(self, tasks: List[ScopedKey]) -> List[Optional[int]]:


Please add a docstring here as well!

dotsdl · 2023-12-21T04:09:43Z

alchemiscale/interface/client.py

+            nest_asyncio.apply()
+            return asyncio.run(coro)
+
+    def _task_attribute_setter(


dotsdl · 2023-12-21T04:21:27Z

alchemiscale/interface/client.py

+        """Set the priority of multiple Tasks.
+


I was wrong about this; max value of a java long is 2**64/2 - 1, or 2**63 - 1. Fixing this myself now.

dotsdl

Fantastic work @ianmkenney! I added guardrails and checks for upper bound of priority; will merge following tests passing.

ianmkenney added 2 commits December 5, 2023 16:16

Added getters and setters for Task priority in AlchemiscaleClient

5583eea

Better function names for API

6208602

ianmkenney added 3 commits December 6, 2023 14:00

Revert "Better function names for API"

5a5165b

The reversed naming was deliberate. This reverts commit 6208602.

Added task priority getter method

8a191ff

Implemented * tasks_get_priority (api) * _get_task_priority (client) * get_tasks_priority (client) * get_task_priority (state store) * test_get_tasks_priority (client integration)

Fixed signature for empty test

5c8ab2c

dotsdl reviewed Dec 7, 2023

View reviewed changes

ianmkenney added 3 commits December 8, 2023 11:15

Pulled out common code for getter methods

3641247

Filled in the test_set_tasks_priority

c89f05a

Implemented task priority setter

4702ba3

ianmkenney added 3 commits December 8, 2023 15:12

Fixed test

3695ce1

Merge branch 'main' into 200-Task-priority-setter-getter

6a2bb43

Expanded tests

cf587a9

ianmkenney changed the title ~~[WIP] Add getters and setters for Task priority in AlchemiscaleClient~~ Add getters and setters for Task priority in AlchemiscaleClient Dec 12, 2023

ianmkenney marked this pull request as ready for review December 12, 2023 19:18

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

dotsdl added 3 commits December 16, 2023 13:41

Merge branch 'main' into 200-Task-priority-setter-getter

8d7dce5

Re-arranged methods in AlchemiscaleClient

20f5600

Remove unused non-async AlchemiscaleClient._set_task_status method

3dda058

dotsdl requested changes Dec 19, 2023

View reviewed changes

dotsdl and others added 3 commits December 18, 2023 18:24

Small formatting adjustments to cypher query

d540476

Added general setter method

5edcb14

Updated doctrings, function args, and tests

011e8cc

ianmkenney requested a review from dotsdl December 20, 2023 00:29

dotsdl reviewed Dec 21, 2023

View reviewed changes

dotsdl added 2 commits December 20, 2023 21:25

Added guardrail for largest java long (integer)

48a4a26

Added priority bounds check

cd88f81

dotsdl approved these changes Dec 21, 2023

View reviewed changes

Test fix

139d03d

Test content fix; avoiding regex errors

9e22c8c

dotsdl merged commit 81d977b into main Dec 21, 2023
4 checks passed

dotsdl deleted the 200-Task-priority-setter-getter branch December 21, 2023 17:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add getters and setters for Task priority in AlchemiscaleClient #213

Add getters and setters for Task priority in AlchemiscaleClient #213

ianmkenney commented Dec 5, 2023

codecov-commenter commented Dec 5, 2023 •

edited

dotsdl left a comment

dotsdl Dec 7, 2023

dotsdl Dec 7, 2023

ianmkenney commented Dec 8, 2023

dotsdl left a comment

dotsdl Dec 16, 2023

dotsdl Dec 21, 2023 •

edited

dotsdl Dec 21, 2023

dotsdl Dec 19, 2023

dotsdl Dec 19, 2023

dotsdl Dec 19, 2023

dotsdl Dec 19, 2023

ianmkenney Dec 19, 2023

dotsdl Dec 19, 2023

dotsdl Dec 21, 2023

dotsdl Dec 21, 2023 •

edited

dotsdl left a comment

Add getters and setters for Task priority in AlchemiscaleClient #213

Add getters and setters for Task priority in AlchemiscaleClient #213

Conversation

ianmkenney commented Dec 5, 2023

codecov-commenter commented Dec 5, 2023 • edited

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

ianmkenney commented Dec 8, 2023

dotsdl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotsdl Dec 21, 2023 • edited

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 Dec 21, 2023 • edited

Choose a reason for hiding this comment

dotsdl left a comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 5, 2023 •

edited

dotsdl Dec 21, 2023 •

edited

dotsdl Dec 21, 2023 •

edited