AlchemiscalClient async+bulk for results, other methods; add request, response compression for large objects #150

dotsdl · 2023-06-23T18:30:33Z

This PR adds async/await methods to the AlchemiscaleBaseClient, as well as usage of these methods to the AlchemiscaleClient for use by users.

It also establishes the pattern for /bulk endpoints on API services, which are not strictly RESTful but do allow for much greater performance when requesting operations on many ScopedKeys in a single call.

This PR adds performance improvements using the above to:

AlchemiscaleClient.get_tasks_status
AlchemiscaleClient.set_tasks_status
AlchemiscaleClient.get_transformation_results
AlchemiscaleClient.get_transformation_failures
AlchemiscaleClient.get_task_results
AlchemiscaleClient.get_task_failures

This PR also adds use of gzip compression for large requests and responses between the AlchemiscaleBaseClient and the API services. For the AlchemiscaleClient, this optimization is by default applied to:

AlchemiscaleClient.create_network
AlchemiscaleClient.get_network
AlchemiscaleClient.get_transformation
AlchemiscaleClient.get_chemicalsystem
AlchemiscaleClient.get_transformation_results
AlchemiscaleClient.get_transformation_failures
AlchemiscaleClient.get_task_results
AlchemiscaleClient.get_task_failures

… request

Need to implement the `set_tasks_status` async/await version next

Using/abusing an async lock for this.

We now hit the bulk API endpoint with async using batches. Get 45s locally for 10,000 tasks.

We're seeing what looks like weird performance issues using `httpx` for synchronous requests vs. `requests`. Sticking with `requests` for synchronous, `httpx` for async for now.

For consistency.

codecov-commenter · 2023-06-24T00:52:14Z

Codecov Report

Patch coverage: 76.20% and project coverage change: -1.38 ⚠️

Comparison is base (f761d36) 83.67% compared to head (f70a6c9) 82.30%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #150      +/-   ##
==========================================
- Coverage   83.67%   82.30%   -1.38%     
==========================================
  Files          21       21              
  Lines        2426     2656     +230     
==========================================
+ Hits         2030     2186     +156     
- Misses        396      470      +74

Impacted Files	Coverage Δ
alchemiscale/compute/service.py	`81.62% <ø> (ø)`
alchemiscale/interface/api.py	`41.78% <25.00%> (-1.87%)`	⬇️
alchemiscale/base/client.py	`77.91% <71.73%> (-9.70%)`	⬇️
alchemiscale/interface/client.py	`92.27% <85.05%> (-3.71%)`	⬇️
alchemiscale/base/api.py	`86.48% <94.73%> (+1.38%)`	⬆️
alchemiscale/compute/api.py	`66.30% <100.00%> (+1.13%)`	⬆️
alchemiscale/storage/statestore.py	`94.12% <100.00%> (-0.06%)`	⬇️

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

…alls Now by default compress retrievals of AlchemicalNetwork, Transformation, and ChemicalSystem. Also compress retrieval of ProtocolDAGResults.

dotsdl · 2023-06-27T02:12:24Z

@hmacdope almost done with this one! Could I get a review from you when you get the chance?

Also, added `rich`-based progress bar to result retrieval.

hmacdope

Great work! Few queries, see comments. :)

alchemiscale/interface/api.py

hmacdope · 2023-06-27T02:40:12Z

alchemiscale/interface/api.py

+    token: TokenData = Depends(get_token_data_depends),
+) -> List[Union[str, None]]:
+    status = TaskStatusEnum(status)
+    if status not in (


Should this be if status in ...? I could be missing something but I thought we didn't want to mutate state of waiting, invalid or deleted tasks, same with the HTTPException below, seems to suggest that status can be changed from terminal state invalid, deleted etc.

Here status isn't the current status of the Tasks we want to set; it's the desired status. waiting, invalid, and deleted are all set-able by the user, at least under most conditions (e.g. going from 'complete' to 'waiting' isn't allowed by the underlying Neo4jStore method).

Ah I understand, sorry about that.

hmacdope · 2023-06-27T02:41:41Z

alchemiscale/interface/api.py

+        except HTTPException:
+            tasks_updated.append(None)
+        else:
+            tasks_updated.extend(n4js.set_task_status([task_sk], status))


Still uses one at a time set unlike /bulk/tasks/status/get.

Correct; we can optimize this further, but I think I'm running out of time on this one. Since we have more complex queries to deal with for status setting, I'd like to make that a future PR.

Perfect! raise an issue, and happy to move on.

I may have managed to get this one in. 😁

hmacdope · 2023-06-27T02:45:41Z

alchemiscale/interface/client.py


    def get_tasks_status(
-        self, tasks: Union[ScopedKey, List[ScopedKey]]
+        self, tasks: List[ScopedKey], batch_size=1000


Love the batching ❤️

alchemiscale/storage/statestore.py

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

alchemiscale/tests/integration/interface/test_api.py

hmacdope · 2023-06-27T03:02:01Z

alchemiscale/base/client.py

@@ -201,13 +329,18 @@ def _query_resource(self, resource, params=None):

    @_retry
    @_use_token
-    def _get_resource(self, resource, params=None):
+    def _get_resource(self, resource, params=None, compress=False):


Did we want to compress by default here and below? Up to you. It seemed set to default to True on a lot of the interface/client.py methods.

My thinking here is to keep compression on these private methods opt-in. Not all post and get calls will benefit from compression, especially for tiny requests/responses, so making it something we enable as the default on specific user-facing methods made the most sense to me.

hmacdope · 2023-06-27T03:02:34Z

alchemiscale/base/client.py

+
+        return resp.json()
+
+    @staticmethod


Haha thank the standard lib: https://docs.python.org/3/library/itertools.html

I think Python 3.12 will have a itertools.batched we can just switch to.

hmacdope

Great work! Few queries, see comments. :)

Also made set_tasks_status work as async/batch, same as get_tasks_status

Also, add scope-based ordering to query outputs.

We use the same patterns we applied for `get_tasks_status`.

dotsdl added 15 commits June 17, 2023 11:48

Small optimization to Neo4jStore.get_task_status

6d98952

AlchemiscaleClient get_tasks_status and set_tasks_status single…

703dd71

… request

Working async/await implementation of get_tasks_status

1ce1de9

Need to implement the `set_tasks_status` async/await version next

Merge branch 'main' into issue-126-task-status-async

1686ca3

Black!

e8f859f

May have optimized get_task_status for bulk usage

443470a

Merge branch 'main' into issue-126-task-status-single

19b8bd0

Faster implementation, single query for get_task_status

66cbfa8

Think I've got token refreshes working with async

e535729

Using/abusing an async lock for this.

Black!

287326b

Merge branch 'issue-126-task-status-single' into user-async-batch

2bf4971

Hybrid approach complete for get_tasks_status

491d2a4

We now hit the bulk API endpoint with async using batches. Get 45s locally for 10,000 tasks.

Small edit

d80dfce

Black!

c3f8d90

Switched to using requests for synchronous HTTP via clients

86b8417

We're seeing what looks like weird performance issues using `httpx` for synchronous requests vs. `requests`. Sticking with `requests` for synchronous, `httpx` for async for now.

dotsdl linked an issue Jun 23, 2023 that may be closed by this pull request

Async or more efficient way of getting the results from a network #140

Closed

This was referenced Jun 23, 2023

[WIP] AlchemiscaleClient Task Status Async #146

Closed

[WIP] AlchemiscaleClient Task Status Batched #147

Closed

dotsdl changed the title ~~AlchemiscalClient async+bulk for results, other methods~~ [WIP] AlchemiscalClient async+bulk for results, other methods Jun 23, 2023

dotsdl added 5 commits June 23, 2023 11:34

Allow string form of Task scoped keys in get_tasks_status

31675bb

Async implementation for result retrieval via AlchemiscaleClient.

2ed8c0a

Black!

e2c0307

Synchronous request token back to using requests

d9e580b

For consistency.

Only want solvent transformations computed for speed of test suite

97a962d

dotsdl added 5 commits June 23, 2023 17:55

Merge branch 'main' into user-async-batch

d9b4278

Merge branch 'main' into user-async-batch

7860c27

Nondeterministic test fixes

39732e9

Merge branch 'main' into user-async-batch

5d99f26

Merge branch 'main' into user-async-batch

6180b03

dotsdl added 5 commits June 26, 2023 17:20

Black!

d414ff3

Added ability to set compression for AlchemiscaleClient.create_network

7e33c47

Black!

e012ad4

Added compression of responses for certain AlchemiscaleClient.get c…

8786f09

…alls Now by default compress retrievals of AlchemicalNetwork, Transformation, and ChemicalSystem. Also compress retrieval of ProtocolDAGResults.

Black!

2f83850

dotsdl changed the title ~~[WIP] AlchemiscalClient async+bulk for results, other methods~~ [WIP] AlchemiscalClient async+bulk for results, other methods; add request, response compression for large objects Jun 27, 2023

dotsdl requested a review from hmacdope June 27, 2023 02:12

dotsdl added 2 commits June 26, 2023 19:36

Added compress control to all result/failure AlchemiscaleClient methods

fae60ff

Also, added `rich`-based progress bar to result retrieval.

Black!

9ca8356

hmacdope reviewed Jun 27, 2023

View reviewed changes

dotsdl added 2 commits June 26, 2023 20:45

Added more progress output to various methods on AlchemiscaleClient

3864393

Also made set_tasks_status work as async/batch, same as get_tasks_status

Black!

23f499b

dotsdl changed the title ~~[WIP] AlchemiscalClient async+bulk for results, other methods; add request, response compression for large objects~~ AlchemiscalClient async+bulk for results, other methods; add request, response compression for large objects Jun 27, 2023

dotsdl linked an issue Jun 27, 2023 that may be closed by this pull request

[ENH] create_network can be very slow with complexes #129

Closed

dotsdl added 2 commits June 26, 2023 22:39

Added exposure of verify kwarg to compute service

3b85619

Black

6c51997

dotsdl linked an issue Jun 27, 2023 that may be closed by this pull request

Raise exception when non-specific Scope given when calling AlchemiscaleClient.create_network #133

Closed

dotsdl added 8 commits June 26, 2023 22:49

Close #133

c1d9e9a

Black

685c411

Added finer grained control over compression for create_network

1a49f2d

Fixes from @hmacdope review

9169b73

Add nest_asyncio for AlchemiscaleClient cases run in envs like Jupyter

42c1a23

Also, add scope-based ordering to query outputs.

Black

41b2239

set_tasks_status methods now run in single query for many Tasks

5d6a09f

We use the same patterns we applied for `get_tasks_status`.

Refinements to terminal set_task_status methods

f70a6c9

dotsdl merged commit dab275b into main Jun 29, 2023
3 checks passed

dotsdl deleted the user-async-batch branch June 29, 2023 07:02

dotsdl mentioned this pull request Jun 5, 2024

AlchemiscaleClient get_tasks_status and set_tasks_status slow for many tasks #148

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AlchemiscalClient async+bulk for results, other methods; add request, response compression for large objects #150

AlchemiscalClient async+bulk for results, other methods; add request, response compression for large objects #150

dotsdl commented Jun 23, 2023 •

edited

Loading

codecov-commenter commented Jun 24, 2023 •

edited

Loading

dotsdl commented Jun 27, 2023

hmacdope left a comment

hmacdope Jun 27, 2023

dotsdl Jun 27, 2023

hmacdope Jun 27, 2023

hmacdope Jun 27, 2023

dotsdl Jun 27, 2023

hmacdope Jun 27, 2023

dotsdl Jun 29, 2023

hmacdope Jun 27, 2023 •

edited

Loading

dotsdl Jun 27, 2023

hmacdope Jun 27, 2023

dotsdl Jun 27, 2023

hmacdope Jun 27, 2023

dotsdl Jun 27, 2023

dotsdl Jun 27, 2023

hmacdope left a comment

AlchemiscalClient async+bulk for results, other methods; add request, response compression for large objects #150

AlchemiscalClient async+bulk for results, other methods; add request, response compression for large objects #150

Conversation

dotsdl commented Jun 23, 2023 • edited Loading

codecov-commenter commented Jun 24, 2023 • edited Loading

Codecov Report

dotsdl commented Jun 27, 2023

hmacdope 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

hmacdope Jun 27, 2023 • edited Loading

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

hmacdope left a comment

Choose a reason for hiding this comment

dotsdl commented Jun 23, 2023 •

edited

Loading

codecov-commenter commented Jun 24, 2023 •

edited

Loading

hmacdope Jun 27, 2023 •

edited

Loading